Closed Bug 1375472 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Installer, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox-esr52 - fixed
firefox54 + wontfix
firefox55 - fixed
firefox56 --- fixed

People

(Reporter: RT, Assigned: mhowell)

References

Details

(Keywords: regression)

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.

Attachments

(1 file)

No description provided.
User Story: (updated)
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
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: win64-rollout
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
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.
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: nobody → mhowell
Status: NEW → ASSIGNED
OS: Unspecified → Windows
Hardware: Unspecified → All
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+
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
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?
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?
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?
https://hg.mozilla.org/mozilla-central/rev/21243fe91b8f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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+
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 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.
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.
(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 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-
You need to log in before you can comment on or make changes to this bug.