163710 - Must File | Quit twice to actually quit the app (mac os' only)

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Tracy Walker [:tracy]]

seen on commercial branch builds:
mac osx 2002-08-20-05-1.0
mac os9 2002-08-20-05-1.0

-Launch app
-Click File | Quit
the current window is closed, but the app is still running
-Click File | Quit again to actually quit the app.

Comment 1

[sairuh (rarely reading bugmail)]

recent regression --this was fine with yesterday's branch build.

tracy, was this a problem with the trunk builds on mac?

Comment 2

[sairuh (rarely reading bugmail)]

danm?

btw, is anyone having problems with this on mozilla? if it's commercial, i'll
move it over to bugscape...

Comment 3

[sairuh (rarely reading bugmail)]

might've been caused by the fix for bug 130719.

side note: cmd-Q behaves the same way here --need to hit it twice to actually
quit the app.

Comment 4

[Gayatri Rimola]

Was this caused by bug 163718?

Comment 5

[Tracy Walker [:tracy]]

this is on the trunk too. seen on mac os9 2002-08-20-03-trunk

Comment 6

[Kristoff Batizy]

I am experiencing this bug with trunk build ID: 2002082109 Mozilla on OS X 6C115
(10.2).

Comment 7

[Tracy Walker [:tracy]]

this is not happening on the branch builds as of today. but it is still 
happening on the trunk

Comment 8

[Dan M]

Attachment: give quit-on-last-window-closed semantics to the Mac after selecting File->Quit

Ugh. Should have seen this coming. In the post bug 130719 world, Mozilla must
be prepared to quit once the last window has closed. Mac apps don't work that
way. This patch makes MacMoz work that way, though only during the Quit
process. This unload handler thing is like an onion.

Comment 9

[Mike Pinkerton (not reading bugmail)]

+  mQuitOnLastWindowClosing(PR_TRUE),
+  mQuitWhenLastWindowCloses(PR_FALSE)

i don't understand the difference between these two flags.

Comment 10

[Dan M]

So what are you saying? More comments than these ?

+  PRPackedBool mQuitOnLastWindowClosing; // normally true, affects win/*unix
+  PRPackedBool mQuitWhenLastWindowCloses; // normally false, affects macintosh

The deal is, the first is normally true; you try to quit when the last window
closes. But it works out to only affect win/*unix builds. The second is normally
false; you don't try to quit when the last window closes. And it works out that
it only affects Mac builds. They're pretty much identical except they pull in
opposite directions and at different stages in the lifetime of the app.
  I thought I was being clever, giving them similar names. It might help if I
gave the new variable some completely different name. ??

Comment 11

[Simon Fraser [no longer active]]

Why aren't they #ifdeffed?

Am I the only one that think that taking changes like these at this stage in the
game is dangerous?

Comment 12

[Dan M]

Attachment: give quit-on-last-window-closed semantics to the Mac after selecting File->Quit

(Mike:) how about this version. Check out the new variable names and comments.
(Simon:) adding an #ifdef to the extant code would be a more invasive change.
Doable, though. But this patch isn't scary, and it fixes behaviour already
broken by another patch. You could well ask why did the other patch go in, but
that was also fixing broken behaviour and I thought this was a decent time for
such stuff.

Comment 13

[Dan M]

More grief with an #ifdef: the PC/Unix variable is effectively part of the
nsIAppShellService.idl interface.

Comment 14

[Brendan Eich [:brendan]]

I think I've persuaded danm to look at all those PRPackedBools and try to unify,
simplifying names to things like mAlreadyStarted rather than
mQuitUnixAppWhenLastWindowCloses (possibly
unifying mInProfileStartup with that if the latter is always the complement of
the former) and mForceShutdown rather than mQuitMacAppWhenLastWindowCloses. 
Looking for a new patch....

/be

Comment 15

[Jean-Pierre Melkonian]

Same for me. Build 2002082109.
Launching (With mail openend at launch) and quitting give the following
JavaScript Errors:

Error: popup has no properties
Source File: chrome://messenger/content/mailNavigatorOverlay.xul
Line: 110

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

Error: document has no properties
Source File:
chrome://global/content/consoleBindings.xml#console-box.createConsoleRow()
Line: 1

Error: gBrowser has no properties
Source File: chrome://navigator/content/navigator.js
Line: 749

I have always seen the first error, but the others are new

This worked correctly with Build 2002081908

Comment 16

[Brendan Eich [:brendan]]

*** Bug 164125 has been marked as a duplicate of this bug. ***

Comment 17

[Jo Hermans]

*** Bug 164520 has been marked as a duplicate of this bug. ***

Comment 18

[Niklas Dougherty]

Quitting works as intended on MOX Mozilla 1.1 release, fortunately.

Comment 19

[Dan M]

Attachment: rework Quit logic once again

Alright. Fewer booleans, a new integer, and no member variables have colliding
names. Quit logic is generally reworked and overall simplified. As far as I can
tell this is the Cadillac of Quit code, meaning that it's quiet and it seems to
run flawlessly today. Any takers?

Comment 20

[timeless]

<+   (Does useful work only on the Mac). */
>+   (Only does useful work on Mac OS*). */
other than that, it looks ok. but it's late and you don't really want my review
anyway.

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 97075 [details] [diff] [review]
rework Quit logic once again

