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

RESOLVED FIXED in Firefox -esr52

Status

()

Firefox
Installer
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: RT, Assigned: mhowell)

Tracking

({regression})

unspecified
Firefox 56
All
Windows
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52- fixed, firefox54+ wontfix, firefox55- fixed, firefox56 fixed)

Details

User Story

Since FF55 Beta was launched, we observed an install timeout increase that seems to relate to an install time increase. 
This does not seem related to the roll-out of 64-bit Firefox since 32 and 64 bit install times seem to have increased:
- 32 bit Firefox install time:https://sql.telemetry.mozilla.org/queries/4955/source#10147
- 64 bit Firefox install time:https://sql.telemetry.mozilla.org/queries/4953/source#10140
We may be able to increase the install success rate by increasing the install timeout. This bug is about (1) clarifying the source of the install time increase and (2) defining the right timeout value in case that's a suitable solution.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

a year ago
User Story: (updated)
Blocks: 1340936
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
(Reporter)

Updated

a year ago
User Story: (updated)
User Story: (updated)
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
Blocks: 1361326
Keywords: regression
Summary: Reexamine install timeout as a consequence of new 64 bit Firefox install default → install_time timeout regression as a consequence of DLL delay loading in bug 1361326
As this install timeout regression affects both 32- and 64-bit installs, it does not need to block our 64-bit rollout.
No longer blocks: 1340936
tracking-firefox55: --- → ?
Summary: install_time timeout regression as a consequence of DLL delay loading in bug 1361326 → Install timeout regression as a consequence of DLL delay loading from bug 1361326
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
OS: Unspecified → Windows
Hardware: Unspecified → All
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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 on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

https://reviewboard.mozilla.org/r/154564/#review159644
Attachment #8883636 - Flags: review?(robert.strong.bugs) → review+

Comment 8

a year ago
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21243fe91b8f
Use an optimized build of the 7-zip SFX stub. r=rstrong
(Assignee)

Comment 9

a year ago
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
Attachment #8883636 - Flags: approval-mozilla-beta?
(Assignee)

Comment 10

a year ago
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.
Attachment #8883636 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 11

a year ago
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.
Attachment #8883636 - Flags: approval-mozilla-release?

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21243fe91b8f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment on attachment 8883636 [details]
Bug 1375472 - Use an optimized build of the 7-zip SFX stub.

fix windows installer regression, beta55+, esr52+
Attachment #8883636 - Flags: approval-mozilla-esr52?
Attachment #8883636 - Flags: approval-mozilla-esr52+
Attachment #8883636 - Flags: approval-mozilla-beta?
Attachment #8883636 - Flags: approval-mozilla-beta+

Comment 14

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6a8f9e8bcabd
status-firefox55: affected → fixed
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?
status-firefox54: unaffected → affected
status-firefox-esr52: --- → affected
tracking-firefox54: --- → ?
tracking-firefox-esr52: --- → ?
Flags: needinfo?(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.
Flags: needinfo?(jcristau)
Attachment #8883636 - Flags: approval-mozilla-esr52+ → approval-mozilla-esr52?
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.
Attachment #8883636 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
No need to track it since the fix is ready/landed.
tracking-firefox55: ? → -
tracking-firefox-esr52: ? → -
So we're good with the icon change on ESR52 then? :)
Flags: needinfo?(rkothari)
(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.
Flags: qe-verify-
Track 54+ for now and see if this impact users a lot.
tracking-firefox54: ? → +
(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.
Flags: needinfo?(rkothari)

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/b3a2ee816410
status-firefox-esr52: affected → fixed

Comment 24

11 months ago
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-.
Attachment #8883636 - Flags: approval-mozilla-release? → approval-mozilla-release-

Updated

11 months ago
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.