449308 - Quitting doesn't work when onbeforeunload handler poses dialog

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

I ran across this when composing gmail messages.

STEPS TO REPRODUCE:
1) Load the URL in the URL field
2) Try to quit the browser (menu item, Cmd-Q).
3) When the alert comes up, click "OK".

ACTUAL RESULTS:  All windows close, but the browser does not quit

EXPECTED RESULTS: Browser quits

Note: In the gmail case it's a prompt(), but the result is the same.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and this is almost certainly Mac-only.

Comment 2

[Dave Miller [:justdave]]

This happens in Thunderbird/Shredder on Mac if you have an unsent/unsaved compose window open, also.  When you tell it to quit, it brings the compose window to the front, prompts if you want to save it first. I say Yes.  It saves and closes the compose window, but the main window stays open and the app doesn't quit.

Comment 3

[Dave Miller [:justdave]]

maybe related to bug 271112? (which seems to have been re-introduced by bug 386002)

Comment 5

[Honza Bambas (:mayhemer)]

--> me

Comment 6

[Honza Bambas (:mayhemer)]

On exit nsAppStartup::Quit returns early on line 226 because if (!hiddenWindow || usefulHiddenWindow) is both true, no nsAppExitEvent is posted and FF hangs. When I jump in debugger over the condition FF exits as expected.

Comment 7

[Dave Townsend [:mossop]]

I wonder if the patch in bug 400479 fixes this.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> I wonder if the patch in bug 400479 fixes this.

It doesn't. The patch applies with offsets on mozilla-central and there is no longer any xpfe/components/startup/src/nsAppStartup.cpp, what just indicates the patch is quit old. I rebuilt the whole tree to be sure.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1

FF doesn't close not becuase of the hidden window but because of inconsistancy of the mConsiderQuitStopper counter - it is too high when we want to quit and there were a dialog open/closed during quiting. 

During quit operation the counter is drop inside of AttemptingQuit(TRUE) called from nsAppStartup::Quit. When a new window (whatever XUL window) is open during call of CloseAllWindows(), AttemptingQuit(FALSE) is called and reverts the counter (increases). But actually, the counter should remain drop because this new window was immediatelly closed yet inside of CloseAllWindows() call and must not break the quit process. Therefor I check the number of actively open windows is not higher then before AttemptingQuit(TRUE) was called and AttemptingQuit(FALSE) was called somewhere from CloseAllWindows(). If so, I call AttemptingQuit(TRUE) again to compensate.

When a window remains open (and AttemptingQuit(FALSE) was called) we must drop back mAttemptingQuit flag when such a window is found in the cycle bellow.

Comment 10

[Honza Bambas (:mayhemer)]

And, I tested bug 400479 comment 0 scenario. On MAC it works for me either with or without my patch :-/

Comment 11

[Benjamin Smedberg]

Comment on attachment 342159 [details] [diff] [review]
v1

I think this is ok, but I'd like a second opinion because of our unhappy lack of unit-tests for this fragile code.

Comment 12

[Dave Townsend [:mossop]]

I think that there are problems here. Can you explain how this doesn't break if I have multiple windows open and during the quit one of them opens a new non-modal window? We should halt the shutdown, but I think your patch will force it to continue because mConsiderQuitStopper is now much lower than previousQuitStopper unless I'm missing something.

I also seem to recall that we don't cancel mAttemptingQuit if the initial close of windows fails because modal windows take time to wrap up their event loops and fully close, then we re-enter Quit with eConsiderQuit as the comment above there says.

Comment 13

[Honza Bambas (:mayhemer)]

You are right. This scenario would break the patch. We could count windows open and left open during mAttempingQuit (or better a new flag, not sure of a name) and call again AttemptingQuit(PR_TRUE) when the count is zero.

Agree?

Comment 14

[Dave Townsend [:mossop]]

Does the modal window open and close all happen essentially synchronously during the call to window->Close in CloseAllWindows? If so I wonder if you can just compare mConsiderQuitStopper before and after that single call. If it has gone down by one then we're still quitting. Just trying to think if there are any hazards there, the shutdown process is so convoluted :s

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #14)
> Does the modal window open and close all happen essentially synchronously
> during the call to window->Close in CloseAllWindows? 

Yes. I count on this in the patch. Dialogs spin their own event queue and doesn't leave it before exit.

> If so I wonder if you can
> just compare mConsiderQuitStopper before and after that single call. If it has
> gone down by one then we're still quitting. Just trying to think if there are
> any hazards there, the shutdown process is so convoluted :s

Not sure I follow. 

What I suggest is to have a different flag (mAttemptingQuitInProgress e.g.) and a different counter (mQuitStoppersDuringAttempQuit) initially zeroed before CloseAllWindows call. Enter and ExitLastWindowClosingSurvivalArea() would, when mAttemptingQuitInProgress set, increase/decrease mQuitStoppersDuringAttempQuit. When return from CloseAllWindows() we will change the condition mConsiderQuitStopper <= previousQuitStopper to mQuitStoppersDuringAttempQuit == 0. Makes sense?

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 342159 [details] [diff] [review]
v1

r- based on he above comments.

