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

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

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.

Comment 1

[Andrew Swan [:aswan]]

(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)

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1625181 Skip over unneeded work when installing system/builtin addons r=zombie

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.

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1625181 Skip blocklist checks for test extensions

Comment 4

[Phabricator Automation]

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.

Comment 5

[Andrew Swan [:aswan]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a62adf9c09cc
Skip over unneeded work when installing system/builtin addons r=zombie

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a62adf9c09cc

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f509247a91c9
Backed out changeset a62adf9c09cc for turning bug 1591590 into near permafail.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/f509247a91c9

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bb8d93586f0
Skip over unneeded work when installing system/builtin addons r=zombie

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0bb8d93586f0

Comment 12

[Marian Raiciof [:marauder]]

== 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

Comment 13

[:Gijs (he/him)]

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?

Comment 14

[Andrew Swan [:aswan]]

(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