8673 - ShutDown() should try to close all open windows.

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Simon Fraser [no longer active]]

Copied from <news:[e-mail]>

> Due to some great work by Simon Fraser, Simon was able to fix some
> memory problems throughout the app that were causing our messenger and
> composer objects to not get destroyed when you exited the application.
> Obviously this had a trickle down effect on a lot of things in our code.
>
> After running with his changes, if I bring up mailnews and then Close
> the app through the menu item, messenger is properly released. However,
> if I choose Exit instead, we leak.
>
> It appears that Close and Exit have slightly different implementations.
> Close gets a root webshell, gets a web window and then closes the web
> window.
>
> Exit gets the appshell and shuts it down.
>
> I wonder if shutting down the appshell is supposed to have a trickle
> down effect where all the windows are then closed. If that's true, then
> the close code is never getting called. Or should our implementation of
> Exit call Close first and then tell the appshell to shut down?

You should post this on xpfe so the xptoolkit folks see it (makes
sense, no?).

Behaviour has always been different for Exit() and Close(), and
I don't think Exit() is doing the right thing, currently. It should
try to close each open window in turn, and, if a window cannot
be closed (say it's a modified composer document, and the user
cancelled the Save prompt) then the exit sequence should be
terminated, and the application stay running.

---------------

Looking in nsAppShellService::Shutdown(), I see some #ifdef'ed out code
that tries to close all open windows. It does not handle the case of windows
which refuse to be closed (e.g. user cancels the Save dialog), and it should.

We also need to fix this so that windows get destroyed in the same way,
whether you hit the close box in the UI, or choose Exit from the menu.

Comment 1

[Bill Law]

Accepting this, I'll look to do something for M8.  Ultimately we will need some
mechanism to block shutdown if a window so desires.  Once
nsIAppShellService::Shutdown is called, I fear it is too late to go back.

Comment 2

[don]

Move to M9 ...

Comment 3

[Bill Law]

Now to M10...

Comment 4

[davidm]

I may have writen this code for globalOverlay.js quit function. It is commented
out right now since there are some RDF problems in the task menu. If you want
feel free to reassign this bug to me.

Comment 5

[Bill Law]

I'll gladly hand this one off to you, David.

My thinking is that we will have to invent a more robust "window close"
mechanism.  My thinking is that we'd turn nsIBrowserWindow::Close into an "ok to
close?" method.  It would have 2 possible results: OK or Cancel.
Implementations of nsIBrowserWindow ought then implement that method by
delegating to some application-specific logic.  In our case, nsIBrowserWindow
ought to call some per-Window JS code (perhaps specified by an onclose handler
on the XUL <window> element).  Implementors of that code would either do
window.close() and return OK, or put up a Save/Don't Save/Cancel dialog and
return OK (after saving or not) or Cancel (if the user says to do that).

nsIAppShellService::Shutdown would then go to each top-level window, get an
nsIBrowserWindow*, and call Close().  This would stop when/if some window said
cancel.  BTW, this code needs to be wired into the platform-specific system
shutdown event(s) (I know Windows has one).

An issue might be that this semantic for nsIBrowserWindow::Close doesn't fit
with it's current semantic (which might be simply a close notification).  If
that's the case, then we probably want to invent a new OkToClose() method and
let Close() continue to work the way it does.

This solution requires some input/cooperation with the toolkit team (to get OK
for onclose handler) and the webshell/browser-window guru(s) since this impacts
any potential webshell embedders.

Anyway, that was my thinking.

Comment 6

[davidm]

Here is the plan of record. Each window can have a JS routine called
tryToClose() which will return true if the window was closed and false if the
window couldn't be. The quit routine ( in JS) will walk the window list from
most recently used to last used and either call tryToClose or close if
tryToClose isn't implemented. It will stop when tryToClose returns false. If it
walks all the way through the window list it will then call AppShellService
shutdown.

Comment 7

[davidm]

fix checked in

Comment 8

[Paul MacQuiddy]

marking verified for now. I wonder if this will break with new webshell stuff.