Closed Bug 893781 Opened 6 years ago Closed 6 years ago

Defect - Restart after update button no longer works

Categories

(Toolkit :: Application Update, defect, P1)

x86_64
Windows 8.1
defect

Tracking

()

VERIFIED FIXED
mozilla25

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1)

Attachments

(1 file, 2 obsolete files)

+++ 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
Summary: Defect - Metro does not restart itself ater closing after an update because Ci.nsIAppStartup.eRestart is not supported in Metro → Defect - Restart after update button sometimes doesn't work
Blocks: metrov1defect&change
No longer blocks: metrov1it8
[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
No longer blocks: 874323
QA Contact: jbecerra
Looks like this regressed in bug 845468, was working originally.
Blocks: 845468
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1
Assignee: nobody → netzen
Status: NEW → ASSIGNED
Priority: P2 → P1
Summary: Defect - Restart after update button sometimes doesn't work → Defect - Restart after update button no longer works
Attached file bug893781_updater.diff (obsolete) —
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.
Attachment #779228 - Flags: review?(tabraldes)
Attached patch bug893781_updater.diff (obsolete) — Splinter Review
Forgot to qref in a line setting the new property to null.
Attachment #779231 - Flags: review?(tabraldes)
Attachment #779228 - Attachment is patch: false
Attachment #779228 - Flags: review?(tabraldes)
Attachment #779228 - Attachment is obsolete: true
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.
Attachment #779231 - Flags: review?(tabraldes)
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.
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 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?
Attachment #779231 - Flags: review+
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.
(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.
Also, if there are changes to this code that make sense to port in the other direction please do so.
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.
Attached patch rev 2Splinter Review
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.
Attachment #780560 - Flags: review+
Attachment #779231 - Attachment is obsolete: true
Duplicate of this bug: 891653
https://hg.mozilla.org/mozilla-central/rev/1ae3aef92373
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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?
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.
> 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.
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!
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
Flags: needinfo?(netzen)
Sounds like you have a bunch of different issues that are not related.  Are you maybe using Windows 8.1 for testing?
Flags: needinfo?(netzen)
(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
(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.
This regressed recently. I'll get console output on tomorrows update.
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.