893781 - Defect - Restart after update button no longer works

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

+++ This bug was initially created as a clone of Bug #874323 +++

Intermittently when applying an update through metro, the restart button receives focus but nothing happens.

p=2

Comment 1

[Brian R. Bondy [:bbondy]]

[10:28:34] <jimm> Error: 'caller, 'callee', and arguements properties may not be accessed on strict mode functions or the arguements objects for calls to them
[10:28:47] <jimm> aboutFlyout.js line 572
[10:29:00] <jimm> ReferenceError: aAppUpdater not defined
[10:29:06] <jimm> correction:
[10:29:16] <jimm> ReferenceError: gAppUpdate not defined
[10:29:21] <jimm> bah
[10:29:24] <jimm> gAppUpdater
[10:29:32] <jimm> there's two of those
[10:29:52] <bbondy> sweet, k I'll add that into the bug and work on it soonish
[10:30:01] <jimm> that error is generated every time I hit the button

Comment 2

[Brian R. Bondy [:bbondy]]

Looks like this regressed in bug 845468, was working originally.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: bug893781_updater.diff

I posted bug 896521 to add tests since this regression would have been avoided if we had tests in place. I'll do those when I have a bit more time after apzc and  after the updater work I have to move to for rstrong.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: bug893781_updater.diff

Forgot to qref in a line setting the new property to null.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 779231 [details] [diff] [review]
bug893781_updater.diff

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

