Closed Bug 1456291 Opened 6 years ago Closed 6 years ago

Avoid loading nsBlocklistService.js at all during statup

Categories

(Toolkit :: Blocklist Implementation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 file)

TelemetryEnvironment.jsm currently forces a load of nsBlocklistService.jsm during startup just to check the isLoaded flag. That adds a measurable amount of overhead to startup.

We should just skip that check when we know we're initializing telemetry during startup.
Comment on attachment 8970369 [details]
Bug 1456291: Avoid loading the blocklist service before UI is interactive.

https://reviewboard.mozilla.org/r/239158/#review244822

::: browser/base/content/test/performance/browser_startup.js:91
(Diff revision 1)
>    // interacting with the first browser window.
>    "before handling user events": {blacklist: {
>      components: new Set([
>        "PageIconProtocolHandler.js",
>        "PlacesCategoriesStarter.js",
> +      "nsIBlocklistService.js",

Both here and in the other place, `nsBlocklistService.js` (without the 'I').

Without fixing the plugins use as well, I don't think this will pass on automation. In fact, even with that I'm not convinced this won't break because of add-on manager use (which is async, but will still trigger a load, if I had to guess off-hand at 11.30pm at night, so take with grains of salt etc.)
Attachment #8970369 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #2)
> Comment on attachment 8970369 [details]
> Bug 1456291: Avoid loading the blocklist service before UI is interactive.
> 
> https://reviewboard.mozilla.org/r/239158/#review244822
> 
> ::: browser/base/content/test/performance/browser_startup.js:91
> (Diff revision 1)
> >    // interacting with the first browser window.
> >    "before handling user events": {blacklist: {
> >      components: new Set([
> >        "PageIconProtocolHandler.js",
> >        "PlacesCategoriesStarter.js",
> > +      "nsIBlocklistService.js",
> 
> Both here and in the other place, `nsBlocklistService.js` (without the 'I').
> 
> Without fixing the plugins use as well, I don't think this will pass on
> automation. In fact, even with that I'm not convinced this won't break
> because of add-on manager use (which is async, but will still trigger a
> load, if I had to guess off-hand at 11.30pm at night, so take with grains of
> salt etc.)

I tested this with manual logging, and at least locally, it does not load before first paint.

The add-on manager uses aren't an issue, since we cache the blocklist value, and only check it again when either the blocklist or an add-on updates. More or less the same thing goes for plugins.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> The add-on manager uses aren't an issue, since we cache the blocklist value,
> and only check it again when either the blocklist or an add-on updates.

Ah right. Is this still true even with the add-ons we install specifically in order to run tests in automation? :-)

> More
> or less the same thing goes for plugins.

In general, I'm a bit sad about this, in the sense that I think it'd be good to have tests also for the cases where this happens, but that's hard/impossible to do given current infrastructure.
(In reply to :Gijs (he/him) from comment #4)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #3)
> > The add-on manager uses aren't an issue, since we cache the blocklist value,
> > and only check it again when either the blocklist or an add-on updates.
> 
> Ah right. Is this still true even with the add-ons we install specifically
> in order to run tests in automation? :-)

That depends on how we install them. But, annoyingly, in this case no :/ we wind up doing a startup scan for new extensions, and checking the blocklist state then.

Which means that the startup that we're testing in this test is pretty different from a usual startup... We should make it a talos job.

I'll just remove these checks and keep the other changes.
Comment on attachment 8970369 [details]
Bug 1456291: Avoid loading the blocklist service before UI is interactive.

https://reviewboard.mozilla.org/r/239158/#review244834

r=me, though for users who have plugins I guess this won't help until bug 1456171 also ships.
Attachment #8970369 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa2a1d2d41e69c953e0b307e46b885271cd6091
Bug 1456291: Avoid loading the blocklist service before UI is interactive. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/1fa2a1d2d41e
https://hg.mozilla.org/mozilla-central/rev/5579f92920c3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)
> (In reply to :Gijs (he/him) from comment #4)
> > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> > blocked) from comment #3)
> > > The add-on manager uses aren't an issue, since we cache the blocklist value,
> > > and only check it again when either the blocklist or an add-on updates.
> > 
> > Ah right. Is this still true even with the add-ons we install specifically
> > in order to run tests in automation? :-)
> 
> That depends on how we install them. But, annoyingly, in this case no :/ we
> wind up doing a startup scan for new extensions, and checking the blocklist
> state then.
> 
> Which means that the startup that we're testing in this test is pretty
> different from a usual startup... We should make it a talos job.

Could we install the add-on used by the test harness later so that it doesn't interfere with startup? This add-on install during startup is also stopping us from preventing regressions in how early NSS is initialized (it's currently initialized after the browser UI is painted for normal startup, but during early startup in our test infrastructure due to the add-on install).
(In reply to Florian Quèze [:florian] from comment #11)
> Could we install the add-on used by the test harness later so that it
> doesn't interfere with startup? This add-on install during startup is also
> stopping us from preventing regressions in how early NSS is initialized
> (it's currently initialized after the browser UI is painted for normal
> startup, but during early startup in our test infrastructure due to the
> add-on install).

There are a few problems, really.

One is that we always start with a new profile, which causes us to do a full scan of all install locations, and re-parse all add-on manifests in them. That, we should be able to fix by warming up a profile before we run any tests. But we'd probably have to cache it, and reset it to that initial state before we start each new Firefox instance, or we might wind up making mochitest runs much slower.

Another is that we set a bunch of non-default prefs, like extensions.autoDisableScopes and extensions.installDistroAddons, which change startup behavior.

The easiest way to fix both of them would probably to just make this a marionette test. Those should be able to warmup their own profile, and change the startup pref overrides that don't make sense for that test.
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.