Closed Bug 1625181 Opened 4 months ago Closed 4 months ago

Add-on manager sometimes blocks startup on blocklist checks, leading to slow startups

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox76 --- wontfix
firefox77 --- fixed

People

(Reporter: Gijs, Assigned: aswan)

References

(Regressed 1 open bug)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file, 1 obsolete file)

Florian got this profile out of a try build earlier today: https://bit.ly/2UB7vzW

This is a profile captured on try for the browser_startup_mainthreadio.js test.

It seems we hit syncLoadManifest and then end up blocking startup on loading the blocklist, which means we block on loading the dump through a worker with a bunch of IPC overhead etc.

It's not clear exactly what trips this, though the suspicion is it's the sideloaded mochitest add-ons. I don't really understand why we wouldn't hit this for builtin or system add-ons, and/or if this would affect people with crapw... I mean important security software that sideloads things, or something.

Either way, ideally we shouldn't need to do this. blocklist v3 will improve things here, but it'll still not be a good idea to block on things because we'll still need to load the data for the blocklist off disk, and even with the new infrastructure that's non-trivial IO - doing it away from the mainthread (as RS does here) doesn't help if we block the main thread on the IO's completion...

:aswan said he was interested in taking this.

(In reply to :Gijs (he/him) from comment #0)

It's not clear exactly what trips this, though the suspicion is it's the sideloaded mochitest add-ons. I don't really understand why we wouldn't hit this for builtin or system add-ons, and/or if this would affect people with crapw... I mean important security software that sideloads things, or something.

I'm pretty sure we do hit this for builtin/system addons (at least if I stick a dump() statement inside ExtensionBlocklistRS.ensureInitialized, I see it very early during startup in a new profile).

For sideloads (outside of testing), this is much more of a corner case now that we don't handle new sideloads (bug 1602839)

Priority: -- → P2
Whiteboard: [fxperf]

We don't put system addons on the blocklist and we don't sync them, so
there's no need to compute blocklist state or a sync guid for them. The
immediate motivation for this is to avoid initializing the blocklist
early in startup for the initial startup in a new profile or after a
browser upgrade when we're installing/udpating system and builtin addons.

Keywords: leave-open

Comment on attachment 9138114 [details]
Bug 1625181 Skip blocklist checks for test extensions

Revision D69604 was moved to bug 1627336. Setting attachment 9138114 [details] to obsolete.

Attachment #9138114 - Attachment is obsolete: true

Spun off the special-powers and mochitest handling in bug 1627336

Keywords: leave-open
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a62adf9c09cc
Skip over unneeded work when installing system/builtin addons r=zombie
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f509247a91c9
Backed out changeset a62adf9c09cc for turning bug 1591590 into near permafail.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla76 → ---
Whiteboard: [fxperf] → [fxperf:p2]
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bb8d93586f0
Skip over unneeded work when installing system/builtin addons r=zombie
Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

== Change summary for alert #25614 (as of Tue, 14 Apr 2020 04:07:19 GMT) ==

Improvements:

4% JS windows7-32-shippable opt 79,428,166.00 -> 76,347,768.84
4% JS windows7-32-shippable opt 79,488,059.57 -> 76,528,361.21
3% JS linux1804-64-shippable opt 106,011,801.24 -> 102,683,548.74
3% JS windows10-64-shippable opt 108,224,629.82 -> 104,876,607.98
3% JS linux1804-64-shippable-qr opt 106,485,300.74 -> 103,221,107.36
3% JS macosx1014-64-shippable opt 107,583,008.37 -> 104,711,332.15
2% Heap Unclassified windows10-64-shippable opt 51,926,572.41 -> 50,696,683.11
2% JS windows10-64-shippable-qr opt 107,769,884.68 -> 105,335,162.11
2% Explicit Memory windows7-32-shippable opt 280,596,308.09 -> 274,870,133.10

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25614

I think we now still do this work when a blocklist update comes in, because we ask the add-on manager for a list of all installed add-ons which (I think) includes these ones. Should we avoid updating blocklist states there, too?

Flags: needinfo?(andrew.swan)

(In reply to :Gijs (he/him) from comment #13)

I think we now still do this work when a blocklist update comes in, because we ask the add-on manager for a list of all installed add-ons which (I think) includes these ones. Should we avoid updating blocklist states there, too?

Yes, that's a good idea, filed bug 1633396

Flags: needinfo?(andrew.swan)
You need to log in before you can comment on or make changes to this bug.