1375472 - Install timeout regression as a consequence of DLL delay loading from bug 1361326

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Comment 1

[Chris Peterson [:cpeterson]]

The install_time only measures the time that the full installer is running and does not include any time spent on network download or signature verification.

Romain identified that the install_time regression started with Firefox 54.0beta10, released on May 23rd. The fix for installer bug 1361326 shipped in the beta and is probably the cause.

https://hg.mozilla.org/releases/mozilla-beta/rev/61932206bf73

Comment 2

[Chris Peterson [:cpeterson]]

As this install timeout regression affects both 32- and 64-bit installs, it does not need to block our 64-bit rollout.

Comment 3

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

I can try to get some numbers on whether bug 1361326 is really slowing down the full installer; I don't have any reason to believe it should, but I can still check. If so, then the short-term mitigation for that will be to increase the timeout by a proportional amount; bug 1361326 is a necessary security fix and should not be backed out over this. The long-term mitigation is to finish bug 995891.

Comment 4

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

I got the numbers, and yes, bug 1361326 is really slowing down the full installer. The reason isn't anything caused by the actual change, it's that I made a mistake implementing it.

Bug 1361326 required changing (only) the 7-zip SFX code. We store that code in both source and binary form in the tree, so changing it means rebuilding the binary and submitting that along with the code patch. Bug 1361326 was the first time I had done that build myself, and I accidentally ran it with compiler optimizations disabled. That means it's running for at least several seconds longer than it should be. I'll submit a patch here with an optimized build.

Here are the numbers I collected. I ran the installer using the /extractDir option so that only the 7-zip binary was run and not the rest of the installer, and averaged that running time over ten trials:
Version                Time
before bug 1361326     9.40s
after bug 1361326     16.71s
Rebuilt optimized      7.33s

So on my machine, which is quite fast, this bug is costing 8 or 9 seconds. On slower machines, I expect the difference would be greater, so this appears to explain much or all of the observed install timeout increase.

Patch to follow.

Comment 5

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

Attachment: Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

Bug 1361326 required rebuilding the 7-zip self-extractor executable, which is
stored in binary form in the tree. I accidentally built that binary with all
compiler optimizations disabled. This binary is built from identical code as
the one from bug 1361326, the only difference is this one was built with /O2
instead of /Od.

Review commit: https://reviewboard.mozilla.org/r/154564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154564/

Comment 6

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

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

Note to reviewer: I copied over all the resources from the previous file into this one, and also manually verified that the fix for bug 1361326 is still present, just to make sure I was building the right code.

Comment 7

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

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

https://reviewboard.mozilla.org/r/154564/#review159644

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21243fe91b8f
Use an optimized build of the 7-zip SFX stub. r=rstrong

Comment 9

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

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

Approval Request Comment

[Feature/Bug causing the regression]:
Bug 1361326

[User impact if declined]:
Slower Firefox installations on Windows, and possible install timeouts when using the stub installer.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Not yet, but I've verified it manually on a local build.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
No actual code changes, just changed compiler settings to re-enable optimizations on a binary that used to have them until bug 1361326 regressed that.

[String changes made/needed]:
None

Comment 10

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

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
I'm including this request for completeness because this bug is for a regression caused by bug 1361326, which was uplifted to ESR, but this patch is not strictly necessary for ESR. The regression can cause the stub installer to fail, but stub installers are not built on ESR, so the only impact of the regression is that extracting the full installer takes longer than it should.

User impact if declined:
Performance regression in Windows Firefox installer, specifically during the extraction phase at the very beginning.

Fix Landed on Version:
56 nightly, uplift to 55 beta has been requested

Risk to taking this patch (and alternatives if risky):
Very low; there are no changes to code, only to compiler settings that an in-tree binary was built with, and only to revert an accidental change to them made in bug 1361326.

String or UUID changes made by this patch:
None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 11

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

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

Approval Request Comment
See beta request in comment 9. This doesn't merit its own point release, but it is causing stub installer failures, so getting it included in any further 54 point releases would be great.

Taking this specific patch would mean that bug 1366763, the new installer icon, comes along for the ride. If that's not acceptable, I can prepare a different patch without that.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21243fe91b8f

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

fix windows installer regression, beta55+, esr52+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6a8f9e8bcabd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Just to confirm, you're OK with taking the new installer icon for ESR52 as well (comment 10)? Or should Matt make a branch-specific version?

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

I missed comment 11 about the icon change.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

If this prevents a perf regression during the extraction phase of the windows installer, it makes sense to take it in ESR52. Given that the risk is very low, let's uplift to ESR52.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

No need to track it since the fix is ready/landed.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

So we're good with the icon change on ESR52 then? :)

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Matt Howell [:mhowell] from comment #9)
> [Is this code covered by automated tests?]:
> No
> 
> [Has the fix been verified in Nightly?]:
> Not yet, but I've verified it manually on a local build.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No

Setting qe-verify- based on Matt's assessment on manual testing needs.

Comment 21

[Gerry Chang [:gchang]]

Track 54+ for now and see if this impact users a lot.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> So we're good with the icon change on ESR52 then? :)

Yes. Please go ahead and land it in ESR52.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/b3a2ee816410

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

We are only one week from 55 RC build. Release54-.