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

[petetnt]

This is the same patch as #12175, but targeted against release branch.

🔥 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 @abose What's the update on this fix? I know we don't have a lot of feedback on benefits and problems of merging it, but all the feedback we do have is positive. Is this going in for 1.7? If so, we should at least assign it a milestone.

[petetnt]

@lkcampbell FWIW I have been using it since I figured it out and haven't ran into the undo-issue myself nor into any bugs caused by it. Like you said, the other feedback we have gotten is rather minimal but at least 100% positive.

[redmunds]

This looks good to me. @peterflynn Can you think of any problem that this very simple change could cause?

I am curious as to why this PR is targetting the release branch because I don't think it's active. This change should be made in master branch, right?

[abose]

We were thinking of triggering a minor release by updating the 1.6 release builds with the patch as this is a critical issue. This PR was initially targetted to master. But as it is already time for the next release, maybe this will go to 1.7. Discussions still going on as triggering a release process is a bit expensive even for a minor release.

[petetnt]

Closing this as it landed to master on #12175

[abose]

We will be doing a minor build refresh of 1.6 with this fix soon.
Merging.

[petetnt]

@abose Would it be possible to include #12145 in the refreshed release? It has prompted 10+ new issues after the launch of 1.6.

[abose]

We had a discussion about this; But putting in too many bug fixes in a build refresh will increase the testing overhead as it is a core change.
I am up for taking #12145 in the refresh.
Tagging @swmitra for opinion.

[swmitra]

@abose @petetnt We should include #12145 for the refresh.