It looks like the change that caused this issue to appear is adding 'use strict' at the top of aboutFlyout.js. In appUpdater.onStopRequest we're using 'arguments.callee' which isn't allowed in strict mode. I think the correct fix would be to name the function on line 550:
    Services.obs.addObserver(function NAME_WOULD_GO_HERE(aSubject, aTopic, aData) {
so that we can reference it on line 572:
    Services.obs.removeObserver(NAME_WOULD_GO_HERE, "update-staged");
instead of using arguments.callee.

I haven't actually tested that this fixes the issue; I'm just reasoning about the js, and my mental js interpreter is pretty flaky so let me know if I'm way off base :)

Fixing the "arguments.callee" issue might just uncover other issues from the addition of "use strict".  If that turns out to be the case, we could just remove "use strict" from the top of aboutFlyout.js.

Comment 6

[Brian R. Bondy [:bbondy]]

The issue fixed here has to do with the new way we're loading the scripts, it's not making gAppUpdater visible outside of that file.   the "use strict" may also be an issue but I'd prefer to fix that in another bug.

Comment 7

[Brian R. Bondy [:bbondy]]

Note that removing "use strict" does not solve the issue in Comment 0.
The issue is that as of the blocked bug we load the scripts in a sandbox so gAppUpdater is not defined.

> Services.scriptloader.loadSubScript(script, sandbox);

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 779231 [details] [diff] [review]
bug893781_updater.diff

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

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> Note that removing "use strict" does not solve the issue in Comment 0.
> The issue is that as of the blocked bug we load the scripts in a sandbox so
> gAppUpdater is not defined.
> 
> > Services.scriptloader.loadSubScript(script, sandbox);

Ah, gotcha. I was looking at the first issue mentioned in comment 1.

In that case, I like the patch, but can we get rid of gAppUpdater entirely and just use AppUpdate.appUpdater?

Comment 9

[Brian R. Bondy [:bbondy]]

I'd of normally did that but rstrong asked that we keep the code as close as possible to the desktop updater code, so that's why I didn't consolidate to that.  I'll test this out on oak and if it can't update or something I'll try what yous said in addition to this.

Comment 10

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

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> I'd of normally did that but rstrong asked that we keep the code as close as
> possible to the desktop updater code, so that's why I didn't consolidate to
> that.  I'll test this out on oak and if it can't update or something I'll
> try what yous said in addition to this.
Specifically, I asked to keep them the same where possible and to diverge if it makes sense to. If there are good reasons to diverge then by all means do so.

Comment 11

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

Also, if there are changes to this code that make sense to port in the other direction please do so.

Comment 12

[Brian R. Bondy [:bbondy]]

In this case it would create about 50 differences and instead we can use 2 line workaround, so I think this way aligns best with what you said.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: rev 2

Carrying forward r+, haven't tested yet but I figured I might as well also fix the error you mentioned. That one didn't cause any harm except for the observer not being removed though, but might as well fix instead of posting new bug.

Comment 14

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/1ae3aef92373

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1ae3aef92373

Comment 17

[Cornel Ionce [:noni]]

Mozilla/5.0 (Windows NT 6.2; WOW64, rv:25.0) Gecko/20130730 Firefox/25.0

Verified this issue as fixed. The restart button now works after updating through metro.

Comment 18

[Manuela Muntean [Away]]

I've noticed that the issue has regressed and started to reproduce again.

On Win 8 64bit, I've tried to update the Nightly from 2013-07-31, from the Metro interface, using the "About" flyout, but after pressing the restart button nothing happens. So, after closing Metro and reopening it, I can see a Windows dialog asking me if I allow the "updater" to perform, I say yes, and when checking the "About" flyout, I can see that Metro is updated.

Also, when I open Nightly, it is also updated.  Does anyone else see this behavior, too?

Comment 19

[Brian R. Bondy [:bbondy]]

It sounds like your updater service is disabled.  Can you check in Firefox options, advanced, update tab, is the checkbox for enabling the service on or off?  

As for the button not working I guess I'd post a new issue for that if it's back, maybe someone landed something that broke it again.

Comment 20

[Manuela Muntean [Away]]

> It sounds like your updater service is disabled.  Can you check in Firefox
> options, advanced, update tab, is the checkbox for enabling the service on
> or off?  

I have the "Automatically update from desktop and Windows 8 style Nightly" checkbox selected.

Comment 21

[Brian R. Bondy [:bbondy]]

Please delete this folder out:
C:\ProgramData\Mozilla\logs

Then reproduce the problem, then attach a zip of the files inside:
C:\ProgramData\Mozilla\logs

Thanks!

Comment 22

[Manuela Muntean [Away]]

I've deleted the folder "logs" completely from C:\ProgramData\Mozilla\logs, then installed the Nightly 64bit from 2013-07-31 (and let it install in the custom folder, C:\Program Files\Nightly), set Nightly as default browser, opened Metro and started to update from the "About" flyout. I reproduced the issue from comment 18, but afterwards, I couldn't find no "logs" folder there....so I can't attach a zip with the files from it. In this case, is there anything else I can do to help?

Another issue that occurs: on the OS start screen, the Nightly icon is displayed instead of the Metro one

Comment 23

[Brian R. Bondy [:bbondy]]

Sounds like you have a bunch of different issues that are not related.  Are you maybe using Windows 8.1 for testing?

Comment 24

[Manuela Muntean [Away]]

(In reply to Brian R. Bondy [:bbondy] from comment #23)
> Sounds like you have a bunch of different issues that are not related.  Are
> you maybe using Windows 8.1 for testing?

No, it's Windows 8 64bit

Comment 25

[Brian R. Bondy [:bbondy]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #22)
> I've deleted the folder "logs" completely from C:\ProgramData\Mozilla\logs,
> then installed the Nightly 64bit from 2013-07-31 (and let it install in the
> custom folder, C:\Program Files\Nightly), set Nightly as default browser,
> opened Metro and started to update from the "About" flyout. I reproduced the
> issue from comment 18, but afterwards, I couldn't find no "logs" folder
> there....so I can't attach a zip with the files from it. In this case, is
> there anything else I can do to help?
> 
> Another issue that occurs: on the OS start screen, the Nightly icon is
> displayed instead of the Metro one

I think some of these are issues related to when the service is disabled, and x64 builds do not use the service right now. Please try with an x86 build.

Comment 26

[Jim Mathies [:jimm]]

This regressed recently. I'll get console output on tomorrows update.

Comment 27

[Samvedana (:Samvedana)]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

WFM
Tested on windows 8 using latest nightly for iteration-12. Firefox Metro restarted after updating.