568917 - urlclassifier should be lazily init'd on first use, not just 2s after startup

Status: RESOLVED WONTFIX

(Toolkit :: Safe Browsing, defect, P5)

Description

[Johnathan Nightingale [:johnath]]

Right now, the urlclassifier is initialized on a 2s timeout fired on load of the browser. Since urlclassifier init is often the first use of crypto code, this also necessitates psm and nss init, which is a costly thing to be doing just as the user starts to want to use their browser.

This should probably be init'd off of the idle service, or lazily at first use, rather than on a fixed timer.

Comment 1

[Dietrich Ayala (:dietrich)]

wouldn't urlclassifier be initialized anyway when safebrowsing checks the url of the first content page loaded?

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> wouldn't urlclassifier be initialized anyway when safebrowsing checks the url
> of the first content page loaded?

Yes.

Comment 3

[Justin Dolske [:Dolske]]

This almost got fixed in bug 769960, then undid it in bug 778855 due to a perf regression.

_delayedStartup() in browser.js currently has:

1378 #ifdef MOZ_SAFE_BROWSING
1379     // Bug 778855 - Perf regression if we do this here. To be addressed in bug 779008.
1380     setTimeout(function() { SafeBrowsing.init(); }, 2000);
1381 #endif

Leaving _this_ bug open because bug 779008 will remove the timeout, but ultimately this should be out of the startup path entirely.

Comment 4

[François Marier [:francois]]

Given comment 2, I don't see how lazily initializing Safe Browsing would change anything. Marking as WONTFIX, but feel free to reopen if there's something I'm missing.

Comment 5

[Dietrich Ayala (:dietrich)]

Here's the thing with startup (at least the last time I profiled it, which was some time ago):

You get non-deterministic results from changing around the order or modifying the set of services that start when the first browser window loads. For example: Sometimes removing a service will *hurt* window load time, because a thing that had previously loaded after our measurement was taken, has now moved up in the queue and initializes before the measurement, and takes longer than the thing you removed. Followed by inhuman howling and much gnashing of teeth.

So comment 2 *might* be correct. And if it is, it *might* impact performance in a way that is counter to our assumptions.

I took a quick look in _delayedStartup(), and didn't see the urlclassifier there. But I didn't look very hard. Some of these services now load by observing the startup topic, so don't load the same way anymore. Which means this bug might be irrelevant as originally filed, or the change might need be done in a different place.

However, we are apparently still playing games to avoid NSS/PSM initialization:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1346

I wonder if we have telemetry around when NSS/PSM *actually* initialize in real life... For example, seems as if the default loading page at startup was https then NSS would initialize regardless, and that might be a more likely scenario these days.

Comment 6

[Dietrich Ayala (:dietrich)]

Tim, do you know anything about what triggers NSS/PSM initialization and whether we have telemetry that tracks at what point in the startup timeline it happens?

(Kinda seems like we just should force initialize it and call it game over. If our mission to encrypt the world is successful, that's where we'll end up anyway.)

Comment 7

[François Marier [:francois]]

(In reply to Dietrich Ayala (:dietrich) from comment #5)
> I wonder if we have telemetry around when NSS/PSM *actually* initialize in
> real life... For example, seems as if the default loading page at startup
> was https then NSS would initialize regardless, and that might be a more
> likely scenario these days.

It seems like if you take into account Safe Browsing, Tracking Protection, snipets, tiles and the new remote about:newtab, NSS initialization will happen pretty early on.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Tim, do you know anything about what triggers NSS/PSM initialization and
> whether we have telemetry that tracks at what point in the startup timeline
> it happens?

I don't know what exactly triggers it. I suspect David knows more?

> (Kinda seems like we just should force initialize it and call it game over.
> If our mission to encrypt the world is successful, that's where we'll end up
> anyway.)

We might already do that anyway? Should check this.

(In reply to François Marier [:francois] from comment #7)
> (In reply to Dietrich Ayala (:dietrich) from comment #5)
> > I wonder if we have telemetry around when NSS/PSM *actually* initialize in
> > real life... For example, seems as if the default loading page at startup
> > was https then NSS would initialize regardless, and that might be a more
> > likely scenario these days.
> 
> It seems like if you take into account Safe Browsing, Tracking Protection,
> snipets, tiles and the new remote about:newtab, NSS initialization will
> happen pretty early on.

Sounds about right.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Currently PSM/NSS is initialized when observers of "addons-startup" are notified:

0 <TOP LEVEL> ["resource://gre/modules/addons/XPIProvider.jsm":1647]
1 AddonManagerInternal.startup() ["resource://gre/modules/AddonManager.jsm":892]
    this = [object Object]
2 this.AddonManagerPrivate.startup() ["resource://gre/modules/AddonManager.jsm":2774]
    this = [object Object]
3 amManager.prototype.observe(aSubject = null, aTopic = "addons-startup", aData = null) ["file:///home/keeler/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/components/addonManager.js":57]
    this = [object Object]

I believe this is unintentional and perhaps unwanted. That top line is https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1647 :

let gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
              .getService(Ci.nsIX509CertDB);

Some interfaces are backed by implementations that know they require NSS, so they initialize PSM/NSS when they are instantiated. If something like that doesn't happen, though, PSM/NSS is initialized by necko via net_EnsurePSMInit: https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/netwerk/base/nsNetUtil.cpp#2131

It would save us a great many headaches if PSM/NSS were initialized in a deterministic, consistent manner rather than "whenever", which is what we have now.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(Whoops, didn't answer all of Dietrich's questions.)

(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Tim, do you know anything about what triggers NSS/PSM initialization and
> whether we have telemetry that tracks at what point in the startup timeline
> it happens?

We don't have telemetry on that.