459376 - quit-application-requested observer notification not generated when last window is closed

Status: NEW

(Toolkit :: Startup and Profile System, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

On linux, when you close the last open window, no "quit-application-requested" observer notification is generated.  Per bsmedberg on 435058#c2, it would appear this is not intentional.

Code path wise, this occurs as a result of a "xul-window-destroyed" observer notification triggering nsAppStartup::ExitLastWindowClosingSurvivalArea which in turn calls nsAppStartup::Quit with eConsiderQuit.  eConsiderQuit only double-checks that there are known existing windows before promoting the mode to eAttemptQuit (which results in a "quit-application-granted" notification.)

My naive suggestion to resolve this problem would be to cause eConsiderQuit to perform the "quit-application-requested" event with abort logic.  I call it naive because I am not sure what sequences happen on other platforms; 403837#c0 would suggest that OS X already handles the last window closing properly (but not windows).

On linux, this would not interfere with quitting via the file menu; that happens via a call to goQuitApplication in mozilla/toolkit/content/globalOverlay.js which dispatches to canQuitApplication in the same file.  canQuitApplication issues the quit-application-requested notification and checks the result.  If nothing wants to bail, it then calls nsIAppStartup.quit(nsIAppStartup.eAttemptQuit).

I am happy to provide a patch for this problem with direction from toolkit people.

Comment 1

[Dave Townsend [:mossop]]

The danger here is that someone may cancel the attempt to quit without opening a window. At which point on windows and linux you would be left with a stuck background process you couldn't interact with I think.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #1)
> The danger here is that someone may cancel the attempt to quit without opening
> a window. At which point on windows and linux you would be left with a stuck
> background process you couldn't interact with I think.

nsAppShutdown has the insight required to make sure that a window appears as a result of the termination process, and to continue it if one does not.  The consumer of interest from my perspective is MailNews' nsMsgShutdown which in fact pops up a dialog when it decides to interrupt the shutdown process.

It could also enforce a concept of "you can only stop shutdown once" so that when that magic dialog that appeared actually closes we do a shutdown without checking.

Comment 3

[Dave Townsend [:mossop]]

So you are saying we should ignore the attempt to cancel the shutdown process if no new window is created? Doesn't the shutdown process get halted anyway if quit-application-granted opens a new window?

My thought would be that we should be sending quit-application-requested as the user clicks close on the last window, but before it is closed, then cancel the window close if the request is denied. I kind of thought we were doing that already but apparently not.

Comment 4

[Benjamin Smedberg]

I don't think it's reasonable to require every chrome window to have an onclose handler that does extra work... I tend to agree with the original premise; and... there are good reasons you might want to keep an application running even if no windows are open: e.g. a taskbar icon for a long-running process.

Comment 5

[Dave Townsend [:mossop]]

I agree we shouldn't require every window to have an onclose handler. But maybe if there were some global onclose handler that did the work when it was the last window?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #4)
> I don't think it's reasonable to require every chrome window to have an onclose
> handler that does extra work... I tend to agree with the original premise;
> and... there are good reasons you might want to keep an application running
> even if no windows are open: e.g. a taskbar icon for a long-running process.

Does this mean you think having eConsiderQuit perform the "quit-application-requested" logic is okay?

In principle (and per the nsIAppStartup interface documentation), whatever mechanism is responsible for the taskbar icon could call nsIAppStartup's enterLastWindowClosingSurvivalArea and exitLastWindowClosingSurvivalArea methods, or cause them to be called.  This would allow for the invariant that if the user can't interact with the app, we should be quitting.  It looks like there is no system tray code in the core, although bug 413517 and bug 325353 have attempts; they don't seem to do anything about this quit mechanism.  In any event, the current logic will quit the app when we hit zero, so I don't see how it would be a regression to continue to enforce the invariant.  (Namely, if someone aborts the quit process and the 'window count' hits zero, we should still quit.)

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #6)
> if the user can't interact with the app, we should be quitting.  It looks like
> there is no system tray code in the core, although bug 413517 and bug 325353

Er, bug 413517 is marked fixed; I thought it didn't land because no check-in comment exists on the bug and a file I looked for didn't exist on trunk with the name in the patch.  I think my argument still stands though that the logic proposed would not change the current behaviour, no matter what the tray icon code is doing.

Comment 8

[Mark Banner (:standard8)]

Unless someone can tell me of another way of holding off quit/shutdown without blocking the main thread, I think we need this bug fixing to be able to fix at least one TB 3 blocker.

My issue is that if we're in the middle of sending mail, we need to do something sensible rather than just abort (= potential dataloss). The best method I can see is temporarily cancel the quit-application-requested and re-invoke it when we're done (which is what our mailnews shutdown service does). However, that needs this bug fixing, or some other acceptable solution so that we can hold off nicely in all instances.

Comment 9

[Mark Banner (:standard8)]

Requesting blocking to notify core drivers that Thunderbird wants to get a fix in for this in 1.9.1 to allow sane shutting down and not loosing sending of your email part way through. If we end up putting the full fix in on trunk, and doing an ifdef MOZ_THUNDERBIRD on 1.9.1 that may be acceptable if its too late to get a core fix in.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Wanted, but not blocking as per comment 9

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

beltzner: my reading of comment 9 is that _something_ must be changed in the toolkit code itself for 1.9.1, even if that something is inside of a Thunderbird-specific ifdef.  Am I misunderstanding the reasoning here, or does that mean this bug wants to block?

Comment 12

[Benjamin Smedberg]

There are certainly workarounds that the mailnews code could use, such as spinning an event loop during shutdown waiting for mail to send... and unless somebody's going to work on it and come up with a detailed plan to avoid regressions, I don't see how we'd accept anything but very targeted changes to this code: it's incredibly regression-prone and hasn't got a useful test infrastructure.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> There are certainly workarounds that the mailnews code could use, such as
> spinning an event loop during shutdown waiting for mail to send...

Have you got any examples of this? It may be a better way, but a starting point would help greatly.

> and unless
> somebody's going to work on it and come up with a detailed plan to avoid
> regressions, I don't see how we'd accept anything but very targeted changes to
> this code: it's incredibly regression-prone and hasn't got a useful test
> infrastructure.

That's not the impression I got from the rest of this bug. Are you saying this just because of where we are in the 1.9.1 cycle? (and btw I was going to look at a patch on Monday).

Comment 14

[David :Bienvenu]

(In reply to comment #13)
> (In reply to comment #12)
> > There are certainly workarounds that the mailnews code could use, such as
> > spinning an event loop during shutdown waiting for mail to send...
> 
> Have you got any examples of this? It may be a better way, but a starting point
> would help greatly.

We already do this - see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1016

I don't think bsmedberg is saying that's a better way, just a work-around. We would very much like to move away from doing that, not add more instances of it.

Comment 15

[Benjamin Smedberg]

Yeah, I'm only suggesting a workaround because of the risk of trying to change things this late on the branch.

Another alternative is to actually open a hidden window for background tasks, or to add a considerquitstopper for them. The problem with both of these options is that the user will close the last window and then not know that anything is still running.

Comment 16

[Nick Kreeger]

> Another alternative is to actually open a hidden window for background tasks,
> or to add a considerquitstopper for them. The problem with both of these
> options is that the user will close the last window and then not know that
> anything is still running.

The shutdown service already handles user interaction when it process the shutdown tasks (synchronously). The real problem is that "quit-application-requested" is _never_ posted when the user closes the last window via the native window chrome (i.e. 'X' button on Windows). This behavior is much different when the user exits Thunderbird via File->Quit.

A potential stop-gap/hack would be to listen for "quit-application-granted" and try to invoke the hidden window (it might be too late at that point) or use PR_CWait() hack the  in the shutdown service.

Comment 17

[David :Bienvenu]

We do open a hidden window for the shutdown processes, and show that window if the tasks take more than a small amount of time.

Perhaps all we need is a notification - it could be a new notification, which would have very little chance of causing regressions.

Comment 18

[Mark Banner (:standard8)]

We've worked around this for TB 3. So reflecting reality. Would be nice to get something sane in for gecko 1.9.2 though.

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to comment #18)
> We've worked around this for TB 3. So reflecting reality. Would be nice to get
> something sane in for gecko 1.9.2 though.

1.9.2 is already is in, or shortly will be in, lockdown, no?

Comment 20

[Wayne Mery (:wsmwk)]

this is needed for Thunderbird send-in-background Bug 511079