386002 - Move tryToClose calls on shutdown

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Michael Wu [:mwu]]

A observer watching "quit-application-requested" can be used to make tryToClose calls automatically instead of having all quit code manually iterate through windows and call tryToClose.

Comment 1

[Michael Wu [:mwu]]

Attachment: Move tryToClose calls, kill nsICloseAllWindows

Still needs more testing but it should work out..

Comment 2

[Simon Bünzli]

Even better would be for tryToClose to go away completely (see bug 338040 for an outdated patch). Currently you can lose data when you've already closed a few or all windows through tryToClose before either one window objects or before an extension which registered later than your new nsTryToClose component objects - i.e. if you actually close windows before quit-application-granted is broadcast.

Anyway, you'd have to make sure that you don't start tryToClose'ing windows before SessionStore gets the quit-application-requested notification (and note: it registers very late!) or otherwise it'll miss some or even all windows for later restoration.

Comment 3

[Simon Bünzli]

Re-read your patch and it makes more sense now: you just probe tryToClose without actually closing the windows. You can safely ignore the previous comment except for the pointer to bug 338040.

Comment 4

[Benjamin Smedberg]

Comment on attachment 269964 [details] [diff] [review]
Move tryToClose calls, kill nsICloseAllWindows

>+++ b/toolkit/components/startup/src/nsAppStartup.cpp

