[HOTFIX] Only update stat and clear contents when old stat is newer than current stat.

[petetnt]

🔥 Potentially fixes #11826 🔥

There is a long running issue with Brackets where the async stat implementation for fetching the timestamp (stat._mtime) can cause rare 🐎 race conditions 🐎. See this issue: #295

This is very rare and usually doesn't matter much. But it does matter when FileSystem.js does this:

      // Update stat and clear contents, but only if out of date
      if (!(stat && oldStat && stat.mtime.getTime() === oldStat.mtime.getTime())) {
         entry._clearCachedData();
         entry._stat = stat;
         this._fireChangeEvent(entry);
      }

Due to the race condition stat.mtime is sometimes older than oldStat.mtime, causing a change event to happen and cache get cleared, nullifying the history -> causing #11826.

Q & A

Q: Brackets has been like that for a long time. Why did this start occurring in 1.5.?

A: I only got speculation, but as it is a race condition, I guess increased code base plays an unfortunate part here: some additional file system operations or anything else made the race condition much more reoccurring.

Q: Does this really fix #11826

A: I... don't know for sure. There's no steps to reproduce #11826 with even 1% success rate

Q: Why do you think it fixes #11826

A: It fixes the issue with my corrupt-brackets-history extension: I tried to save 10000 times in a row and history was kept intact. Without the fix the issue usually occurs before the 50th save.

Q: Does this have other side-effects

A: Not sure. Most likely not, but I am a bit worried that it might have unwanted effects on some external changes. Obviously it should be better to fix the race condition itself, but this is just a hotfix.

Q: Why is the history getting reseted

A: The change event getting erroneously called eventually leads to this chain:

FileSyncManager#reloadChangedDocs 
-> FileSyncManager#reloadDoc
-> Document#refreshText 
-> Editor#resetText

Editor#resetText which calls:

 // Make sure we can't undo back to the empty state before setValue(), and mark
// the document clean.
this._codeMirror.clearHistory();
this._codeMirror.markClean();

[lkcampbell]

@petetnt I have been racking my brain and I can't think of a reason why we would want to even consider a scenario where the old modified date is newer than the new modified date.

The comment before the code: // Update stat and clear contents, but only if out of date seems to indicate that we should only be testing for a single scenario: new modified date is newer than old modified date.

Based on these two arguments, your fix seems solid. But I have the same reservations as you have about changing code at such a low level.

Maybe someone else who knows the file system code better than I do has some insight as well.

[abose]

🚒
@swmitra @nethip

[abose]

the fix looks safe[though a bit wierd] to me.
@petetnt Could you target the change to the release branch too.
We are going to refresh silently the release builds of 1.6 with this change.
That will also give a good enough sample set for us to see if this fix is working.

[petetnt]

@abose See #12195 which is targeted against release.

[abose]

Merging to master as the release discussion is still going on for 1.6.
This will give enough testing in the brackets-dev groups.
Merging.

[abose]

@petetnt Can't thank you enough for this fix 😃

[petetnt]

@abose 😹 Thanks!

🎂