882007 - Restart notification is not shown when staging is enabled

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Comment 1

[Robert Strong (they/them - no direct email)]

See bug 829221 comment #9

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev2

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev3

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: 1. main patch in progress

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: 2. remove now unused method (trunk only)

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: 3. tests in progress

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: 1. main patch rev1

getCanStageUpdates needs to check if service is installed as well but I want to do that and a couple of other things in a separate bug since the changes in this bug are getting rather large.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: 3. tests rev1

This includes a ton of cleanup (I've been meaning to get around to cleaning them up) along with several new tests and fixed tests. I plan on adding tests for background downloads with staging but I want to get the review process started to get this in before the merge.

Comment 10

[Robert Strong (they/them - no direct email)]

I sent the patches to try
https://tbpl.mozilla.org/?tree=Try&rev=13f6d907a94c

and here is an earlier try run with essentially the same patches
https://tbpl.mozilla.org/?tree=Try&rev=583bf4aab6df

So, I think this will be good to go.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 764551 [details] [diff] [review]
1. main patch rev1

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>...
>@@ -3122,26 +3137,44 @@ UpdateManager.prototype = {
>     // Send an observer notification which the update wizard uses in
>     // order to update its UI.
>     LOG("UpdateManager:refreshUpdateStatus - Notifying observers that " +
>         "the update was staged. state: " + update.state + ", status: " + status);
>     Services.obs.notifyObservers(null, "update-staged", update.state);
> 
>     // Do this after *everything* else, since it will likely cause the app
>     // to shut down.
>-    if (update.state == STATE_APPLIED) {
>+#ifdef MOZ_WIDGET_GONK
>+    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC) {
This should just be
if (update.state == STATE_APPLIED) {

Also, I don't want to change MOZ_WIDGET_GONK until I have time to investigate its behavior here and I'll file a followup for that.

>       // Notify the user that an update has been staged and is ready for
>       // installation (i.e. that they should restart the application). We do
>       // not notify on failed update attempts.
>       var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>                      createInstance(Ci.nsIUpdatePrompt);
>       prompter.showUpdateDownloaded(update, true);
>     } else {
>       releaseSDCardMountLock();
>     }
>+#else
>+    // Only prompt when the UI isn't already open.
>+    let windowType = getPref("getCharPref", PREF_APP_UPDATE_ALTWINDOWTYPE, null);
>+    if (Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) ||
>+        windowType && Services.wm.getMostRecentWindow(windowType)) {
>+      return;
>+    }
>+
>+    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC ||
>+        update.state == STATE_PENDING || update.state == STATE_PENDING_SVC) {
>+      // Notify the user that an update has been staged and is ready for
>+      // installation (i.e. that they should restart the application).
>+      var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>+                     createInstance(Ci.nsIUpdatePrompt);
>+      prompter.showUpdateDownloaded(update, true);
>+    }
>+#endif
>   },

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 764552 [details] [diff] [review]
3. tests rev1

Removed the following two lines from utils.js locally
  Services.prefs.setBoolPref("javascript.options.strict", true);
  Services.prefs.setBoolPref("browser.dom.window.dump.enabled", true);

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 764552 [details] [diff] [review]
3. tests rev1

Also removed this change
-var DEBUG_AUS_TEST = false;
+var DEBUG_AUS_TEST = true;

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: 5. more tests rev1

It should be possible to remove and / or consolidate these new tests with test_0092, test_0096, and test_0097 but I'd like to think about it more before doing so.

Comment 15

[Robert Strong (they/them - no direct email)]

Another try run with the additional tests included
https://tbpl.mozilla.org/?tree=Try&rev=dbe48cc763db

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #14)
> Created attachment 764579 [details] [diff] [review]
> 4. more tests rev1
> 
> It should be possible to remove and / or consolidate these new tests with
> test_0092, test_0096, and test_0097 but I'd like to think about it more
> before doing so.
Also, in a separate bug.

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 764551 [details] [diff] [review]
1. main patch rev1

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

If you'd like to you can push all these patches to:
https://hg.mozilla.org/projects/oak/

And then fire off 2 nightly builds here by putting the latest changeset inside the "nightly builds on oak" field:
https://secure.pub.build.mozilla.org/buildapi/self-serve/oak