>@@ -449,27 +451,35 @@ nsAppStartup::Observe(nsISupports *aSubj
>                       const char *aTopic, const PRUnichar *aData)
> {
>   NS_ASSERTION(mAppShell, "appshell service notified before appshell built");
>   if (!strcmp(aTopic, "profile-change-teardown")) {
>     nsresult rv;
>     EnterLastWindowClosingSurvivalArea();
>     // NOTE: No early error exits because we need to execute the
>     // balancing ExitLastWindowClosingSurvivalArea().
>-    nsCOMPtr<nsICloseAllWindows> closer =
>-            do_CreateInstance("@mozilla.org/appshell/closeallwindows;1", &rv);
>-    NS_ASSERTION(closer, "Failed to create nsICloseAllWindows impl.");
>-    PRBool proceedWithSwitch = PR_FALSE;
>-    if (closer)
>-      rv = closer->CloseAll(PR_TRUE, &proceedWithSwitch);
>-
>-    if (NS_FAILED(rv) || !proceedWithSwitch) {
>+    nsCOMPtr<nsIObserverService> obsServ =
>+      do_GetService("@mozilla.org/observer-service;1");
>+    NS_ASSERTION(obsServ, "Failed to get observer service");
>+
>+    nsCOMPtr<nsISupportsPRBool> cancelSwitch =
>+      do_CreateInstance(NS_SUPPORTS_PRBOOL_CONTRACTID);
>+    PRBool abortSwitch = PR_FALSE;
>+    if (obsServ && cancelSwitch) {
>+      rv = obsServ->NotifyObservers(cancelSwitch, "quit-application-requested", nsnull);
>+      cancelSwitch->GetData(&abortSwitch);
>+    }
>+
>+    if (NS_FAILED(rv) || abortSwitch) {
>       nsCOMPtr<nsIProfileChangeStatus> changeStatus(do_QueryInterface(aSubject));
>       if (changeStatus)
>         changeStatus->VetoChange();
>+    } else if (obsServ) {
>+      obsServ->NotifyObservers(nsnull, "quit-application-granted", nsnull);
>+      CloseAllWindows();
>     }

In the toolkit, profile-shutdown is not a cancelable event. Therefore profile-change-teardown ought to simply fire quit-application-granted without quit-application-requested. Don't be confused by the legacy nsIProfileChangeStatus API: if you call VetoChange you will assert, see http://mxr.mozilla.org/mozilla/source/toolkit/xre/nsXREDirProvider.cpp#725

With that change, r=me

Comment 5

[Michael Wu [:mwu]]

Attachment: Move tryToClose calls, kill nsICloseAllWindows, v2

I dropped all the quit-application-requested and quit-application-granted notifications. Otherwise, we get two quit-application-granted notifications on shutdown. This update also removes the tryToClose code from the test suite's quit code, which was copied from toolkit's quit code.

Comment 6

[Michael Wu [:mwu]]

Attachment: Move tryToClose calls, kill nsICloseAllWindows, v3

Remove all remaining references to nsCloseAllWindows.js in browser/installer/*/packages-static.

Comment 7

[Michael Wu [:mwu]]

Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.116; previous revision: 1.115
done
Checking in browser/installer/windows/packages-static;
/cvsroot/mozilla/browser/installer/windows/packages-static,v  <--  packages-static
new revision: 1.128; previous revision: 1.127
done
Checking in layout/tools/reftest/quit.js;
/cvsroot/mozilla/layout/tools/reftest/quit.js,v  <--  quit.js
new revision: 1.2; previous revision: 1.1
done
Checking in testing/mochitest/tests/SimpleTest/quit.js;
/cvsroot/mozilla/testing/mochitest/tests/SimpleTest/quit.js,v  <--  quit.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/startup/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/startup/public/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/startup/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/startup/src/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/startup/src/nsAppStartup.cpp;
/cvsroot/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,v  <--  nsAppStartup.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/startup/src/nsAppStartup.h;
/cvsroot/mozilla/toolkit/components/startup/src/nsAppStartup.h,v  <--  nsAppStartup.h
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/content/globalOverlay.js;
/cvsroot/mozilla/toolkit/content/globalOverlay.js,v  <--  globalOverlay.js
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.126; previous revision: 1.125
done
Checking in toolkit/mozapps/extensions/content/list.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/list.js,v  <--  list.js
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/mozapps/update/content/updates.js;
/cvsroot/mozilla/toolkit/mozapps/update/content/updates.js,v  <--  updates.js
new revision: 1.73; previous revision: 1.72
done
Checking in toolkit/xre/nsCommandLineServiceMac.cpp;
/cvsroot/mozilla/toolkit/xre/nsCommandLineServiceMac.cpp,v  <--  nsCommandLineServiceMac.cpp
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/toolkit/components/startup/src/nsTryToClose.js,v
done
Checking in toolkit/components/startup/src/nsTryToClose.js;
/cvsroot/mozilla/toolkit/components/startup/src/nsTryToClose.js,v  <--  nsTryToClose.js
initial revision: 1.1
done
Removing toolkit/components/startup/public/nsICloseAllWindows.idl;
/cvsroot/mozilla/toolkit/components/startup/public/nsICloseAllWindows.idl,v  <--  nsICloseAllWindows.idl
new revision: delete; previous revision: 1.1
done
Removing toolkit/components/startup/src/nsCloseAllWindows.js;
/cvsroot/mozilla/toolkit/components/startup/src/nsCloseAllWindows.js,v  <--  nsCloseAllWindows.js
new revision: delete; previous revision: 1.1
done

Comment 8

[Frank Wein [:mcsmurf]]

Shouldn't the file nsCloseAllWindows.js be added to browser/installer/removed-files.in?

Comment 9

[Michael Wu [:mwu]]

Attachment: Add nsCloseAllWindows.js to removed-files.in

(In reply to comment #8)
> Shouldn't the file nsCloseAllWindows.js be added to
> browser/installer/removed-files.in?
> 
Sure. This patch adds it for all the removed-files.in I can find.

Comment 10

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

Looks like the following was missed
http://lxr.mozilla.org/seamonkey/source/mail/installer/windows/packages-static#327

Comment 11

[Michael Wu [:mwu]]

Attachment: packages-static change for mail, calendar

(In reply to comment #10)
> Looks like the following was missed
> http://lxr.mozilla.org/seamonkey/source/mail/installer/windows/packages-static#327
> 
Thanks for catching this. This patch fixes mail and calendar.

Comment 12

[Michael Wu [:mwu]]

Checking in calendar/installer/windows/packages-static;
/cvsroot/mozilla/calendar/installer/windows/packages-static,v  <--  packages-static
new revision: 1.39; previous revision: 1.38
done
Checking in mail/installer/windows/packages-static;
/cvsroot/mozilla/mail/installer/windows/packages-static,v  <--  packages-static
new revision: 1.65; previous revision: 1.64
done
Checking in browser/installer/removed-files.in;
/cvsroot/mozilla/browser/installer/removed-files.in,v  <--  removed-files.in
new revision: 1.28; previous revision: 1.27
done
Checking in calendar/installer/removed-files.in;
/cvsroot/mozilla/calendar/installer/removed-files.in,v  <--  removed-files.in
new revision: 1.12; previous revision: 1.11
done
Checking in mail/installer/removed-files.in;
/cvsroot/mozilla/mail/installer/removed-files.in,v  <--  removed-files.in
new revision: 1.35; previous revision: 1.34
done

Comment 13

[Mike Cowperthwaite]

This may have caused a regression; see bug 271112 comment 6, 10.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This also likely caused bug 449308.