Closed Bug 1177310 Opened 9 years ago Closed 8 years ago

[e10s] Stop using CPOWs on application shutdown

Categories

(Firefox :: Session Restore, defect)

34 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
e10s m8+ ---
firefox45 --- fixed

People

(Reporter: ttaubert, Assigned: mconley)

References

Details

Attachments

(4 files, 1 obsolete file)

      No description provided.
Tim, are you the right assignee here?
Assignee: nobody → ttaubert
tracking-e10s: --- → m8+
Started to look into it, I have a few ideas but not enough time to work on it currently. Hope I can get back to this soon, the control center work for 42 is keeping me busy.
Assignee: ttaubert → mconley
Hey Tim, I'm assigning this to Mike for now in case he can get to it before you free up. If you can get to it before he does, feel free to steal it back.
Assignee: mconley → wmccloskey
Assignee: wmccloskey → mconley
No longer blocks: 1223533
Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r?Yoric
Attachment #8687428 - Flags: review?(dteller)
Bug 1177310 - Don't flush windows synchronously on application shutdown. r?Yoric
Attachment #8687429 - Flags: review?(wmccloskey)
Attachment #8687429 - Flags: review?(dteller)
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

https://reviewboard.mozilla.org/r/25207/#review22801

This might be okay. I just want to find out more here.

::: browser/components/sessionstore/SessionStore.jsm:1370
(Diff revision 1)
> +      this.saveStateDelayed();

What's this saveStateDelayed line for? I'm assuming that we run onQuitApplication after the flush completes (is that true?). If so, then we'll save the state at that point.

::: browser/components/sessionstore/SessionStore.jsm:1374
(Diff revision 1)
> +    AsyncShutdown.quitApplicationGranted.addBlocker(
> +      "SessionStore: flushing all windows", this._flushAllWindowsAsync());

So my understanding is that this will spin a nested event loop until the flush completes. Will the XUL windows remain on-screen during that time? If they do, that seems concerning since the user could continue to interact with them.

Mike, can you try a test where you load a page that spins in JS and then quit the browser? I'm curious what happens.

Also, I assume there's some sort of timeout here. If the blocker isn't resolved after some number of seconds, do we proceed with shutdown or do we just kill ourselves or something?

::: browser/components/sessionstore/SessionStore.jsm:1382
(Diff revision 1)
> +  _flushAllWindowsAsync: Task.async(function*() {

No need for an underscore. This is already SessionStoreInternal.
Attachment #8687429 - Flags: review?(wmccloskey)
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

https://reviewboard.mozilla.org/r/25205/#review22833

Could you also update nsIAsyncShutdown.idl and nsAsyncShutdown.js?
Attachment #8687428 - Flags: review?(dteller)
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

https://reviewboard.mozilla.org/r/25207/#review22837

::: browser/components/sessionstore/SessionStore.jsm:627
(Diff revision 1)
> -        this.onQuitApplicationRequested();
> +        this.onQuitApplicationGranted();

Be sure to ask confirmation from ttaubert on this.

::: browser/components/sessionstore/SessionStore.jsm:1370
(Diff revision 1)
> +      this.saveStateDelayed();

I am a bit rusty with this code, but calling `this.saveStateDelayed` for all windows seems a little scary.

::: browser/components/sessionstore/SessionStore.jsm:1375
(Diff revision 1)
> +      "SessionStore: flushing all windows", this._flushAllWindowsAsync());

It would be great if you could provide a `getState()` argument to aid with debugging crashes.
Blocks: 1185674
https://reviewboard.mozilla.org/r/25207/#review22801

> What's this saveStateDelayed line for? I'm assuming that we run onQuitApplication after the flush completes (is that true?). If so, then we'll save the state at that point.

Yes, good point. Removing.
https://reviewboard.mozilla.org/r/25207/#review22801

> So my understanding is that this will spin a nested event loop until the flush completes. Will the XUL windows remain on-screen during that time? If they do, that seems concerning since the user could continue to interact with them.
> 
> Mike, can you try a test where you load a page that spins in JS and then quit the browser? I'm curious what happens.
> 
> Also, I assume there's some sort of timeout here. If the blocker isn't resolved after some number of seconds, do we proceed with shutdown or do we just kill ourselves or something?

Hrm, yeah - we do keep the windows on screen it seems. :/ That's not great. I suppose we could close the windows while we're waiting?

Yes, AsyncShutdown will crash the browser after a timeout of 1 minute: https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/toolkit/components/asyncshutdown/AsyncShutdown.jsm#76
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/1-2/
Bug 1177310 - TabStateFlusher Promises should always resolve. r?billm

They'll always resolve, but might receive a negative success state
and a message. We're doing this so that we can maintain a Set of
in-flight flushes that we can call Promise.all on (which will
fast-fail if any Promise rejects, or will just never resolve if
one or more of the Promises never resolve).
Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r?billm
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/1-2/
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Hey Tim,

Do you foresee any problems with us waiting until quit-application-granted to add an AsyncShutdown blocker and flush each window? We close each window right after requesting the flush, so the user shouldn't be able to change the state in the pocket of time between the start and end of the flush. Also note that because the quit-application-granted blocker is added dynamically in onQuitApplicationGranted, by the time that flushAllWindowsAsync starts running, RunState.isRunning should be false, so we won't end up running the onClose window flush for each closing window.

How does that sound?
Attachment #8687429 - Flags: feedback?(ttaubert)
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/2-3/
Attachment #8687428 - Flags: review?(dteller)
Attachment #8692082 - Flags: review?(wmccloskey)
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/1-2/
Comment on attachment 8692083 [details]
MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26227/diff/1-2/
Attachment #8692083 - Flags: review?(wmccloskey)
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/2-3/
Attachment #8687429 - Attachment description: MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r?Yoric → MozReview Request: Bug 1177310 - [WIP] Don't flush windows synchronously on application shutdown. r?Yoric,feedback=ttaubert
Attachment #8687429 - Flags: feedback?(ttaubert)
Attachment #8687429 - Flags: feedback?(ttaubert)
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

https://reviewboard.mozilla.org/r/26225/#review23729

Moar documentation? Guaranteed r+ :)
Attachment #8692082 - Flags: review+
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

