311614 - Partial Update Apply failure Window after app startup should appear in front of browser window, not behind

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Marcia Knous [:marcia]]

Seen during Beta1->Beta2 testing

The partial update window appears behind the Firefox window, when it probably
should appear in the front since it it requires user intervention to continue
(when the partial update could not be applied)

Comment 1

[Marcia Knous [:marcia]]

nominating.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I see this in Windows, too.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: possible patch

This seems to work, given some limited testing, but it's kind of hacky. Changes
the dialog to a modal one, and changes the attempted restart to a forced one to
prevent the loading of the browser window after choosing downloading the full
update and choosing "restart and apply now".

Comment 4

[Asa Dotzler [:asa]]

Comment on attachment 198979 [details] [diff] [review]
possible patch

Darin, or Ben, can one of you look at this patch? Thanks.

Comment 5

[Asa Dotzler [:asa]]

Ben, this makes our update feature much harder for users understand, especially
on the mac, but it impacts all platforms. Can you dig into this.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 198979 [details] [diff] [review]
possible patch

This should be safe.  We already do plenty of checking for actions that should
abort a quit before this, so this is actually the best behaviour in this
situation.

Comment 7

[Darin Fisher]

Comment on attachment 198979 [details] [diff] [review]
possible patch

I'm not comfortable taking the modal dialog changes without review from Ben. 
It was his intention that this dialog be non-modal.

Comment 8

[Mike Connor [:mconnor]]

Darin, in the case where we failed to update and we're going to reattempt
updating, do we want to show browser UI?  That seems like a mixed message, to me.

Comment 9

[Darin Fisher]

> Darin, in the case where we failed to update and we're going to reattempt
> updating, do we want to show browser UI?  That seems like a mixed message, to me.

"reattempt updating" means downloading a complete update in most cases.  That
could take hours to complete, so yes... I don't think we want it to prevent the
user from browsing the web during that time.

I think the challenge here is that the initial panel of the wizard UI is
informative about the error, but subsequent panels show download progress.

Comment 10

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

Comment on attachment 198979 [details] [diff] [review]
possible patch

I don't think it should be modal. Instead, it should show the error and
automatically grab the larger file. Then it doesn't matter so much that it's in
the background. Creating a modal dialog for an extended duration operation is
not the right idea.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Another alternative would be to have the window focus itself after a delay...
That has the potential of being equally confusing, I suppose.

Comment 12

[Darin Fisher]

> Another alternative would be to have the window focus itself after a delay...
> That has the potential of being equally confusing, I suppose.

Right, and you also have to worry about the default dialog option being
mistakenly accepted by the user if the window gets focus while the user is
typing into the browser window.

I personally don't think it's that big of a deal if the partial update window
appears behind the browser window.  It's fine if the user notices the dialog
later.  Moreover, this is an edge case.  Most users will never experience this
dialog.  I say punt on this for 1.5 final since I think the real solution
involves breaking the dialog into two parts: a modal informative dialog and a
non-modal progress dialog.  I don't see us making that kind of UI change now.

Comment 13

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

-> bugs acct so it shows up on my bookmarked queries. 

Comment 14

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

This only happens after a patch fails, right? Which implies a glitch in either
the build id, or the distribution site. This is very unlikely. And it's not
catastrophic. Like Darin, I don't think this is a blocker. 

Comment 15

[Scott MacGregor]

This was a bigger deal on the Mac IIRC because you don't know the window exists
at all since there is no taskbar.

Comment 16

[Marcia Knous [:marcia]]

I had my Windows machine set for auto update and let it run overnight.  When I
came in this morning, the software update window was behind the open browser
window. I believe this to a bad user experience that we should fix at some
point. I see the bug got minused already, but if it isn't a difficult fix I
think we should still consider it.

Comment 17

[Marcia Knous [:marcia]]

I hit the trigger a bit soon. I just forced an update on the same machine and
the software update window appeared in front of the browser window.  Let me do a
little bit more testing and report back.

Comment 18

[Scott MacGregor]

nominating for 1.8.1

Comment 19

[Marcia Knous [:marcia]]

This bug has a patch and seems to still be sub optimal behavior on the Mac where you don't get a notification in the taskbar.  Nominating for 1.8.0.2.