r=pink assuming it's been tested on all 3 platforms ;)

does any of this code impact embedding or -turbo?

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 97075 [details] [diff] [review]
rework Quit logic once again

>Index: xpfe/appshell/public/nsIAppShellService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/appshell/public/nsIAppShellService.idl,v
>retrieving revision 1.26
>diff -u -r1.26 nsIAppShellService.idl
>--- xpfe/appshell/public/nsIAppShellService.idl	24 Aug 2002 18:57:43 -0000	1.26
>+++ xpfe/appshell/public/nsIAppShellService.idl	28 Aug 2002 20:49:14 -0000
>@@ -191,10 +191,14 @@
>   void hideSplashScreen();
>   
>   /**
>-   * We may need to show a XUL dialog before there are any other windows.
>-   * In this case we don't nescesarily want to quit when it is closed.
>-   * Use with extreme caution.
>+   * During application startup (and at other times!) we may temporarily
>+   * encounter a situation where all application windows will be closed
>+   * but we don't want to take this as a signal to quit the app. Bracket
>+   * the code where the last window could close with these.
>+   * (And at application startup, on platforms which don't normally quit
>+   * when the last window has closed, call Enter... once, but not Exit...)

Nit, sorry: "which don't" should be "that don't", and those ellipses seem
mannered to me -- why not just a period at the end?

>    */
>-  attribute boolean quitOnLastWindowClosing;
>-
>+  void EnterLastWindowClosingSurvivalArea();
>+  void ExitLastWindowClosingSurvivalArea();

Use interCaps, not InterCaps, in IDL, please.  The C++ methods in the generated
.h file will be capitalized.

>+/* We know we're trying to quit the app, but may not be able to do so
>+   immediately. Enter a state where we're more ready to quit.
>+   (Does useful work only on the Mac). */

Timeless's nit-pick was wrong, it misplaced "only" -- danm has it right, but
can the period go inside the parens, please?

>+    EnterLastWindowClosingSurvivalArea();
>+
>     // If we being launched in turbo mode, profile mgr cannot show UI

Could you fix this pre-existing typo and put an "are" after the "we"?  Thanks.

>@@ -514,6 +525,9 @@
>      this code is part of the eForceQuit case, so I'm checking against
>      that value anyway. Reviewers made me add this comment. */

Argh, I didn't make you add this -- I suggested an assertion that aFerocity !=
eForceQuit here, just before the "can't happen" test for the inverse of that
condition.

