400479 - Cannot quit or restart application while modal windows are present

Status: RESOLVED FIXED

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

Description

[RenegadeX]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101905 Minefield/3.0a9pre

Here's a strange one..

Tools -> Options -> Manage Add-ons -> Restart Minefield

.. DOES NOT close and restart the app, however it DOES close & restart with no problem if I do: Tools-> Add-ons -> Restart Minefield

Reproducible: Always

Actual Results:  
Using the Tools->Options->Manage Add-ons route, the only thing that happens when I click "Restart Minefield' is that the Add-ons window closes. The Options window and the main browser window remain open. Checking Task Manager, there is no significant CPU usage during this - in fact, except for a momentary spike, it returns to '0'. In testing, I've repeatedly sat and waited as much as 5 minutes for something to happen, but nothing ever does.

If I close the Options window and then manually close the browser (using the 'X' in the top right), the browser window disappears (as expected) but the process remains active and moments later, the browser re-opens. So obviously the restart command registered, it's just having an issue shutting down.

A system reboot did not make any difference.


I am using the zip build of 3.0a9pre, WinXP-Pro.

If someone can pinpoint the date/build when this was first added to the Options dialog, I'll go back and see if I can get the that to work.

Comment 1

[Robert Strong (they/them - no direct email)]

Possibly due to browser.preferences.instantApply being set to false

Comment 2

[Dave Townsend [:mossop]]

Just realised that this button allows us to open a non-modal window from a modal window on windows if I remember right? Isn't that a UI no-no?

Comment 3

[Robert Strong (they/them - no direct email)]

iirc it depends on whether the window being opened is itself setting prefs. For example, the bookmarks manager window is not modal just as the Add-ons manager window is not modal.

Comment 4

[RenegadeX]

(In reply to comment #1)
> Possibly due to browser.preferences.instantApply being set to false

I checked about:config and yes, it is set to false (which seems to be the default as it's not bolded).

Btw, this was observed on a brand new profile.

Comment 5

[RenegadeX]

Found this appearing in the Error Console after failed close on restart initialization:
----
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppStartup.quit]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: restartApp :: line 1715"  data: no]
----

Comment 6

[RenegadeX]

I previously said that if I did:
Tools->Add-ons->Restart Minefield
.. then that worked. And it does. However, playing around a little, I discovered that if I open it via that route, then do 'Tools->Options', then swap back to the Add-ons pane and hit 'Restart', the original problematic behaviour is replicated. That is, the Add-ons pane closes, but the Options pane stays open.

I really don't understand the code all that well, but looking through http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.js
I cannot find anything in the restart or shutdown code sections that query the Options panel's status, or call for it to close. It does clearly check the Add-ons pane and the Update pane (as the 'Restart Minefield' message may be present in both) and call for them to be closed if open. So I believe it's basically just missing an Options pane check, and trigger. How to do that, I have no clue....

Comment 7

[Dave Townsend [:mossop]]

Something quick to try, what happens to you do tools - options - manage addons, then close the options window, then try to restart?

I suspect the problem is that the modal window opened from the browser window is stopping the canQuit handler in the browser window from being run when we try to shutdown. This would not be an extension manager bug but well something else.

Comment 8

[RenegadeX]

(In reply to comment #7)
> Something quick to try, what happens to you do tools - options - manage addons,
> then close the options window, then try to restart?

You can't actually do that as the when the Add-ons pane is opened via the Options window, it 'steals and locks the focus' on that Add-ons pane. Clicking anywhere outside of it within the app (be it browser window or the open Options window) results in the Windows XP Ding.wav sound (Default Beep) and the border and titlebar of the Add-ons pane briefly flashing--alerting the user that the open Add-ons pane must be dealt with before continuing.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

If you do the above steps and open afterwards some windows in that order: organize bookmarks, add-ons manager, and options the restart button only closes all windows except the main window.

If you close Firefox/Minefield afterwards it will be restarted and comes up again.  So the restart flag was still saved.

Comment 11

[Marc N.]

There are two cases with modal dialogs I just tested (XP-home sp2) on FX 3.0b1:

1) If the addon window is opened and the multiple tab closing message box is brought forth, clicking restart button closes all windows except the window which showed the alert, and the Error log has the same 0x80004005 failure code as previously reported.
If there were multiple browser windows, the non-prompting ones were closed as well. If several windows were showing the tab closing confirmation prompt, they stay opened. However, windows that were blocked by a modal Javascript alert() are normally closed.

2) If the modal dialog is the native Save As dialog, and closing occurs from the addons window, the functionality behaves as expected.

Comment 12

[RenegadeX]

I found another similar case where Restart doesn't happen due to unclosed items:

Steps to Reproduce:
1. 'Bookmarks' -> 'Show All Bookmarks' -> 'Import And Backup' -> 'Import'
   .. an 'Import Wizard' dialog opens
