Stub installer fails to launch Firefox when refresh is enabled

VERIFIED FIXED in Firefox 62

Status

()

VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: agashlin, Assigned: agashlin)

Tracking

({regression})

62 Branch
Firefox 64
Unspecified
Windows
regression
Points:
---

Firefox Tracking Flags

(firefox62+ verified, firefox63 verified, firefox64 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
I introduced a bug in the stub installer in bug 1451366: due to insufficient quoting at [1], the macro command line parameter is interpreted as multiple arguments to StrCpy. When Firefox is intended to start with an argument, as in [2], this causes StrCpy to misbehave as it interprets arguments past the first as the length and start offset of the string to copy.

This should be simply fixed by quoting the macro parameter in [1].

I think this means that since this was introduced, performing a refresh from the stub installer would fail to start Firefox at all, and the refresh would not be performed.

[1] https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/toolkit/mozapps/installer/windows/nsis/common.nsh#8135
[2] https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/browser/installer/windows/nsis/stub.nsi#1664
Blocks: 1451366
(Assignee)

Comment 1

4 months ago
Created attachment 9009822 [details]
Bug 1491999: Quote command line for multiple components. r?mhowell

The macro expansion of ExecAndWaitForInputIdle consumes the quotes
around the macro argument. The argument has an extra layer of quotes
around the executable name, which keeps it together even if it has
spaces, but we need yet another layer around the whole command line or
else each component of the command line appears as another argument to
StrCpy.

Standard practice is to quote args in the macro definition, I overlooked
this in the initial implementation.
(In reply to Adam Gashlin [:agashlin] from comment #1)
> Standard practice is to quote args in the macro definition, I overlooked
> this in the initial implementation.

To be fair to Adam, I'll point out I also overlooked this in the initial review.
Comment on attachment 9009822 [details]
Bug 1491999: Quote command line for multiple components. r?mhowell

Matt Howell [:mhowell] has approved the revision.
Attachment #9009822 - Flags: review+

Comment 4

4 months ago
Pushed by agashlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e22acd56b75
Quote command line for multiple components. r=mhowell
[Tracking Requested - why for this release]:
A regression with launching firefox after an install has been discovered in our release product. We probably want this in a point release as a ride along. Note this is only for "refresh" installs, so it's limited in scope. We're tracking down some telemetry on this now.
status-firefox62: --- → affected
status-firefox63: --- → affected
status-firefox64: --- → affected
tracking-firefox62: --- → ?
(Assignee)

Comment 6

4 months ago
Here's the stats for whether installs requested refresh for the last few weeks:

https://sql.telemetry.mozilla.org/queries/59036

It looks like around 12% of all stub installer pings. These installs aren't broken, but Firefox won't start automatically after the installer runs and refresh won't be performed.

Comment 7

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e22acd56b75
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
I'm hoping to gtb 62.0.2 today.  Adam, can you request uplift to beta and release ASAP?  Brindusa, can your team verify on nightly?
tracking-firefox62: ? → +
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
Flags: needinfo?(agashlin)
(Assignee)

Comment 9

4 months ago
Comment on attachment 9009822 [details]
Bug 1491999: Quote command line for multiple components. r?mhowell

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1451366
[User impact if declined]: Users eligible for refresh (see bug 1369255) will not start running Firefox automatically after install, they will have to run it manually.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, the fix is working in
https://archive.mozilla.org/pub/firefox/nightly/2018/09/2018-09-19-12-38-06-mozilla-central/Firefox%20Installer.en-US.exe

[Needs manual test from QE? If yes, steps to reproduce]: Yes.
1. Install Firefox
2. Uninstall
3. Install from an affected stub installer, allow the install to proceed with refresh
After the install the affected installer will not start Firefox.

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The error and fix are straightforward, this is a return to the old behavior.
[String changes made/needed]: None
Flags: needinfo?(agashlin)
Attachment #9009822 - Flags: approval-mozilla-release?
Attachment #9009822 - Flags: approval-mozilla-beta?
(Assignee)

Comment 10

4 months ago
Verified on Nightly.
Sorry for the delay, hopefully this can still make 62.0.2 given the AV holdup.
Flags: needinfo?(brindusa.tot)
Comment on attachment 9009822 [details]
Bug 1491999: Quote command line for multiple components. r?mhowell

Uplift approved for 63 beta 8, thanks.
Attachment #9009822 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I also can confirm that 64.0a1 (2018-09-19) is verified fixed. Now Firefox is successfully launched after the stub installation (reproduced the initial issue using the 64.0a1 (2018-09-17) stub build and Windows 10 x64). Based on comment 10, too, I will mark Firefox 64 accordingly. 
I will finish the verification process as soon as the latest beta and release stub builds will be available.
Status: RESOLVED → VERIFIED
status-firefox64: fixed → verified

Comment 13

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/57df5aaadcbc
status-firefox63: affected → fixed
Comment on attachment 9009822 [details]
Bug 1491999: Quote command line for multiple components. r?mhowell

stub installer regression fix for 62.0.2
Attachment #9009822 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 15

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/150c70d39363
status-firefox62: affected → fixed
Keywords: regression
As a followup for comment 12, I can confirm the fix is successfully implemented on 62.0.2 build1 (20180920131237) and 63.0b8 build1 (20180920135444), using  Windows 10 x64.
status-firefox62: fixed → verified
status-firefox63: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.