922814 - nsIAppStartup.shuttingDown is reset to false during shutdown

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Steven MacLeod [:smacleod]]

Both nsAppExitEvent (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#111 ) and nsAppStartup::Quit (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#459 ) reset nsIAppStartup.shuttingDown to false when the application is definitely shutting down.

This causes problems for any code relying on |shuttingDown| which is still executing after the flag is reset, such as the final session store file write - see Bug 922785

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Looking at the source code of nsAppStartup.cpp, it is not quite clear why we set |mShuttingDown = false|. There is a comment "turn off the reentrancy check flag" that makes me wary of getting rid of all occurrences of |mShuttingDown = false|.

Ehsan, since you are the author of one of these occurrences, could you tell us whether you think we would be breaking something?

Comment 2

[(no longer active)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Ehsan, since you are the author of one of these occurrences, could you tell
> us whether you think we would be breaking something?

Did you look at the change I made there? :-)  I just replaced PR_FALSE with false across our code base, I don't know anything about this code.

Comment 3

[(no longer active)]

(Note that getting useful blames for the code often involves looking at the actual changes and blaming on the parent revisions if the changes do not seem to match what you're looking for.)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Oops :)
Hey, look what I found – that piece of code was written by a certain bsmedberg, a long time ago, in a galaxy far far away: https://github.com/jrmuizel/mozilla-cvs-history/commit/3055933c278e6a315788ee06fade12ac152a0159

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Once the application is shutting down, it remains shutting down until there is no more application

Let's see if we break something: https://tbpl.mozilla.org/?tree=Try&rev=2000dc9f298d

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 812842 [details] [diff] [review]
Once the application is shutting down, it remains shutting down until there is no more application

Review of attachment 812842 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/startup/nsAppStartup.cpp
@@ -456,5 @@
>  
> -  // turn off the reentrancy check flag, but not if we have
> -  // more asynchronous work to do still.
> -  if (!postedExitEvent)
> -    mShuttingDown = false;

After looking at this again, this case should probably be left in. We only reset |mShuttingDown| here when |!postedExitEvent|, so we aren't actually shutting down right now from this call to nsAppStartup::Quit.

I suspect the culprit for the unexpected reset is just that other case you removed.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

I'm not quite clear. It's the nsAppExitEvent that used to reset mShuttingDown to false. The snippet you mention performs the reset even if for some reason we haven't posted that event.

Comment 8

[Benjamin Smedberg]

Comment on attachment 812842 [details] [diff] [review]
Once the application is shutting down, it remains shutting down until there is no more application

smacleod is correct. The hunk at the bottom of nsAppStartup::Quit needs to stay in so that if we are using eConsiderQuit or eAttemptQuit and we decided *not* to quit that the flag reverts to its normal false state.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Once the application is shutting down, it remains shutting down until there is no more application, v2

In that case, here we go.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/57594d25cc7e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57594d25cc7e