Determine when all prefs have been written to disk

[redmunds]

This is for #7683.

This does not seem to always work. Posting for initial review.

[RaymondLim]

@redmunds I tried it with your branch and I got crash on quit most of the time with only one exception. That exception is when I have some windows update running at the background.

[redmunds]

@RaymondLim I know that this is not the final fix. I created the pull request so you and @dangoor could look at the code and let me know if it's on the right track, cases that were missed, etc.

[RaymondLim]

@redmunds I put new discoveries in the issue. Thanks for fixing it. I think you're on the right track since it fixes the issue in the case of making some changes in the document. So I believe if you can resolve the dangling promises, then we find the right solution.

[redmunds]

@RaymondLim @dangoor With this branch, it now only seems to crash if you Quit immediately after opening. I'm wondering if preferences are still being initialized, but I was able to capture console once and I did not see that code tried to write a pref after being finalized (i.e. line 1563 ). Maybe need to add a Deferred object when loading prefs?

[dangoor]

@redmunds Yes, this does look like the right track to me.

There would be a lot more potential places where it could crash immediately after opening if reads were a problem, too.

There are promises for when preferences and viewstate are done loading, if that turned out to be the cause of the crash on quit. PreferencesManager.ready and PreferencesManager._smUserScopeLoading.

[redmunds]

@dangoor @RaymondLim I added those promises, but I still crash sometimes when closing immediately after startup. I suppose there could be some non-Preferences startup code that's causing this. Anyway, try it out and let me know how it works.

[dangoor]

@redmunds Does it make sense to land this and then continue trying to track down other possible sources of the crash?

[redmunds]

@dangoor If you agree that there's nothing more that can be done as far as waiting on the prefs system (i.e. the problem is elsewhere), then, yes, I think this should merged into master.

[dangoor]

@redmunds I just took a look at _finalize more closely a noticed a couple of things.

GitHub doesn't want to show me the diff for this. _finalize (pasted below for reference):

// Return the promise for the `_nextSaveDeferred` if there is one.
// If there isn't, return the promise for the current save operation (if there is one).
// If there isn't one of those, return a resolved promise.
var deferred = this._nextSaveDeferred || this._saveInProgress || (new $.Deferred()).resolve();

deferred.done(function () {
    this.finalized = true;
});

return deferred.promise();

1. The callback in deferred.done is not a bound function... so this.finalized = true is not actually setting finalized on the right object, right?
2. _nextSaveDeferred
   It's possible for _nextSaveDeferred to be unset but _saveInProgress to be set. So, the _saveInProgress promise is returned. If save() is called then, _finalized is still not set, so _nextSaveDeferred will be set. Net result is that under this circumstance, it could actually try to save again even though it's supposed to be finalizing.
3. Async promises

It's better to assume that promises resolve asynchronously, even if that's not always the case with jQuery promises. If promises can resolve asynchronously, then it would be possible for a save to sneak in before this.finalized = true is run. So, something like:

// Return the promise for the `_nextSaveDeferred` if there is one.
// If there isn't, return the promise for the current save operation (if there is one).
// If there isn't one of those, return a resolved promise.
var deferred = this._nextSaveDeferred || this._saveInProgress;

if (!deferred) {
    this.finalized = true;
    return (new $.Deferred()).resolve().promise();
}

deferred.done(function () {
    this.finalized = true;
}.bind(this));

return deferred.promise();

may be safer.

[redmunds]

@dangoor Changes pushed.

1. Well, that's embarrassing.
2. I was thinking that the save() call could cause some other saves to get done, and we'd want those to execute -- is that not true? If not, then this.finalize should be set to true immediately, right?
3. Done.

Status: I did more testing with this fix and narrowed it down some more:

• Crash on close only seems to happen if there is a file that gets opened at startup.
• Only happens if file is JavaScript or is an HTML file that references a JS file
• If I hit Ctrl-Q before file is shown in editor (i.e. can still see "code the web"), then there's no crash
• If I wait a few seconds after file is shown in editor before Ctrl-Q, then there's no crash
• Best way to reproduce crash is to hit Ctrl-Q as soon as you see any file contents

So, the remaining crash seems to point to JS Code Hints.

[dangoor]

The problem with the number 2 case in my comment above is that the deferred returned by finalize will be the wrong deferred if save() is called after finalize() but before the last save can finish. In other words, the caller to finalize will think things are finalized even though the next save is allowed to proceed. This is probably an unlikely scenario. Perhaps the best thing is to just set the finalized flag right away.

[redmunds]

@dangoor Immediately setting finalize = true; causes preference saving to fail because of the flag itself, so I left it like it is.

I checked in some JSDoc fixes and opened this JSDoc bug: jbalsas/apify#52

[dangoor]

I'm still a little worried that if we leave a possibility open for this condition to be hit that we're going to see bugs on this filed down the line and have to go track it down afresh. If we do this in _finalize(), I think we can be sure that all saves have completed before the _finalize() deferred is resolved:

var deferred = this._nextSaveDeferred,
    self = this;

if (!deferred) {
    deferred = new $.Deferred();
    function checkForSaveAndFinalize() {
        if (self._saveInProgress) {
            self._saveInProgress.done(checkForSaveAndFinalize);
        } else {
            self.finalized = true;
            deferred.resolve();
        }
    }
    checkForSaveAndFinalize();
} else {
    deferred.done(function () {
        self.finalized = true;
    });
}

return deferred.promise();

What do you think?

Thanks for the doc fixes!

[redmunds]

@dangoor Ready for another review.

I see -- we need to keep waiting until last last _saveInProgress promise completes.

In your sample code, if there is a _nextSaveDeferred, we'll resolve when _nextSaveDeferred resolves, but a new save() could also come in after that one.

But, there will only ever be a _nextSaveDeferred, if there is already a _saveInProgress and it will become the new _saveInProgress as soon as previous _saveInProgress resolves, so only need to wait for _saveInProgress, right?

It also simplifies the code a bit.

[dangoor]

Good call. That looks much nicer and seems reasonable. As long as this works as well as the last version for you, then this is ready to merge.

[redmunds]

@dangoor Yes, I am seeing same results, so go ahead and merge.

Do you have any suggestions for a JS Code Hint promise that can be waited on to try to fix remaining issue described in this comment? Looks like ScopeManager._readyPromise might be it, but we'd have to create a new CodeHintManager API to access it.

[dangoor]

Branch merged (but not deleted... it probably makes more sense to start a new PR for the code hints stuff. Go ahead and delete the branch if you'll be making a new one.)

Augmenting CodeHintManager for this specifically seems like we're spreading around knowledge of files that we're waiting on in a way that's hard to track. Perhaps we need the counterpart to AppInit (AppShutdown?) that calls callbacks which can return a promise for when it's safe to shutdown?