And then you can test to make sure updates work fine.  If you feel that the local testing and tests are enough though, that's fine.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +643,5 @@
> +    LOG("getCanStageUpdates - able to stage updates because we'll use the service");
> +    return true;
> +  }
> +#endif
> +  return gCanStageUpdates;

It's a bit strange to me that we have both gCanStageUpdates and getCanStageUpdates.  Please add a comment to both about the differences and referencing each other so that future people working on this code will know which one to use. And/Or possibly differentiate the names to indicate the difference.

@@ +1902,5 @@
>        this._offlineStatusChanged(data);
>        break;
> +    case "nsPref:changed":
> +      if (data == PREF_APP_UPDATE_LOG) {
> +        gLogEnabled = getPref("getBoolPref", PREF_APP_UPDATE_LOG, false);

nice :)

@@ +3142,5 @@
>  
>      // Do this after *everything* else, since it will likely cause the app
>      // to shut down.
> +#ifdef MOZ_WIDGET_GONK
> +    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC) {

nit: update.state == STATE_APPLIED_SVC is only used on windows

@@ +3158,5 @@
> +    let windowType = getPref("getCharPref", PREF_APP_UPDATE_ALTWINDOWTYPE, null);
> +    if (Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) ||
> +        windowType && Services.wm.getMostRecentWindow(windowType)) {
> +      return;
> +    }

Rather than checking this pref again, I think you can just do 
let altUpdateWindow = this._getAltUpdateWindow();
let updateWindow = Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME);
if (altUpdateWindow || updateWindow) {
  return;
}

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 764552 [details] [diff] [review]
3. tests rev1

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

::: toolkit/mozapps/update/test/chrome/test_0097_restartNotification_stagedServiceBackground.xul
@@ +23,5 @@
>    pageid: PAGEID_FINISHED_BKGRD,
>    buttonClick: "extra1"
>  } ];
>  
> +var g

nit: I don't think this should be here

::: toolkit/mozapps/update/test/chrome/utils.js
@@ +186,5 @@
>  
>  // Set to true to log additional information for debugging. To log additional
>  // information for an individual test set DEBUG_AUS_TEST to true in the test's
>  // onload function.
> +var DEBUG_AUS_TEST = true;

Is this intentionally left true?

@@ +806,5 @@
>  /**
> + * Creates a backup of files the tests need to modify so they can be restored to
> + * the original file when the test has finished and then modifies the files.
> + */
> +function setupFiles() {

It would be nice to maybe pass in the basedir here with getAppBasesDir() as the default if not passed, and then use it in the other tests that have this code duplicated, but I won't hold up the review for that.

@@ +872,5 @@
>  
>  /**
> + * Restores files that were backed up for the tests and general file cleanup.
> + */
> +function resetFiles() {

see note in setupFiles

::: toolkit/mozapps/update/test/sharedUpdateXML.js
@@ +36,5 @@
> +                               "dd8d7be2859cfd2f75b242f272a8fbbcbe4419d399259" +
> +                               "35d732";
> +const SHA512_HASH_SIMPLE_MAR = "9f9afbb5a6fa70e71fa28c73c5b1968cf8831ae02ebd4" +
> +                               "28839ed2acf0b257c08c76c1f286c94c029dd3a4ad708" +
> +                               "f9703f61e6bf5f30982e9deee7d6838c1dbe14";

This seems fine but just wanted to check why these are changing

::: toolkit/mozapps/update/test_svc/unit/test_0203_app_launch_apply_update_svc.js
@@ +261,5 @@
>    resetEnvironment();
>  
>    let processDir = getAppDir();
>    // Restore the backup of the updater.ini if it exists
> +  // Restore the backup of the updater.ini if it exists.

nit: This comment already exists, pls remove the dupe without the period

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #18)
> Comment on attachment 764552 [details] [diff] [review]
> 3. tests rev1
> 
> Review of attachment 764552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/mozapps/update/test/chrome/
> test_0097_restartNotification_stagedServiceBackground.xul
> @@ +23,5 @@
> >    pageid: PAGEID_FINISHED_BKGRD,
> >    buttonClick: "extra1"
> >  } ];
> >  
> > +var g
> 
> nit: I don't think this should be here
Fixed