Comment 20

[Daniel Veditz [:dveditz]]

no reviewed, tested patch: not making 1.5.0.2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Dão, this seems to be related to the work you're doing in bug 391598. Can you comment?

Comment 22

[Dão Gottwald [:dao]]

No, what I'm doing in bug 391598 isn't related to this bug.

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Patch

This should work out of the box for Firefox, Fennec, SeaMonkey, Thunderbird and Instantbird. Any other apps still have a race condition between the 'final-ui-startup' notification (which is "Triggered just before the first window for the application is displayed."[1]) and the browser window actually displaying. But even this is expected to be better than the 'profile-after-change' notification used previously.

Comment 26

[Stephen A Pohl [:spohl]]

[1] https://developer.mozilla.org/en-US/docs/Observer_Notifications

Comment 27

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

Comment on attachment 8750595 [details] [diff] [review]
Patch

There are no other consumers of resource://gre/modules/sdk/system/Startup.js in the tree besides add-on code and I really don't want app update relying on code that it is the only consumer for. That typically ends in tears.

http://mxr.mozilla.org/mozilla-central/search?string=%2FStartup.js&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

I'll take a look at this later this week.

Comment 28

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 29

[Stephen A Pohl [:spohl]]

Add-on code is still the only other consumer, but by moving this to toolkit/modules I wonder if it's clear enough that this is no longer add-on specific.

Comment 30

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

Comment on attachment 8750852 [details] [diff] [review]
Patch

I don't want app update to rely on a module that no other code in the tree is using (besides the add-on sdk).

For our purposes this can be implemented in app update.

Note: nsUpdateServiceStub.js should not need any changes. nsUpdateService.js should have the additional observer added which lessens the calls during startup.

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 32

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 33

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

Comment on attachment 8751582 [details] [diff] [review]
Patch

MUCH NICER!

You could use the app ID as the key and avoid the for loop.

Instead of keeping a values of all of nsIXULAppInfo in this._appInfo just use
Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID;

We also set nsIXULAppInfo for some of the xpcshell tests and if any tests are failing because of this it would be better to just have the tests set the info if possible.

Comment 34

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 35

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

Comment on attachment 8751901 [details] [diff] [review]
Patch

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #33)
>...
> Instead of keeping a values of all of nsIXULAppInfo in this._appInfo just use
> Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID;
> 
> We also set nsIXULAppInfo for some of the xpcshell tests and if any tests
> are failing because of this it would be better to just have the tests set
> the info if possible.
As in just use Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID; directly instead of putting it in a function with a try catch and make the failing xpcshell tests initialize the custom nsIXULAppInfo so the try catch isn't necessary.

What do you think of putting APPID_TO_TOPIC in the observe function since that is the only place it is used?

Comment 36

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #35)
> Comment on attachment 8751901 [details] [diff] [review]
> Patch
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #33)
> >...
> > Instead of keeping a values of all of nsIXULAppInfo in this._appInfo just use
> > Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID;
> > 
> > We also set nsIXULAppInfo for some of the xpcshell tests and if any tests
> > are failing because of this it would be better to just have the tests set
> > the info if possible.
> As in just use
> Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID; directly
> instead of putting it in a function with a try catch and make the failing
> xpcshell tests initialize the custom nsIXULAppInfo so the try catch isn't
> necessary.

This just mirrors what's been done in[1]. My intent here was to avoid two calls to Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).ID;. Maybe that's not enough of a benefit for the additional member and method?

The try/catch was also taken from [2]. I haven't actually run all the xpcshell tests to see which ones (if any) are failing. I'm guessing the tests in toolkit/mozapps/update/tests/ should be enough?

> What do you think of putting APPID_TO_TOPIC in the observe function since
> that is the only place it is used?

If I put it before the switch statement it gets declared every time a notification comes in regardless of the notification. If I limit it to the notifications that we're interested in, I would have to declare it twice. I was trying to avoid both situations. Were you thinking of something else?

[1] http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/xul-app.jsm
[2] http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/xul-app.jsm#23

Comment 37

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

Went over this on irc

Comment 38

[Stephen A Pohl [:spohl]]

