1558306 - Load L10nRegistry.jsm and Fluent.jsm when the first content process is created

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

In bug 1555662, a few items were added to the browser_startup.js whitelist here:

https://searchfox.org/mozilla-central/rev/ee806041c6f76cc33aa3c9869107ca87cb3de371/browser/base/content/test/performance/browser_startup.js#38-39

The browser_startup.js whitelist lists the various modules that are expected to load during various start-up phases. It seems as if L10nRegistry.jsm and Fluent.jsm are now loading at the earliest phase, during profile selection.

Speaking with gandalf, it seems that it really only makes sense to load those two modules when the first Fluent-localized document is loaded. So something has gone a little awry here, and we should figure out why these are loaded so early.

Presuming that these two modules don't need to be loaded so early during start-up, we should get that fixed, and then remove these items from the whitelist.

Comment 1

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

Here's the relevant chunk from the original test failure for browser_startup.js:

[task 2019-06-07T23:43:49.197Z] 23:43:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup.js | should have no unexpected modules loaded before profile selection - Got 1, expected 0
[task 2019-06-07T23:43:49.198Z] 23:43:49     INFO - Stack trace:
[task 2019-06-07T23:43:49.199Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:test_is:1324
[task 2019-06-07T23:43:49.200Z] 23:43:49     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup.js:null:208
[task 2019-06-07T23:43:49.201Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1115
[task 2019-06-07T23:43:49.203Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1143
[task 2019-06-07T23:43:49.203Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1004
[task 2019-06-07T23:43:49.204Z] 23:43:49     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-06-07T23:43:49.205Z] 23:43:49     INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-06-07T23:43:49.208Z] 23:43:49     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup.js | unexpected modules: resource://gre/modules/Fluent.jsm - 
[task 2019-06-07T23:43:49.209Z] 23:43:49     INFO - Stack trace:
[task 2019-06-07T23:43:49.210Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:test_ok:1313
[task 2019-06-07T23:43:49.211Z] 23:43:49     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup.js:null:211
[task 2019-06-07T23:43:49.213Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1115
[task 2019-06-07T23:43:49.214Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1143
[task 2019-06-07T23:43:49.215Z] 23:43:49     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1004
[task 2019-06-07T23:43:49.216Z] 23:43:49     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-06-07T23:43:49.217Z] 23:43:49     INFO - 0 <TOP LEVEL> ["resource://gre/modules/L10nRegistry.jsm":5:53]
[task 2019-06-07T23:43:49.219Z] 23:43:49     INFO - 1 observe() ["resource://gre/modules/MainProcessSingleton.jsm":59:18]
[task 2019-06-07T23:43:49.220Z] 23:43:49     INFO - 

It looks like the L10nRegistry.jsm is being loaded at the app-startup observer notification here.

L10nRegistry.jsm loads Fluent.jsm here.

So that solves the mystery of how this is happening. What we need to know now is why this is necessary to occur at app-startup.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah! That makes so much sense. Thank you Mike!

So, the issue here is that in theory it can happen that parent process doesn't have any need to load L10nRegistry, but content process does.

My original patch made the content process L10nRegistry load its l10n-registry categories on its own, and then sync with parent if parent populated the sharedData cache [0]. This allowed us to happily skip loading L10nRegistry on parent at all.

Kris didn't like that we loaded categories both on parent and content and asked me to add this line to trigger L10nRegistry on parent before any content process can be loaded, so that parent populates the sharedData.

I can see one of the three solutions here:

a) bring back the full laziness and load categories in content process
b) delay loading L10nRegistry in parent process till later (BrowserGlue?) but ensure its loaded before content processes are loaded and may want it
c) accept that l10nregistry is on the early startup path.

I understand that mconley and florian prefer not to go with (c). Kris - can you recommend between (a) and (b)? And if (b), where is the right place in the startup to load it?

[0] https://searchfox.org/mozilla-central/source/intl/l10n/L10nRegistry.jsm#256

Comment 3

[Kris Maglione [:kmag]]

At some point, we're going to need Fluent on the startup path from outside a document. That means having at least the L10nRegistry loaded as soon as possible. We also ideally need it loaded and initialized before the first content process starts, or we'll need to broadcast its sharedData changes, rather than just having them available immediately at content process startup. Which is obviously bad for performance.

We can, of course, get away with using lazy getters for Fluent.jsm, but lazy getters aren't free, and if we're going to be loading it on the startup path anyway, we're really better off just doing it eagerly.

Comment 4

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

(In reply to Kris Maglione [:kmag] from comment #3)

We also ideally need it loaded and initialized before the first content process starts, or we'll need to broadcast its sharedData changes, rather than just having them available immediately at content process startup. Which is obviously bad for performance.

You're right, that does sound bad for performance. Would the ipc:content-created perhaps be a better observer notification to load it at then? I believe that'll fire before the content process has any opportunity to try to talk to Fluent.

At some point, we're going to need Fluent on the startup path from outside a document. That means having at least the L10nRegistry loaded as soon as possible.

I realize that ipc:content-created doesn't get us Fluent loaded as soon as possible, but perhaps it's soon enough for now?

We can, of course, get away with using lazy getters for Fluent.jsm, but lazy getters aren't free, and if we're going to be loading it on the startup path anyway, we're really better off just doing it eagerly.

I agree that lazy getters that we know we're about to call into are wasted overhead.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #4)

I realize that ipc:content-created doesn't get us Fluent loaded as soon as possible, but perhaps it's soon enough for now?

I think that would be slightly too late, though I'm not 100% sure. The sharedData mapping is one of the first things we send to new content processes, and I think it happens well before we notify that observer. And, after checking, yeah, it does. It happens from InitInternal and the observer notification happens in Init:

https://searchfox.org/mozilla-central/rev/0da35261b6789eec65476dbdd4913df6e235af6d/dom/ipc/ContentParent.cpp#2156,2173

That said, I don't think there's anything stopping us from adding a new observer notification that fires just before we create the first content process, and I think that initializing the L10nRegistry from there should be fine. If anything needs to access it from the parent process before then, it should be automatically initialized anyway.

Comment 6

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

(In reply to Kris Maglione [:kmag] from comment #5)

That said, I don't think there's anything stopping us from adding a new observer notification that fires just before we create the first content process, and I think that initializing the L10nRegistry from there should be fine. If anything needs to access it from the parent process before then, it should be automatically initialized anyway.

That sounds like a great idea. Let's call it ipc:first-content-created, for lack of creativity.

So I guess the plan is then to:

1. Add a static boolean or something that remembers whether or not we've ever created a content process before.
2. If we haven't set that value to true, fire the observer notification around here, and then set that value to true.
3. Update the MainProcessSingleton to use that new observer notification here and here.
4. Remove the L10nRegistry.jsm and Fluent.jsm entries from the whitelist, and push to try
5. Ensure that the browser_startup.js test continues to pass.

gandalf, you mentioned in IRC that you'd be happy to implement a decision here. Is it okay if I assign this to you?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yes!

Should the book be on ContnetProcess? Who'll be my reviewer?

Comment 8

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

The general solution makes sense for DOM :: Content Processes, but ultimately, this work is to serve moving the localization code from that early point in startup, so I suggest leaving it here for now.

I'll happily review.

Thanks!

Comment 9

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

From #fx-team in IRC:

<gandalf> mconley: Can you rephrase your response about the location of the bool? What do you mean by "here"?

Around here's where I suggest defining the boolean, perhaps calling it sCreatedFirstContentProcess, and initializing it to false.

Here's approximately where I'd check that boolean, and if it's false, I'd fire the observer notification.

And then in the same conditional block, I'd set the boolean to true. So it'd be something like:

if (!sCreatedFirstContentProcess) {
  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
  if (obs) {
    obs->NotifyObservers(nullptr, "ipc:first-content-created", nullptr);
  }
  sCreatedFirstContentProcess = true;
}

Does that give you enough information?

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1558306 - Switch L10nRegistry initialization to happen right before the first content process gets created.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1558306 - Clean up app-startup category/topics for readability.

Depends on D34572

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7832d4ed2c61
Switch L10nRegistry initialization to happen right before the first content process gets created. r=mconley
https://hg.mozilla.org/integration/autoland/rev/a09644bb0e6e
Clean up app-startup category/topics for readability. r=mconley

Comment 13

[Andreea Pavel [:apavel]]

Backed out on request

Backout: https://hg.mozilla.org/integration/autoland/rev/141838e0d7b57f34efd9f4c4131326954289931c

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/912ea3c956b3
Switch L10nRegistry initialization to happen right before the first content process gets created. r=mconley
https://hg.mozilla.org/integration/autoland/rev/12409096d018
Clean up app-startup category/topics for readability. r=mconley

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/912ea3c956b3
https://hg.mozilla.org/mozilla-central/rev/12409096d018