> ::: toolkit/mozapps/update/test/chrome/utils.js
> @@ +186,5 @@
> >  
> >  // Set to true to log additional information for debugging. To log additional
> >  // information for an individual test set DEBUG_AUS_TEST to true in the test's
> >  // onload function.
> > +var DEBUG_AUS_TEST = true;
> 
> Is this intentionally left true?
No... see comment #13

> @@ +806,5 @@
> >  /**
> > + * Creates a backup of files the tests need to modify so they can be restored to
> > + * the original file when the test has finished and then modifies the files.
> > + */
> > +function setupFiles() {
> 
> It would be nice to maybe pass in the basedir here with getAppBasesDir() as
> the default if not passed, and then use it in the other tests that have this
> code duplicated, but I won't hold up the review for that.
This is mochitest specific and there are already xpcshell helpers for that where they make multiple calls. I'll look into it when I do more test code cleanup.

> 
> @@ +872,5 @@
> >  
> >  /**
> > + * Restores files that were backed up for the tests and general file cleanup.
> > + */
> > +function resetFiles() {
> 
> see note in setupFiles
> 
> ::: toolkit/mozapps/update/test/sharedUpdateXML.js
> @@ +36,5 @@
> > +                               "dd8d7be2859cfd2f75b242f272a8fbbcbe4419d399259" +
> > +                               "35d732";
> > +const SHA512_HASH_SIMPLE_MAR = "9f9afbb5a6fa70e71fa28c73c5b1968cf8831ae02ebd4" +
> > +                               "28839ed2acf0b257c08c76c1f286c94c029dd3a4ad708" +
> > +                               "f9703f61e6bf5f30982e9deee7d6838c1dbe14";
> 
> This seems fine but just wanted to check why these are changing
The mochitests are now using simple.mar instead of simple_no_pib.mar so the hash changes. I've also added FILE_SIMPLE_NO_PIB_MAR and associated hashes which used to be FILE_SIMPLE_MAR since I want to add mochitests using them.

> 
> :::
> toolkit/mozapps/update/test_svc/unit/test_0203_app_launch_apply_update_svc.js
> @@ +261,5 @@
> >    resetEnvironment();
> >  
> >    let processDir = getAppDir();
> >    // Restore the backup of the updater.ini if it exists
> > +  // Restore the backup of the updater.ini if it exists.
> 
> nit: This comment already exists, pls remove the dupe without the period
Fixed

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Comment on attachment 764551 [details] [diff] [review]
> 1. main patch rev1
> 
> Review of attachment 764551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you'd like to you can push all these patches to:
> https://hg.mozilla.org/projects/oak/
> 
> And then fire off 2 nightly builds here by putting the latest changeset
> inside the "nightly builds on oak" field:
> https://secure.pub.build.mozilla.org/buildapi/self-serve/oak
> 
> And then you can test to make sure updates work fine.  If you feel that the
> local testing and tests are enough though, that's fine.
I pretty confident these changes are safe to land as is.

> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +643,5 @@
> > +    LOG("getCanStageUpdates - able to stage updates because we'll use the service");
> > +    return true;
> > +  }
> > +#endif
> > +  return gCanStageUpdates;
> 
> It's a bit strange to me that we have both gCanStageUpdates and
> getCanStageUpdates.  Please add a comment to both about the differences and
> referencing each other so that future people working on this code will know
> which one to use. And/Or possibly differentiate the names to indicate the
> difference.
Fixed

> @@ +1902,5 @@
> >        this._offlineStatusChanged(data);
> >        break;
> > +    case "nsPref:changed":
> > +      if (data == PREF_APP_UPDATE_LOG) {
> > +        gLogEnabled = getPref("getBoolPref", PREF_APP_UPDATE_LOG, false);
> 
> nice :)
It is about time isn't it? :)

> 
> @@ +3142,5 @@
> >  
> >      // Do this after *everything* else, since it will likely cause the app
> >      // to shut down.
> > +#ifdef MOZ_WIDGET_GONK
> > +    if (update.state == STATE_APPLIED || update.state == STATE_APPLIED_SVC) {
> 
> nit: update.state == STATE_APPLIED_SVC is only used on windows
Already fixed (see comment #11)

> 
> @@ +3158,5 @@
> > +    let windowType = getPref("getCharPref", PREF_APP_UPDATE_ALTWINDOWTYPE, null);
> > +    if (Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) ||
> > +        windowType && Services.wm.getMostRecentWindow(windowType)) {
> > +      return;
> > +    }
> 
> Rather than checking this pref again, I think you can just do 
> let altUpdateWindow = this._getAltUpdateWindow();
> let updateWindow = Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME);
> if (altUpdateWindow || updateWindow) {
>   return;
> }
this._getAltUpdateWindow is in UpdatePrompt. I considered refactoring things but chose to hold off on it.

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: 1. main patch rev2

Carrying forward r+

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: 2. additional fixes

I'm going to fix the hasMutex issue I discussed on irc with you in another bug.

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: 3. remove now unused method (trunk only)

carrying forward r+

Comment 24

[Robert Strong (they/them - no direct email)]

Attachment: 4. tests with comments addressed

carrying forward r+

Comment 25

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #22)
> Created attachment 765047 [details] [diff] [review]
> 2. additional fixes
> 
> I'm going to fix the hasMutex issue I discussed on irc with you in another
> bug.
Filed bug 885096

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: Aurora patch

Comment 27

[Robert Strong (they/them - no direct email)]

Attachment: Aurora tests

Comment 28

[Robert Strong (they/them - no direct email)]

Also running the changes through try again
https://tbpl.mozilla.org/?tree=Try&rev=acd8a42275d2

Comment 29

[Robert Strong (they/them - no direct email)]

Attachment: m-c combined main patch

Pushed combined main patch to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f6325c015e

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: m-c combined test patch

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d23a7000b52

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: Beta main patch

Fixed a typo in a log message

Comment 32

[Robert Strong (they/them - no direct email)]

Attachment: Beta test patch

Comment 33

[Robert Strong (they/them - no direct email)]

I ran tests on Win 7 aurora and they passed as well.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c7f6325c015e
https://hg.mozilla.org/mozilla-central/rev/5d23a7000b52

Comment 35

[Robert Strong (they/them - no direct email)]

I discussed this bug with akeybl today and decided on holding off on requesting branch approval until after it has baked a little longer. We can evaluate taking this on beta in a week or so. Also, the aurora patches will be applicable for beta by that time.

Comment 36

[Robert Strong (they/them - no direct email)]

I verified this with a background update by changing
app.update.promptWaitTime to 10
and
app.update.idletime to 10

Both the "toast" notification and the restart prompt were displayed.

Comment 37

[Robert Strong (they/them - no direct email)]

bug 829221 uncovered this bug and can be used to verify that this is fixed. akeybl has asked to get this onto beta if possible so asking for verification on aurora / nightly.

Comment 38

[u279076]

Is the snippet from bug 829221 comment 1 still available to qualify this?

Comment 39

[u279076]

Need-infoing nthomas to answer comment 38.

Comment 40

[Nick Thomas [:nthomas] (UTC+12)]

I've set it up again, see bug 829221 comment #22.

Comment 41

[u279076]

This is working for Firefox 24.0a1 2013-06-22 on Windows 7.

Comment 42

[Robert Strong (they/them - no direct email)]

Comment on attachment 765128 [details] [diff] [review]
Beta main patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 307181
User impact if declined: Windows users with the service and staging enabled won't be notified to restart Firefox primarily after an update has been downloaded and staged in the background. This also blocks bug 829221 and bug 829227 and akeybl asked me to uplift this if I felt it was safe to do so which I do.
Testing completed (on m-c, etc.): on m-c, verified by QA, verified by myself, and has very good test coverage in the tree.
Risk to taking this patch (and alternatives if risky): minimal due to testing completed.
String or IDL/UUID changes made by this patch: None.

Comment 43

[Alex Keybl [:akeybl]]

Comment on attachment 765128 [details] [diff] [review]
Beta main patch

\o/ \o/ \o/

Comment 44

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/bf59aa6d1747

This actually landed in time for the Firefox 23 merge so changing milestone.

Comment 45

[Robert Strong (they/them - no direct email)]

meh... so many version numbers! It landed for Firefox 24 so putting milestone back

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet ESR landing criteria, wontfixing for that branch.