FxAccounts modules loaded too early at startup

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: eoger)

Tracking

(Blocks 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Splitting out from bug 1369436:

> Slightly off-topic for this bug, but there are currently 4 fx account
> modules loaded after first paint but before we handle user events:
> FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm
> FxAccountsStorage.jsm 
> Could these also be loaded lazily?
Priority: -- → P3
Here is a startup profile showing it: https://perfht.ml/2r7dyLE

This is started from _delayedStartup in browser.js (so I guess http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/browser/base/content/browser.js#1639)

Can we skip most of this when sync has never been configured (no username)?
Priority: P3 → --
Also visible in profiles of creating new windows: http://bit.ly/2rzrLBY (this is on a slow netbook)

I doubt this.EnsureFxAccountsWebChannel is needed for each new window; should some of this be moved from browser.js _delayedStartup to nsBrowserGlue.js?
Assignee

Comment 3

2 years ago
Florian would moving the gSync.init() call [0] later in _delayedStartup (or even wrapping it in a requestIdleCallback) work for you?

[0] http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1638
(In reply to Edouard Oger [:eoger] from comment #3)
> Florian would moving the gSync.init() call [0] later in _delayedStartup (or
> even wrapping it in a requestIdleCallback) work for you?
> 
> [0]
> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#1638

It's already in _delayedStartup, so moving it later in _delayedStartup wouldn't improve anything for performance (all of that function will run before we return to the event loop). Calling it from an idle callback would certainly be an improvement.

I still think we should completely skip this when we have no sync username in the preferences.
Assignee: nobody → eoger
Priority: -- → P2
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
5 sec max timeout okay for you Mark?
Flags: needinfo?(markh)
(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> I still think we should completely skip this when we have no sync username
> in the preferences.

So is there a reason why this isn't possible?

I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93
Flags: needinfo?(eoger)
Assignee

Comment 8

2 years ago
Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at the moment. We would need to refactor it if we wanted to make this lazy.
In any case we still require FxAccounts.jsm in UIState.jsm anyway.
Flags: needinfo?(eoger)
Assignee

Comment 9

2 years ago
> I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93

This makes the test fail :/
Commenting |gSync.init()| makes the test pass, so we can assume no one else is initializing these modules.
(In reply to Edouard Oger [:eoger] from comment #9)
> > I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93
> 
> This makes the test fail :/
> Commenting |gSync.init()| makes the test pass, so we can assume no one else
> is initializing these modules.

That's likely because the test also uses requestIdleCallback, and these 2 calls compete. I guess then we can't test this until loading all of this becomes lazy.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

https://reviewboard.mozilla.org/r/154256/#review159362

r+ because this is a net improvement over the current situation, but I think we can (and should) do better.

(In reply to Edouard Oger [:eoger] from comment #8)
> Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at
> the moment.

Needed by what for which purpose?

> We would need to refactor it if we wanted to make this lazy.

Seems like a reasonable thing to do ;-).

> In any case we still require FxAccounts.jsm in UIState.jsm anyway.

So... can we ensure none of the sync code ever gets loaded unless either sync is already configured, or the user touches the sync configuration UI? Even the browser-sync.js file probably wants to become lazy loaded.
Attachment #8883367 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
Assignee

Comment 13

2 years ago
How about this? This avoids loading FxAccounts.jsm in UIState.jsm if no username is defined.

> (In reply to Edouard Oger [:eoger] from comment #8)
> > Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at
> > the moment.
> 
> Needed by what for which purpose?

Currently we have no way to open a WebChannel instance until a web page actually needs it. (See bug 1358326)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

https://reviewboard.mozilla.org/r/154256/#review159392
Attachment #8883367 - Flags: review?(markh) → review+
(In reply to Edouard Oger [:eoger] from comment #6)
> 5 sec max timeout okay for you Mark?

Sounds fine given it is supposed to be the max time, thanks.
Flags: needinfo?(markh)

Comment 16

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dec7cb09336e
Sync UI startup performance improvements. r=florian,markh
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89fda6fb1946
Sync UI startup performance improvements. r=florian,markh

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89fda6fb1946
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee

Updated

2 years ago
Flags: needinfo?(eoger)
The changes made to browser.js here covered the normal browser window case, but didn't cover the gBrowserInit.nonBrowserWindowDelayedStartup case; I'm applying the same change there in bug 1381853 attachment 8888419 [details] [diff] [review].

Updated

2 years ago
Product: Core → Firefox
Target Milestone: mozilla56 → Firefox 56
You need to log in before you can comment on or make changes to this bug.