https://reviewboard.mozilla.org/r/25205/#review23765

Looks good to me, thanks.
Attachment #8687428 - Flags: review?(dteller) → review+
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> Do you foresee any problems with us waiting until quit-application-granted
> to add an AsyncShutdown blocker and flush each window? We close each window
> right after requesting the flush, so the user shouldn't be able to change
> the state in the pocket of time between the start and end of the flush.

This seems okay to me, although it's really hard to say with all the things going on on shutdown. Letting SessionStore close all windows is _probably_ fine, although I would feel way more comfortable with a few shutdown/restart tests but here we are.

Should we try to get a QA person to check for anything this might break? A day or a few hours of restarting and quitting Firefox might reveal bigger problems. Or we would try to find some time to convert some of the tests from bug 923606, with the suite that Henrik suggested.

> Also
> note that because the quit-application-granted blocker is added dynamically
> in onQuitApplicationGranted, by the time that flushAllWindowsAsync starts
> running, RunState.isRunning should be false, so we won't end up running the
> onClose window flush for each closing window.

Is that really true? It seems that if onQuitApplicationGranted() runs before RunState.jsm is notified we'll have at least the first window end up in closed windows because the task runs immediately, no? Anyway, I think this would be easy to fix by introducing/calling RunState.setQuitting() before adding/creating the blocker.
Attachment #8687429 - Flags: feedback?(ttaubert) → feedback+
(In reply to Tim Taubert [:ttaubert] from comment #25)
> Is that really true? It seems that if onQuitApplicationGranted() runs before
> RunState.jsm is notified we'll have at least the first window end up in
> closed windows because the task runs immediately, no?

Upon re-examination, I'm less sure of this now. I had assumed that RunState would always be loaded before SessionStore.init (meaning that its quit-application-granted observer would always fire before SessionStore's), but it looks like RunState can (and usually does) run _after_ SessionStore.init.

So good catch.

> Anyway, I think this
> would be easy to fix by introducing/calling RunState.setQuitting() before
> adding/creating the blocker.

Sounds good.
Working on restart tests in bug 1228120.
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/3-4/
Attachment #8687428 - Attachment description: MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r?Yoric → MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/2-3/
Attachment #8692082 - Attachment description: MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r?billm → MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric
Attachment #8692082 - Flags: review?(wmccloskey)
Comment on attachment 8692083 [details]
MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26227/diff/2-3/
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/3-4/
Attachment #8687429 - Attachment description: MozReview Request: Bug 1177310 - [WIP] Don't flush windows synchronously on application shutdown. r?Yoric,feedback=ttaubert → MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r?Yoric,feedback=ttaubert
Attachment #8687429 - Flags: feedback+ → review?(dteller)
https://reviewboard.mozilla.org/r/25207/#review24083

::: browser/components/sessionstore/SessionStore.jsm:1462
(Diff revision 4)
> +      // Close the window immediately so that the user can't start changing
> +      // its state.
> +      win.close();
> +      // And then wait for the window flush to finish
> +      yield flushWindow;

This doesn't seem quite right to me. If we have a bunch of windows open, and the first one we close takes a long time to flush, then the others will remain open. Maybe that's okay because we haven't started collecting data for those other windows yet. But it's still a little strange. Could we wait for them all at once after they're all closed?
Comment on attachment 8692083 [details]
MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r=billm