Attachment: Patch

Sending to try shortly.

Comment 39

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

Comment on attachment 8752000 [details] [diff] [review]
Patch

If tests need to be fixed please submit a new patch and thanks!

Comment 40

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=127576981d5d

Comment 41

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

Comment on attachment 8752000 [details] [diff] [review]
Patch

Sorry about this but I forgot that nsIXULAppInfo is available from Services.appinfo.
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm#22

Please use that instead.

Comment 42

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

BTW: you can just use Services.appinfo.ID for the comparisons without using variables. Also, since this is a jsm it will use the cached version on subsequent calls.

Comment 43

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

Looks like the xpcshell tests are passing though it is still waiting on the Windows tests.

The mochitest-other tests are timing out though. This is likely because nsUpdateServiceStub.js calling nsUpdateService.js observe with the post-update-processing topic and the observers not firing since this is just simulating startup.

One possible solution is to do what nsUpdateServiceStub.js does against nsUpdateService.js with a new observer topic for the test instead of using nsUpdateServiceStub.js. The new case statement for this topic should be below the removal of the observer to prevent it from trying to remove the observer that was never added.

Comment 44

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

and when you push this to try you only need to run mochitest-o and xpcshell on the supported platforms.

Comment 45

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/67ecb8c3b41caf32b1231472b1f9a58d6c5f4a0f
Bug 311614: Show update dialog in front of the browser window if one needs to be displayed after startup. r=rstrong,mhowell

Comment 46

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/128140f36993ae225a2602f7dea3148bb124f501
Bug 311614: Show update dialog in front of the browser window if one needs to be displayed after startup. r=rstrong,mhowell

Comment 47

[Stephen A Pohl [:spohl]]

Tests still need to be fixed, but Adrian, would you be able to verify the fix on Oak? The build is: 
http://archive.mozilla.org/pub/firefox/nightly/2016/05/2016-05-13-20-52-10-oak/firefox-49.0a1.en-US.mac.dmg 

It would be great to verify that both of these scenarios display the dialog in front of the browser:
1. During an update that requires elevation, verify that the dialog that notifies the user of the update is displayed in front of browser.
2. When canceling the username/password dialog during an elevated update, verify that the update failure dialog is displayed in front of the browser after the browser restarts.

Thanks!

Comment 48

[Adrian Florinescu [:aflorinescu]]

The fix looks fine on OSX 10.10 for the moment. I will keep the NI until I can check on 10.9 and 10.11.

Stephen does this fix affect anything else that should be checked, besides the elevate scenarios?

Comment 49

[Stephen A Pohl [:spohl]]

This same dialog is used to notify the user of various error messages etc. I *think* testing the two scenarios in comment 47 might be enough. Robert, do you agree or would you like to see other notifications/error dialogs tested as well? If so, could you let Adrian know how to test those? Thanks!

Comment 50

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #42)
> BTW: you can just use Services.appinfo.ID for the comparisons without using
> variables. Also, since this is a jsm it will use the cached version on
> subsequent calls.

This has come a long way and feels much cleaner. Thanks!

Comment 51

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/630696ec8c887434d3b798322564d9dfeb2e487f
Bug 311614: Use Services.appinfo.ID to retrieve nsIXULAppInfo.ID. r=rstrong

Comment 52

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

Comment on attachment 8752953 [details] [diff] [review]
Patch

This makes me very happy. :)

>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
>@@ -187,16 +187,31 @@ const DEFAULT_SOCKET_MAX_ERRORS = 10;
> 
> // The number of milliseconds to wait before retrying a connection error.
> const DEFAULT_UPDATE_RETRY_TIMEOUT = 2000;
> 
> // Default maximum number of elevation cancelations per update version before
> // giving up.
> const DEFAULT_MAX_OSX_CANCELATIONS = 3;
> 
>+// This maps app IDs to their respective notification topic which signals when
>+// app startup is complete.
nit: this isn't actually "app startup" and there is an app-startup topic which this would be confused with. Please use something more descriptive such as:
the application's user interface has been displayed.

Thanks!

Comment 53

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/cde4e42296982c4e676f338bd02465df50aa2b3e
Bug 311614: Update tests. r=rstrong