(In reply to comment #15)
> What I suggest is to have a different flag (mAttemptingQuitInProgress e.g.) and
> a different counter (mQuitStoppersDuringAttempQuit) initially zeroed before
> CloseAllWindows call. Enter and ExitLastWindowClosingSurvivalArea() would, when
> mAttemptingQuitInProgress set, increase/decrease mQuitStoppersDuringAttempQuit.
> When return from CloseAllWindows() we will change the condition
> mConsiderQuitStopper <= previousQuitStopper to mQuitStoppersDuringAttempQuit ==
> 0. Makes sense?

If you mean have a counter for the number of windows opened and closed during the CloseAllWindows method, then wouldn't that generally be negative? Even if one of the windows happened to open and close a modal window during the close.

Comment 17

[Honza Bambas (:mayhemer)]

Yeah, forget to say we must remeber the window in a hash set. If it is NOT found there during deregistration we must NOT drop the secondary counter. Still relativelly simple to implement.

Comment 18

[Dave Townsend [:mossop]]

So my thought, in CloseAllWindows was something like:

if (window) {
  windowCount = mConsiderQuitStopper;
  window->Close()
  if (windowCount < mConsiderQuitStopper) {
    // reset attempting quit.
  }
}

Another option is just to store the window we are currently trying to close in a member variable, then when a new window open is detected see if its parent is the currently closing window and if the new window is modal. If so then we can not reset attempting quit, we know the quit process is paused waiting for the modal dialog to close.

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to comment #18)
> So my thought, in CloseAllWindows was something like:
> 
> if (window) {
>   windowCount = mConsiderQuitStopper;
>   window->Close()
>   if (windowCount < mConsiderQuitStopper) {
>     // reset attempting quit.
>   }
> }
> 

This won't work, but I like this smart aproach. When there are e.g. 5 windows open we have 5+0 quit stoppers (5 windows and 0 hidden windows), window opens a dialog -> we get 6+1 (hidden window is returned), dialog closes -> 5+1, window itself closes -> 4+1, we are back at 5, but do not reset, according to your condition.

> Another option is just to store the window we are currently trying to close in
> a member variable, then when a new window open is detected see if its parent is
> the currently closing window and if the new window is modal. If so then we can
> not reset attempting quit, we know the quit process is paused waiting for the
> modal dialog to close.

Probably better idea. I had more generic approach, but this seems to be enough for this case. If you agree I will do this approach.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> (In reply to comment #18)
> > So my thought, in CloseAllWindows was something like:
> > 
> > if (window) {
> >   windowCount = mConsiderQuitStopper;
> >   window->Close()
> >   if (windowCount < mConsiderQuitStopper) {
> >     // reset attempting quit.
> >   }
> > }
> > 

Actually that comparison should be the other way round for it to work.

> This won't work, but I like this smart aproach. When there are e.g. 5 windows
> open we have 5+0 quit stoppers (5 windows and 0 hidden windows), window opens a
> dialog -> we get 6+1 (hidden window is returned), dialog closes -> 5+1, window
> itself closes -> 4+1, we are back at 5, but do not reset, according to your
> condition.

Yeah the hidden window makes everything funky unfortunately.

> > Another option is just to store the window we are currently trying to close in
> > a member variable, then when a new window open is detected see if its parent is
> > the currently closing window and if the new window is modal. If so then we can
> > not reset attempting quit, we know the quit process is paused waiting for the
> > modal dialog to close.
> 
> Probably better idea. I had more generic approach, but this seems to be enough
> for this case. If you agree I will do this approach.

I think it does a good job of limiting the impact of this patch, maybe see what bsmedberg thinks, but you'll need to get a re-review off him anyway.

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v2

I rereviewed your first proposal and realized we can detect "reopen" of the hidden window thanks the mAttemptingQuit flag. I didn't find a scenario when this patch would not work as expected.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 345518 [details] [diff] [review]
v2

I think this looks ok. I think I can still imagine some weird scenarios where we can break it, but I think they are getting so convoluted that we shouldn't be trying to solve them.

>diff --git a/toolkit/components/startup/src/nsAppStartup.cpp b/toolkit/components/startup/src/nsAppStartup.cpp
>+        PRInt32 currentQuitStoppers = RealQuitStoppers();
>+        // If the current number of windows is smaller or same then the number
>+        // recorded before window close, we must re-attempt quit. 
>+        // 'Or same' condition is here because the actual window deregisters
>+        // later asynchronously.

Does it always de-register later? If so then would it work for the comparison to just be ==?

>+        if (quitStoppers >= currentQuitStoppers)
>+          AttemptingQuit(PR_TRUE);

Can you just swap these around, it reads more sensibly to me the other way for some reason.

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to comment #22)
/components/startup/src/nsAppStartup.cpp
> >+        PRInt32 currentQuitStoppers = RealQuitStoppers();
> >+        // If the current number of windows is smaller or same then the number
> >+        // recorded before window close, we must re-attempt quit. 
> >+        // 'Or same' condition is here because the actual window deregisters
> >+        // later asynchronously.
> 
> Does it always de-register later? If so then would it work for the comparison
> to just be ==?
> 

It would, but nsGlobalWindow::WindowClose() is, for browser windows, always invoked after we process Quit(). WindowClose() fires nsCloseEvent that asynchronously deregisters the window. Dialogs deregister synchronously because they don't leave their loops until nsCloseEvent is processed, this event also calls ExitModalLoop.

Comment 24

[Honza Bambas (:mayhemer)]

Hm... I misread your question. Actually, if the window closed normally the number of quit stoppers should not change (because it will be decreased later). I tried, but in one limited case with dialogs during close we do not quit with just ==. So I'll leave it.

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 345703 [details] [diff] [review]
v2.1

(Not sure who else should do sr for this).

Comment 27

[Honza Bambas (:mayhemer)]

Please commit comment 25, patch v2.1.

Comment 28

[Dave Townsend [:mossop]]

dcamp landed this: http://hg.mozilla.org/mozilla-central/rev/7cc91b5a33b4

Comment 30

[Aakash Desai [:aakashd]]

verified FIXED on builds: 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre ID:20090414035212