1491999 - Stub installer fails to launch Firefox when refresh is enabled

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

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

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: 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.

Comment 2

[Molly Howell (she/her) (no longer active)]

(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 3

[Molly Howell (she/her) (no longer active)]

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

Matt Howell [:mhowell] has approved the revision.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e22acd56b75
Quote command line for multiple components. r=mhowell

Comment 5

[Jim Mathies [:jimm]]

[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.

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

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

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4e22acd56b75

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

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?

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

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

Comment 10

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Verified on Nightly.
Sorry for the delay, hopefully this can still make 62.0.2 given the AV holdup.

Comment 11

[Pascal Chevrel:pascalc]

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

Uplift approved for 63 beta 8, thanks.

Comment 12

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57df5aaadcbc

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

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

stub installer regression fix for 62.0.2

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/150c70d39363

Comment 16

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.