>   if (aFerocity == eAttemptQuit || aFerocity == eForceQuit) {
>+
>+    AttemptingQuit(PR_TRUE);

Gratuitous extra line before the AttemptingQuit call?  Maybe it looks better in
context (but maybe that's cuz the comment before the if is weighing too heavily
[visually] upon the if)?

> NS_IMETHODIMP
>-nsAppShellService::GetQuitOnLastWindowClosing(PRBool *aQuitOnLastWindowClosing)
>+nsAppShellService::EnterLastWindowClosingSurvivalArea(void)
> {
>-    NS_ENSURE_ARG_POINTER(aQuitOnLastWindowClosing);
>-    *aQuitOnLastWindowClosing = mQuitOnLastWindowClosing;
>-    return NS_OK;
>+  ++mConsiderQuitStatus;
>+  return NS_OK;
> }
> 
> NS_IMETHODIMP
>-nsAppShellService::SetQuitOnLastWindowClosing(PRBool aQuitOnLastWindowClosing)
>+nsAppShellService::ExitLastWindowClosingSurvivalArea(void)
> {
>-    mQuitOnLastWindowClosing = aQuitOnLastWindowClosing;
>-    return NS_OK;
>+  NS_ASSERTION(mConsiderQuitStatus > 0, "consider quit status out of bounds");
>+  --mConsiderQuitStatus;
>+  return NS_OK;
> }

As nsAppShellService uses NS_IMPL_THREADSAFE_{ADDREF,RELEASE} for some reason,
should these methods us PR_AtomicIncrement and PR_AtomicDecrement (and a
PRInt32 for mConsiderQuitStatus)?  Or is all true thread-safety here unneeded,
and a bad idea?  If so, who tried wallpapering over assert-botches by using
NS_IMPL_THREADSAFE_...?

>+  PRUint16 mConsiderQuitStatus; // Quit(eConsiderQuit) fails if this is > 0

Maybe mDontConsiderQuit or (my fave) mConsiderQuitStopper or something less
vague and misleading than Status?

Sorry for all the nits, but this code is clearly tricky, and I think names,
comments and assertions can help make it less so.  Fix what you agree is worth
fixing in the list above, and sr=[e-mail].

/be

Comment 23

[Dan M]

Mike, comment 21:
It's all Mozilla-app-specific, so it shouldn't affect embedding. It totally
affects -turbo. My test matrix:
1-launch, quit.
2-launch, close window, (quit - Mac-only)
3-launch, open window, quit.
4-launch, open window, close new window, quit.
5-launch, load Page With Unload Handler That Opens New Window, close window.
6-launch, load PWUHTONW, quit.
7-launch, open new window, load PWUHTONW, quit.

repeat on Linux,Mac,Win and then again Win -turbo. All good, unless I skipped
one in practice which seems likely.

Brendan, comment 22:
>As nsAppShellService uses NS_IMPL_THREADSAFE_{ADDREF,RELEASE} for some reason,
>should these methods us PR_AtomicIncrement and PR_AtomicDecrement (and a

I see through your attempt to get me to use 32 bit integers. As for whether this
counter should be threadsafe, I don't think it's a problem and if it is it's not
the only one in this file but, why not... (note the ellipsis)

>Maybe mDontConsiderQuit or (my fave) mConsiderQuitStopper or something less
>vague and misleading than Status?

Fifteen minutes with a thesaurus gave me no inspiration, enlightenment,
illumination. So I went with your favourite.

>Gratuitous extra line before the AttemptingQuit call?  Maybe it looks better in
>context (but maybe that's cuz the comment before the if is weighing too heavily
>[visually] upon the if)?

Exactly! It's nice to have company with my aesthetics.

Patch is in.

Comment 24

[John Morrison]

*** Bug 165613 has been marked as a duplicate of this bug. ***

Comment 25

[Dan M]

(from comment 23)
d'oh!
8-launch, load PWUHTONW, open new window, quit (subtly different from case 7)
doesn't work. The app just quits. Man, this *is* pointlessly subtle code. I'm
calling this a problem with that other bug 130719. Making a note there now.

Comment 26

[Greg K.]

*** Bug 165612 has been marked as a duplicate of this bug. ***

Comment 27

[Jo Hermans]

*** Bug 165869 has been marked as a duplicate of this bug. ***

Comment 28

[Tracy Walker [:tracy]]

verified with trunk builds:

mac os9 2002-09-03-09-trunk
mac osx 2002-09-03-09-trunk

Comment 29

[Jean-Pierre Melkonian]

WFM with same build

Comment 30

[Greg K.]

*** Bug 168001 has been marked as a duplicate of this bug. ***