Closed Bug 1579367 Opened 1 year ago Closed 3 months ago

User prefs are loaded after creating the first JSContext in the parent process

Categories

(Core :: XPConnect, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The prefs we load here always have their default values in the parent process, because user pref overrides are loaded later on, in Preferences::InitializeUserPrefs(). Bug 1578350 is adding a similar pref affected by this.

The JS engine wants to move to process-wide prefs, for simplicity, and require these to be set early on. However XPConnect is initialized very early on before the user prefs are loaded.

I don't know what to do about this. Loading user prefs much later than all.js and similar seems like a pretty big footgun, so maybe that's the thing to fix somehow.

This also removes some dead code for cooperative JSContexts (the
JS engine support for this was removed last year in bug 1434211).

Comment on attachment 9090990 [details]
Bug 1579367 - WIP: Initialize XPCJSContext after loading user prefs

How do you feel about this approach? The patch isn't finished yet but the browser starts up and we now get the correct prefs in the parent process too.

Attachment #9090990 - Flags: feedback?(kmaglione+bmo)

Hey Kris, after our IRC conversation yesterday I pushed this to Try. The ts_paint and ts_paint_webext numbers don't seem to be affected much. What do you think?

Flags: needinfo?(kmaglione+bmo)

OK, then let's give it a shot, I guess. We have telemetry for how often we need to wait for a script from the preloader or re-decode it on the main thread, so we should be able to see whether it causes regressions on the wild with pretty high confidence.

Flags: needinfo?(kmaglione+bmo)
Depends on: 1591923
Attachment #9090990 - Attachment description: Bug 1579367 - WIP: Load JS prefs after reading user prefs. → Bug 1579367 - WIP: Initialize XPCJSContext after loading user prefs
Blocks: 1592523

This way we get the correct values for start-up prefs in the parent process.

Depends on D51060

Attachment #9105151 - Attachment description: Bug 1579367 part 2 - Initialize XPCJSContext explicitly, after loading user prefs. r?kmag! → Bug 1579367 - Initialize XPCJSContext explicitly, after loading user prefs. r?kmag!
Attachment #9105149 - Attachment is obsolete: true
Attachment #9090990 - Attachment is obsolete: true
Attachment #9090990 - Flags: feedback?(kmaglione+bmo)
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/955256297d6d
Initialize XPCJSContext explicitly, after loading user prefs. r=kmag
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

That's causing a startup crash when JS components are used to implement directory providers because they are instantiated by https://hg.mozilla.org/mozilla-central/file/d73cfe3a04e90b5dff64f6d4e3c3a9183aec4ec5/xpcom/build/XPCOMInit.cpp#l459 while NS_InitXPCOM is called with aInitJSContext == false.

I'm not sure what the right solution for that is...

Regressions: 1594404

Backed out changeset 955256297d6d (bug 1579367) for causing a top crash in Bug 1594404. a=backout

As requested by pascalc in #sheriffs.

https://hg.mozilla.org/mozilla-central/rev/e163e84188dd95d8ad3909bbde2c52f3c4d2c7a8

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla72 → ---

(In reply to [:fabrice] Fabrice Desré from comment #9)

That's causing a startup crash when JS components are used to implement directory providers because they are instantiated by https://hg.mozilla.org/mozilla-central/file/d73cfe3a04e90b5dff64f6d4e3c3a9183aec4ec5/xpcom/build/XPCOMInit.cpp#l459 while NS_InitXPCOM is called with aInitJSContext == false.

I'm not sure what the right solution for that is...

How did you find out about this? Is it something we need to support still, or can we do this later in startup? It's been frustrating to have these features we apparently care about but don't have any tests for..

(In reply to Jan de Mooij [:jandem] from comment #11)

How did you find out about this? Is it something we need to support still, or can we do this later in startup? It's been frustrating to have these features we apparently care about but don't have any tests for..

I'm working on a gecko based product that happens to have a such a js component. If the solution is to rewrite it in c++, so be it...

Any thoughts on the JS-implemented directory providers, Kris?

Flags: needinfo?(kmaglione+bmo)

(In reply to Jan de Mooij [:jandem] from comment #13)

Any thoughts on the JS-implemented directory providers, Kris?

I don't see any reason to support that.

Flags: needinfo?(kmaglione+bmo)

This seems to fix the Valgrind issue in bug 1582512, so I'll try relanding this soon..

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3fd24f5f0a7
Initialize XPCJSContext explicitly, after loading user prefs. r=kmag
Status: REOPENED → RESOLVED
Closed: 4 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Regressions: 1599569
Regressions: 1607703
You need to log in before you can comment on or make changes to this bug.