I introduced a bug in the stub installer in bug 1451366: due to insufficient quoting at , the macro command line parameter is interpreted as multiple arguments to StrCpy. When Firefox is intended to start with an argument, as in , 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 . 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.  https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/toolkit/mozapps/installer/windows/nsis/common.nsh#8135  https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/browser/installer/windows/nsis/stub.nsi#1664
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+
Pushed by firstname.lastname@example.org: 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: --- → ?
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.
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: ? → +
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
Verified on Nightly. Sorry for the delay, hopefully this can still make 62.0.2 given the AV holdup.
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
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+
status-firefox62: affected → fixed
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
You need to log in before you can comment on or make changes to this bug.