https://reviewboard.mozilla.org/r/26227/#review24081

::: browser/components/sessionstore/TabStateFlusher.jsm:91
(Diff revision 3)
> +  // All in-flight flush Promises, for both browsers and windows

Need a period.

::: browser/components/sessionstore/TabStateFlusher.jsm:115
(Diff revision 3)
> +      this._activePromises.delete(flushPromise);

Why do this in a sepatate step?
Attachment #8692083 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/26227/#review24081

> Why do this in a sepatate step?

The function passed to the Promise constructor is called immediately - I only want to remove the Promise from the \_activePromises set when the Promise resolves. I chose doing this over attempting to remove the Promise in either resolve or resolveAll since in those two methods, I don't have easy access to the Promise, just the resolve method.
(In reply to Bill McCloskey (:billm) from comment #32)
> https://reviewboard.mozilla.org/r/25207/#review24083
> 
> ::: browser/components/sessionstore/SessionStore.jsm:1462
> (Diff revision 4)
> > +      // Close the window immediately so that the user can't start changing
> > +      // its state.
> > +      win.close();
> > +      // And then wait for the window flush to finish
> > +      yield flushWindow;
> 
> This doesn't seem quite right to me. If we have a bunch of windows open, and
> the first one we close takes a long time to flush, then the others will
> remain open. Maybe that's okay because we haven't started collecting data
> for those other windows yet. But it's still a little strange. Could we wait
> for them all at once after they're all closed?

Yeah, let's do that. I think that's definitely a valid concern.
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/4-5/
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/3-4/
Comment on attachment 8692083 [details]
MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26227/diff/3-4/
Attachment #8692083 - Attachment description: MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r?billm → MozReview Request: Bug 1177310 - Expose a Promise on TabStateFlusher that resolves when all in-flight flushes are done. r=billm
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/4-5/
Attachment #8687429 - Attachment description: MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r?Yoric,feedback=ttaubert → MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r?billm,feedback=ttaubert
Attachment #8687429 - Flags: review?(dteller) → review?(wmccloskey)
Blocks: 1195295
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

https://reviewboard.mozilla.org/r/25207/#review24377

I can't think of anything wrong with this patch. Let's try it :-)!

::: browser/components/sessionstore/SessionStore.jsm:1477
(Diff revision 5)
> +    yield TabStateFlusher.pendingFlushesDone();

What is this for?

::: browser/components/sessionstore/SessionStore.jsm:1478
(Diff revision 5)
>      // we must cache this because _getMostRecentBrowserWindow will always

Newline here.
Attachment #8687429 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/25207/#review24377

> What is this for?

I honestly can't remember the reason I added this. I think I was originally worried about anybody attempting to flush some other window or a browser while we were waiting for a window to flush, but we'll eventually flush that other window and wait for the message to come back from it, so any preceding flush Promises would have already resolved.

I'll just remove this and the revision that added pendingFlushesDone. Thanks for spotting that.
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/5-6/
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/4-5/
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/5-6/
Attachment #8687429 - Attachment description: MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r?billm,feedback=ttaubert → MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert
Attachment #8692083 - Attachment is obsolete: true
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/6-7/
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/5-6/
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/6-7/
Comment on attachment 8687428 [details]
MozReview Request: Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25205/diff/7-8/
Comment on attachment 8692082 [details]
MozReview Request: Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26225/diff/6-7/
Comment on attachment 8687429 [details]
MozReview Request: Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25207/diff/7-8/
https://hg.mozilla.org/integration/fx-team/rev/478290d4a81d801a744fb4999cc49c398909d098
Bug 1177310 - Add quit-application-granted to AsyncShutdown parent process phases. r=Yoric

https://hg.mozilla.org/integration/fx-team/rev/c6626646ae1cf8c72953edf8fa57fdc69afa8e99
Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

https://hg.mozilla.org/integration/fx-team/rev/15ded62d213cb2e01feebcd21af52be74a535983
Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert
Hey :tracy, have you heard any murmurs about people having shutdown crashes or a spike in bad session restores since this landed?
Flags: needinfo?(twalker)
No. It doesn't look like (scanning over topcrash stats) there is any new shutdown hang/crash that first appeared since this landed. However, there are some pre-existing shutdown hang signatures (without bugs) that could be masking data.  I have also not heard anything about session store other than seeing some bug mail for bug 1217904 which you're already working on with alex.
Flags: needinfo?(twalker)
Depends on: 1235379
Depends on: 1255511
Depends on: 1260461
Depends on: 1284687
You need to log in before you can comment on or make changes to this bug.