496434 - Mailnews shutdown service doesn't respect restarting flags.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Nick Kreeger]

When an application receives an update or when a user installs a an extension, the "quit-application-requested" observer topic is notified. This is the notice that the shutdown service uses to halt shutdown and process it's pending tasks. However, when that topic is observed an additional parameter is passed in |aData| specifying that the operation is going to restart (aData is a literal string "restart"). The shutdown service currently does not respect this flag and just shuts the app down.

We caught this when using a spin-off of the shutdown service inside of Songbird during update testing. This is a very simple fix and I have a patch.

Comment 1

[Nick Kreeger]

Attachment: Patch V1

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 381613 [details] [diff] [review]
Patch V1

>-    NS_ENSURE_TRUE(appStartup, );
>-    NS_ENSURE_SUCCESS(appStartup->Quit(nsIAppStartup::eAttemptQuit), );
>+    NS_ENSURE_TRUE(appStartup, /* void */);
>+    NS_ENSURE_SUCCESS(appStartup->Quit(mQuitMode), /* void */);

I'd prefer not to add the comments here - the function is void and that can clearly be seen.

>+
>+      // If the attempted quit was a restart, be sure to restart the app once
>+      // the tasks have been run. This is usually the case when addons or 
>+      // updates are going to be installed.
>+      if (aData == NS_LITERAL_STRING("restart").get()) 
>+      {
>+        mQuitMode |= nsIAppStartup::eRestart;
>+      }

This if statement doesn't work - you are comparing to PRUnichar pointers. Use something like nsDependentString(...).EqualsLiteral(...) to make it work.

nit: there is also unnecessary whitespace on the end of the blank lines and one of the comment lines.

Comment 3

[Nick Kreeger]

Attachment: Patch V2

Here is an updated patch, all issues should be addressed. (sorry for the poor quality patch first go-round)

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 382577 [details] [diff] [review]
Patch V2

That works nicely, thanks.

r=Standard8

Comment 5

[David :Bienvenu]

Comment on attachment 382577 [details] [diff] [review]
Patch V2

sr=me, if you remove the unneeded braces here:

+      {
+        mQuitMode |= nsIAppStartup::eRestart;
+      }

Comment 6

[David :Bienvenu]

Comment on attachment 382577 [details] [diff] [review]
Patch V2

actually marking sr+

Comment 7

[Nick Kreeger]

Pushed to comm-central.

changeset:   2886:72f343ec0f86
user:        Nick Kreeger <[e-mail]>
date:        Thu Jun 18 23:55:20 2009 -0700
summary:     Fixing bug 496434 -  Mailnews shutdown service doesn't respect restarting flags.

changeset:   2889:56d5e9e1c5d7
tag:         tip
parent:      2886:72f343ec0f86
parent:      2888:64aaeb609c9a
user:        Nick Kreeger <[e-mail]>
date:        Fri Jun 19 07:40:44 2009 -0700
summary:     Merging for checkin of bug 496434.