Comment 54

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed nit, carrying over r+.

Comment 55

[Stephen A Pohl [:spohl]]

Attachment: Test changes

All tests seem to pass with this patch (see Oak run in comment 53). Does this address everything that we've discussed via IRC? Would you like to change the naming of the notification topic and/or the new function in shared.js?

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Stephen, in case you also want to test with our Firefox UI tests, there are update tests we run for each nightly and release. You can trigger it via `mach firefox-ui-update`. Use --help to see more `--update-*` arguments you can pass in.

Comment 57

[ovidiu boca[:Ovidiu]]

We found a related sidebug, which manifests when FF is not default browser and the "default browser check" window sends the updater window behind the browser. 

STR:
1.Download the build and installed on admin user. http://archive.mozilla.org/pub/firefox/nightly/2016/05/2016-05-13-20-52-10-oak/firefox-49.0a1.en-US.mac.dmg
2.Switch to standard. go to About Nightly, and after the update is downloaded, click "Restart Nightly to update"
3. Software Update window is prompted in front of the browser. 
4. Click "Restart Later"
5. Go and click again "Restart Nightly to Update"


Actual results: 
The "Software Update" window is open behind the browser.

Expected results:
The "Software Update" window should be  open in front of the browser.

Note1: When you start FF and is not set as the default browser the "default browser check" window will be shown which will send the updater window behind the browser. 
Note2: After the 3rd restart, the "default browser check" window is not shown anymore.
If you uncheck the button "Always perform this check when starting Nightly" next time when you start the browser the "Software Update" window is displayed in front of the browser, as expected. So in conclusion we think that the "Default Browser" window is the cause.

If you want us to log a new issue for this, please instruct us so.

Comment 58

[Adrian Florinescu [:aflorinescu]]

Continuing Comment 48 and excluding the exception in the Comment 57, the updater window in the elevate case is verified on OSX 10.9 and 10.11 as well.

Comment 59

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin (:whimboo) from comment #56)
> Stephen, in case you also want to test with our Firefox UI tests, there are
> update tests we run for each nightly and release. You can trigger it via
> `mach firefox-ui-update`. Use --help to see more `--update-*` arguments you
> can pass in.

Is this supposed to work with local builds? I can't get `mach firefox-ui-update` to pass. All tests seem to fail, even if I don't have any patches applied and build trunk directly...

Comment 60

[Stephen A Pohl [:spohl]]