2. click back in the browser window and do 'Tools' -> 'Addons' -> 'Restart Minefield'

Actual results:
- Everything closes, *except* for 'Library' (aka 'Places Organizer')

Should I file that separately, or should this Bug be renamed to become more general?

Just for kicks, I tried to get Firefox 2.0.0.11 to choke in a similar manner ('Import' open)- and by installing an extension so that 'Restart Firefox' is made available, however it performed flawlessly. So it would seem this is a regression. 

I came across Bug 303512 and Bug 312744 while looking for previously filed bugs on the issue, are they at all relevant?

Comment 13

[Dave Townsend [:mossop]]

This is a result of bug 334891.

When anything tries to shutdown the app the app tries to close all open windows. It then checks to see if any windows are still open and if so aborts the shutdown. Bug 334891 made it so calling close on a window with a modal dialog open from it has no effect. The order that we end up going through windows means we hit the parent window and fail to close it before the child dialog, so the shutdown fails.

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #13)

One way to solve that would be to make the loop that goes through the open windows and tries to close them restart as long as the number of open windows changed when going through the list of windows. That way first time through the list, the modal dialog would be closed but the opener might stay open, second time through the opener of the modal dialog would close, and so on.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> (In reply to comment #13)
> 
> One way to solve that would be to make the loop that goes through the open
> windows and tries to close them restart as long as the number of open windows
> changed when going through the list of windows. That way first time through the
> list, the modal dialog would be closed but the opener might stay open, second
> time through the opener of the modal dialog would close, and so on.

This won't work, neither will my even more cunning plan of enumerating the windows front to back to always hit the modal windows first.

Turns out that the quit is being run from an event triggered from here:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/appshell/src/nsXULWindow.cpp&rev=1.178&mark=398#394

Only after the quit method has completed can any close call on the modal window actually have an effect and that method end and it is only after that method has ended that the modal state on the parent window is cleared.

Comment 16

[Johnny Stenback (:jst)]

Maybe each iteration through the list of windows we want to close needs to happen off of its own event then, so that modal dialogs get a chance to close in between? I really don't know if that would work, but might be worth a try...

Comment 17

[Dave Townsend [:mossop]]

I think closing by z order should work but yes you'd have to spin a new event for each window close. Or I guess multiple iterations might involve less events depending on the ratio of modal/non-modal windows.

But in order to do that the Quit method would have to become one that returned immediately and you'd get no error from it in the event it couldn't close all windows.

The only other alternative I can think of is to shift the modal handling around so EnterModalState were called from within nsXULWindow::ShowModal and ExitModalState called from nsXULWindow::ExitModalLoop. Looking over bug 334981 though it seems that that will cause issues for embeddors?

Comment 18

[Dave Townsend [:mossop]]

I think that switching Quit to be an immediate return is probably the way to go. Looking at all the callers there seem to be none that actually do anything sensible with a thrown exception. Most of the ones that even try to catch it are in our testing tools and they just log the exception and move on as if nothing had happened.

Comment 19

[Dave Townsend [:mossop]]

Umm wait a minute, Quit already is an operation that returns before it is done. The whole modal dialog issue was there before but hidden because every window accepted the close call then later after Quit finished the windows close and the last one re-enters Quit with eConsiderQuit which caused the shutdown.

Now however the parents of modal windows ignore the Close call so the windows no longer shut down so we never re-enter Quit.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is a patch that works well, I'm just a little sure about a timeout issue that I'll mention shortly.

Basically it rips out the window closing and checking from nsAppStartup::Quit. Once it has determined that a shutdown can happen it creates an nsWindowCloseEvent and pushes it onto the thread event queue then finishes.

The event firstly checks that the quit attempt hasn't been cancelled then it runs through all of the open windows attempting to close them. Only windows without modal children will actually accept the close call.

After the run through we check how many windows were open. If there were none then the observer notification is sent out and the final exit event is set up to run to handle the real shutdown.

The event keeps a count of how many windows were open on the previous run through. If the number has changed then some windows managed to close ok, might have unblocked their parents and so more windows might have accepted the close call this time, so the event is scheduled to run again. Otherwise no new windows closed for some reason so it just gives up the shutdown attempt.

After the event finishes but before it starts again any modal windows will wrap up their event loops, finish up and unblock their parents. Here is the kicker, when the child unblocks the parent, a new event is scheduled to run any pending timeouts on the parent. This event ends up running after the next nsWindowCloseEvent, which closes the parent. This causes an assertion when the pending timeout event runs. To avoid this I have put a check in that pending timeouts shouldn't run if the window has already been closed.

Comment 21

[Benjamin Smedberg]

Comment on attachment 305736 [details] [diff] [review]
patch rev 1

I'm almost positive this doesn't handle the eForceQuit behavior correctly. When processing with eForceQuit, I think we should either do the recursive thing and fail when no more windows close, or force-quit immediately the first time. In any case, we need code for that.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 305736 [details] [diff] [review]
patch rev 1

Yep. I have a patch that fixes that, unfortunately there is some strange race condition going on with NS_ProcessNextEvent processing multiple events which breaks this approach randomly :(

Comment 23

[Dave Townsend [:mossop]]

Attachment: alternate approach

This is an alternate approach which I think is a lot saner, avoiding all kind of threading issues.

Basically this makes windows aware that a shutdown is in progress. When a window with modal children is told that it is no longer modally blocked and a shutdown is in progress it closes itself.

So the process is pretty much as it used to be. nsAppStartup tries to close all windows. A number of them remain open, wrapping up their event loops after the fact but now the modal ones trigger their parents to close. The last window to close triggers a call to nsAppStartup::Quit again with eConsiderQuit to complete the shutdown.

Comment 24

[Dave Townsend [:mossop]]

Attachment: alternate approach

Oops, that was an old version

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 307703 [details] [diff] [review]
alternate approach

sr=jst assuming bsmedberg buys off on this too.

Comment 27

[Simon Bünzli]

Comment on attachment 307703 [details] [diff] [review]
alternate approach

>+      obsService->NotifyObservers(nsnull, "quit-application-cancelled", nsnull);

Does this notification really mean that a previous "quit-application-granted" should be ignored and things will continue running as if nothing had happened? If so, wouldn't this break the "clean-up (and disable) at shutdown" pattern seen in several XPCOM components (such as SessionStore)?

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #27)
> (From update of attachment 307703 [details] [diff] [review])
> >+      obsService->NotifyObservers(nsnull, "quit-application-cancelled", nsnull);
> 
> Does this notification really mean that a previous "quit-application-granted"
> should be ignored and things will continue running as if nothing had happened?
> If so, wouldn't this break the "clean-up (and disable) at shutdown" pattern
> seen in several XPCOM components (such as SessionStore)?

There is no real change in behaviour. As things stand right now it is possible for the app to fail to shutdown after the quit-application-granted notification has been dispatched. The only difference is that with this patch there is a notification when it happens where there isn't currently.

Comment 29

[Simon Bünzli]

(In reply to comment #28)
> As things stand right now it is possible for the app to fail to shutdown
> after the quit-application-granted notification has been dispatched.

In what state will the app then be? Potentially several windows closed already or everything still untouched? If the former, wouldn't something like "quit-application-interrupted" be more explicit (telling components that they'll first have to check what's been left before deciding on how to continue)?

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> (In reply to comment #28)
> > As things stand right now it is possible for the app to fail to shutdown
> > after the quit-application-granted notification has been dispatched.
> 
> In what state will the app then be? Potentially several windows closed already
> or everything still untouched? If the former, wouldn't something like
> "quit-application-interrupted" be more explicit (telling components that
> they'll first have to check what's been left before deciding on how to
> continue)?

Some windows potentially closed. Basically this happens when a new window is opened during the shutdown process, as part of an existing window's unload handler for example.

Comment 31

[Mike Schroepfer]

Taking off the blocker list

Comment 32

[Benjamin Smedberg]

Comment on attachment 307703 [details] [diff] [review]
alternate approach

Per the granparadiso meeting, don't want to take this without tests.

Comment 34

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This bug affects Thunderbird trunk builds, both debug and nightly (3rd April) builds, as well.

On a self-compiled trunk debug build version 3.0a1pre (2008040301) on a fresh
profile in 10.5, Thunderbird fails to quit when modal windows are present.

Steps to reproduce:

1: View settings for any account (Local Folders will do as well)
2: Leaving the settings window open, now go open "Preferences".
3: Attempt to quit application.

The application doesn't quit when it should. Only the settings and Preferences windows get closed.

Relevant debug output:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppStartup.quit]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goQuitApplication :: line 51"  data: no]

This shows up as the quit command is pressed using the mouse or the keyboard shortcut cmd-q.

Comment 38

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #32)
> (From update of attachment 307703 [details] [diff] [review])
> Per the granparadiso meeting, don't want to take this without tests.

Dave, can you please give an update to the status of your work?

Comment 40

[Dave Townsend [:mossop]]

(In reply to comment #39)
> (In reply to comment #32)
> > (From update of attachment 307703 [details] [diff] [review] [details])
> > Per the granparadiso meeting, don't want to take this without tests.
> 
> Dave, can you please give an update to the status of your work?

It is no longer assigned to me since I am no longer working on this. It needs someone to figure out how to test this.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #40)
> It is no longer assigned to me since I am no longer working on this. It needs
> someone to figure out how to test this.

Oh sure. I've overseen that. Sorry. Tony or Clint, could this be covered by a Mozmill test or which framework would be the best one?

Comment 42

[Tony Chung [:tchung]]

yeah, i believe mozmill can handle focal dialog test cases.   Just need to find a volunteer to write it.

Comment 47

[Dave Townsend [:mossop]]

Fixed by bug 537449