If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Mailnews shutdown service doesn't respect restarting flags.



MailNews Core
8 years ago
8 years ago


(Reporter: Nick Kreeger, Assigned: Nick Kreeger)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

3.00 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review


8 years ago
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

8 years ago
Created attachment 381613 [details] [diff] [review]
Patch V1
Assignee: nobody → nick.kreeger
Attachment #381613 - Flags: review?(bugzilla)
Attachment #381613 - Flags: review?(bugzilla) → review-
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

8 years ago
Created attachment 382577 [details] [diff] [review]
Patch V2

Here is an updated patch, all issues should be addressed. (sorry for the poor quality patch first go-round)
Attachment #381613 - Attachment is obsolete: true
Attachment #382577 - Flags: review?(bugzilla)
Attachment #382577 - Flags: review?(bugzilla) → review+
Comment on attachment 382577 [details] [diff] [review]
Patch V2

That works nicely, thanks.


Comment 5

8 years ago
Comment on attachment 382577 [details] [diff] [review]
Patch V2

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

+      {
+        mQuitMode |= nsIAppStartup::eRestart;
+      }
Attachment #382577 - Flags: superreview?(bienvenu)

Comment 6

8 years ago
Comment on attachment 382577 [details] [diff] [review]
Patch V2

actually marking sr+
Attachment #382577 - Flags: superreview?(bienvenu) → superreview+

Comment 7

8 years ago
Pushed to comm-central.

changeset:   2886:72f343ec0f86
user:        Nick Kreeger <nick.kreeger@park.edu>
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 <nick.kreeger@park.edu>
date:        Fri Jun 19 07:40:44 2009 -0700
summary:     Merging for checkin of bug 496434.
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.