1300126 - Remove things from _delayedStartup that only need to happen once per session

Status: RESOLVED DUPLICATE of bug 1388145

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

I just noticed that there are some things in browser.js's _delayedStartup that probably don't belong there.

Some examples:

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1148

SafeBrowsing is a singleton, and repeated .init() calls on it just log an "already initialized" and then return. That seems useless.

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1249

I'm not sure we need to do this DownloadsCommon.initializeAllDataLinks here either...

We do this startup crash tracking stuff here per window as well: https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1311

And more:

The GMP installer...

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1321

This Telemetry stuff for video playback..

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1334

Monitoring slow add-ons...

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1354

Master Password Telemetry...

https://dxr.mozilla.org/mozilla-central/rev/d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1369


Like, I understand that we want these things, and we want to do them after the first window paints, but we're doing this _for every window_, not just the first one. A lot of these should probably go into the browser-delayed-startup-finished observer in nsBrowserGlue.js instead.

Comment 1

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #0)

> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1249
> 
> I'm not sure we need to do this DownloadsCommon.initializeAllDataLinks here
> either...

Here is a profile where it causes 138ms of UI jank during the startup jank: https://perfht.ml/2pBgv6O

This is despite being started off a 10s timer, but startup can take way more than 10s on a slow machine.

Comment 2

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> This is despite being started off a 10s timer, but startup can take way more
> than 10s on a slow machine.

Is there a way to delay this until we've done other startup tasks?

We should definitely restart downloads that were paused at the end of the previous session, and we should do that at some point near the beginning of the session. It doesn't need to happen immediately though. If there is any interaction with the Downloads button, we restart the downloads when that happens anyway.

Comment 3

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #2)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > This is despite being started off a 10s timer, but startup can take way more
> > than 10s on a slow machine.
> 
> Is there a way to delay this until we've done other startup tasks?

I'm hoping bug 1353206 will help us here.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Mike Conley (:mconley) from comment #0)
> The GMP installer...
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1321

Bug 1360864 moved this to a requestIdleCallback. We could change this to check for updates on startup only if we don't have any plugins installed, so that if the user immediately loads Netflix or tries to use WebRTC it's more likley to work. If we do have plugins installed do the update check while the browser under less load. Maybe via the nsIIdleService observer?


> This Telemetry stuff for video playback..
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1334

Oh goodness...

The video playback telemetry certainly doesn't need to be once per window. My intention was for it to be once on browser startup.

Additionally, this code actually causes us to check that we can create Windows system video and audio decoders, and to do that we need to load DLLs from disk, that is, we need to do disk I/O. Which we'll be doing on the main thread in the chrome process. 

So yes, this is likely a cause of jank.

I'll write a patch to move this off main thread, and do it once per startup instead of once per window.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

I filed Bug 1362212 to remove the video playback telemetry.

Comment 6

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #0)

> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1148
> 
> SafeBrowsing is a singleton, and repeated .init() calls on it just log an
> "already initialized" and then return. That seems useless.

Bug 1388145 fixed this.

> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1249
> 
> I'm not sure we need to do this DownloadsCommon.initializeAllDataLinks here
> either...

Bug 1388145 moved this to an idle task, but it's still done for every windows.

> We do this startup crash tracking stuff here per window as well:
> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1311

Bug 1388145 fixed this.

> Monitoring slow add-ons...
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1354

I can't find this code anymore, I think it has been removed.

> 
> Master Password Telemetry...
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1369

Bug 1388145 fixed this.


Felipe, do you see anything left to fix here? If not, let's dupe this to bug 1388145.

Comment 7

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > Monitoring slow add-ons...
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > d5f20820c80514476f596090292a5d77c4b41e3b/browser/base/content/browser.js#1354
> 
> I can't find this code anymore, I think it has been removed.

bug 1309946

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Yeah, I think we can dupe it and file two follow ups:

1 - I thought about moving DownloadsCommon.initializeAllDataLinks to nsBrowserGlue but it wasn't immediately clear to me if it could be done, so I avoided uncertain changes like that in Bug 1388145 

2 - The only other thing is that I feel (from a quick glance) that gXPInstallObserver doesn't need to be per-window, but that requires more investigation as part of another bug.