298292 - nsIAppStartup Quit restart flag does not always work

Status: RESOLVED INCOMPLETE

(Toolkit :: Startup and Profile System, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

The Quit function call with the eRestart flag does not always work since
mShuttingDown is set multiple times during the shutdown process, which causes
mRestart on nsAppStartup to be reset when other callers call Quit without the
eRestart flag after the initial caller. 

A simple fix is to make mRestart settable only once and never again. (See
attachment)

A fix for pondering is making sure mShutdown is only ever set once the appshell
really has exited, and not multiple times.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix.

Comment 2

[Darin Fisher]

bsmedberg: could you please comment on this patch.  the other solution is to
only set mShuttingDown to FALSE once nsIAppShell.run has returned.  that seems
like a better fix to me, but i'm worried about regressing something.  what do
you think?

Comment 3

[Darin Fisher]

Attachment: v2 patch

Here's the alternative (more risky) patch.  I haven't tested this yet, but I
think it should work ;-)

Comment 4

[Benjamin Smedberg]

Comment on attachment 187058 [details] [diff] [review]
v2 patch

r=me if it works ;-)

Comment 5

[Darin Fisher]

Comment on attachment 187058 [details] [diff] [review]
v2 patch

So, this patch does not work.  The problem is that in the eAttemptQuit case, we
clear mShuttingDown at the bottom of nsAppStartup::Quit.  So, the next call to
Quit (which seems to be generated by XUL windows closing), ends up resetting
the value of mRestart.

I don't know if it would be best to go with the original patch or to invent a
better way to flag the application for restart.  It seems odd to me that
subsequent calls to Quit would not trump earlier calls, so the first patch
still feels hackish to me.

Thoughts?

Comment 6

[Darin Fisher]

For reference, the first patch went in accidentally, but I think that's ok for
now.  I want to come up with a better solution shortly.

I think it might make sense to just define a new attribute on nsIAppStartup to
schedule a restart, thus separating that setting from the manner in which the
application is actually shutdown.

Comment 7

[Benjamin Smedberg]

See also bug 310076, bug 310378.