Closed Bug 386002 Opened 13 years ago Closed 13 years ago

Move tryToClose calls on shutdown

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mwu, Assigned: mwu)

References

()

Details

Attachments

(3 files, 2 obsolete files)

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.
Still needs more testing but it should work out..
Assignee: nobody → michael.wu
Status: NEW → ASSIGNED
Attachment #269964 - Flags: review?(benjamin)
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.
Blocks: 338040
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 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
Attachment #269964 - Flags: review?(benjamin) → review+
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.
Attachment #269964 - Attachment is obsolete: true
Attachment #270639 - Flags: review?(benjamin)
Blocks: 363741
Blocks: 386810
Remove all remaining references to nsCloseAllWindows.js in browser/installer/*/packages-static.
Attachment #270639 - Attachment is obsolete: true
Attachment #271535 - Flags: review?(benjamin)
Attachment #270639 - Flags: review?(benjamin)
Attachment #271535 - Flags: review?(benjamin) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Shouldn't the file nsCloseAllWindows.js be added to browser/installer/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.
Attachment #272666 - Flags: review?(benjamin)
(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.
Attachment #272666 - Flags: review?(benjamin) → review+
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
Depends on: 271112
This may have caused a regression; see bug 271112 comment 6, 10.
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
This also likely caused bug 449308.
Depends on: 539997
You need to log in before you can comment on or make changes to this bug.