(In reply to ovidiu boca[:Ovidiu] from comment #57)
> [...]
> If you want us to log a new issue for this, please instruct us so.

Yes, please file a new bug. :rstrong mentioned that he will be providing additional steps to verify the fix here. Until then, please leave this bug open. Thanks!

Comment 61

[ovidiu boca[:Ovidiu]]

For comment 57 I've logged a new bug, you can find it here: https://bugzilla.mozilla.org/show_bug.cgi?id=1273536

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Stephen A Pohl [:spohl] from comment #59)
> Is this supposed to work with local builds? I can't get `mach
> firefox-ui-update` to pass. All tests seem to fail, even if I don't have any
> patches applied and build trunk directly...

You will need a build with updates enabled, and at least one update available. If this is not the case you could still overwrite the update URL via `--update-override-url` so that it points to a local update snippet (XML file) which contains a valid partial or complete mar file. 

If the oak branch is building nightly builds you can test it initially. Just grab a previous build and run the update against it via the --binary option. If you have more questions feel free to ping me in #automation.

Comment 63

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

Attachment: files to verify fix

To verify this fix works for the other cases you can unzip these files into the directory that contains the active-update.xml and updates.xml files. The window with the failure message should appear in front of the window.

Comment 64

[Stephen A Pohl [:spohl]]

Adrian, please do the following:
1. Check for an update with the build you've been testing on Oak
2. Once the button appears to restart to update in the about dialog, replace the files under ~/Library/Caches/Mozilla/updates/... with the files from attachment 8753587 [details]. 
3. Click the button to restart Firefox.

After restart, the failure dialog should appear in front of the browser window. Thanks!

Comment 65

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

Comment on attachment 8753119 [details] [diff] [review]
Test changes

>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
>@@ -1991,16 +1991,18 @@ UpdateService.prototype = {
>         // intentional fallthrough
>       case "sessionstore-windows-restored":
>       case "mail-startup-done":
>       case "xul-window-visible":
>         if (Services.appinfo.ID in APPID_TO_TOPIC) {
>           Services.obs.removeObserver(this,
>                                       APPID_TO_TOPIC[Services.appinfo.ID]);
>         }
>+        // intentional fallthrough
>+      case "mochitest-app-startup-finished":
please name this test-post-update-processing

>         // Clean up any extant updates
>         this._postUpdateProcessing();
>         break;
>       case "network:offline-status-changed":
>         this._offlineStatusChanged(data);
>         break;
>       case "nsPref:changed":
>         if (data == PREF_APP_UPDATE_LOG) {
>diff --git a/toolkit/mozapps/update/tests/data/shared.js b/toolkit/mozapps/update/tests/data/shared.js
>--- a/toolkit/mozapps/update/tests/data/shared.js
>+++ b/toolkit/mozapps/update/tests/data/shared.js
>@@ -132,16 +132,23 @@ XPCOMUtils.defineLazyServiceGetter(this,
>                                    "@mozilla.org/process/environment;1",
>                                    "nsIEnvironment");
> 
> XPCOMUtils.defineLazyGetter(this, "gZipW", function test_gZipW() {
>   return Cc["@mozilla.org/zipwriter;1"].
>          createInstance(Ci.nsIZipWriter);
> });
> 

Please add a doc comment
>+function signalAppStartupFinished() {
I'd go with testPostUpdateProcessing

>+  let aus = Cc["@mozilla.org/updates/update-service;1"].
>+            getService(Ci.nsIApplicationUpdateService).
>+            QueryInterface(Ci.nsIObserver);
>+  aus.observe(null, "mochitest-app-startup-finished", "");
Just use gAUS.observe(null, "test-post-update-processing", "");

Comment 66

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/66947a0678313afb4f651eb556f6e64d9442d871
Bug 311614: Address review feedback for test updates. r=rstrong

Comment 67

[Stephen A Pohl [:spohl]]

Attachment: Test changes

Comment 68

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin (:whimboo) from comment #62)
> (In reply to Stephen A Pohl [:spohl] from comment #59)
> > Is this supposed to work with local builds? I can't get `mach
> > firefox-ui-update` to pass. All tests seem to fail, even if I don't have any
> > patches applied and build trunk directly...
> 
> You will need a build with updates enabled, and at least one update
> available. If this is not the case you could still overwrite the update URL
> via `--update-override-url` so that it points to a local update snippet (XML
> file) which contains a valid partial or complete mar file. 
> 
> If the oak branch is building nightly builds you can test it initially. Just
> grab a previous build and run the update against it via the --binary option.
> If you have more questions feel free to ping me in #automation.

Thanks, this worked. All tests passed when run against an Oak build.

Comment 69

[ovidiu boca[:Ovidiu]]

I have tested this issue on Mac OS X 10.9, 10.10 and 10.11, following the instructions from comment 64, and it worked. The failure dialog appeared in front of the browser window, as expected.

Comment 70

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/fx-team/rev/d6eae4e1bac706a905d92b36cd10e5c19f48afe6
Bug 311614: Show update dialog in front of the browser window if one needs to be displayed after startup. r=rstrong

https://hg.mozilla.org/integration/fx-team/rev/55016b253dde7da0b07348bd66ee313c748c0a0c
Bug 311614: Update tests. r=rstrong

Comment 71

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated to what landed on fx-team.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6eae4e1bac7
https://hg.mozilla.org/mozilla-central/rev/55016b253dde

Comment 74

[Adrian Florinescu [:aflorinescu]]

Build ID 0160602030220 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:49.0) Gecko/20100101 Firefox/49.0 

Although intermittent, in the case of the elevated update the failure dialog or the elevated update window appears sometimes(50%) behind the browser. (best reproducible in the case of first elevate cancel)

Note: When we used the files and steps you gave us in comment 64 in the above Nightly build, the position of the window was always in front of the browser.

Comment 75

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

Adrian, please file a new bug for this and include steps to reproduce. Thanks