Closed Bug 1398895 Opened 7 years ago Closed 7 years ago

JSM sharing pref not used when manually set

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mccr8, Assigned: kmag)

References

Details

Attachments

(2 files)

Kris noticed this, and I have the same thing happening for me: even with the jsloader sharing pref set to true, there is no sharing in the parent process.

"I think there might just be some uncertainty about the timing of the pref service initialization. We can probably just force it when we check the pref, if necessary. And maybe do what I suggested with the pre-loader compilation scope so it happens a bit later"
It works when the pref is set to true in all.js, but not when you manually set it in about:config.
Summary: JSM sharing pref not always used in main process → JSM sharing pref not used when manually set
Assignee: nobody → kmaglione+bmo
Comment on attachment 8906768 [details]
Bug 1398895: Part 2 - Ensure component loader initialization after user prefs load.

https://reviewboard.mozilla.org/r/178492/#review183508

Thanks for fixing this!
Attachment #8906768 - Flags: review?(continuation) → review+
Attachment #8906767 - Flags: review?(dtownsend) → review?(florian)
Comment on attachment 8906767 [details]
Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.

https://reviewboard.mozilla.org/r/178490/#review184088

(In reply to Andrew McCreight [:mccr8] from comment #1)
> It works when the pref is set to true in all.js, but not when you manually
> set it in about:config.

So isn't this good enough for the purpose of being able to turn off the "shared global for JSMs" behavior in beta if needed? I don't think users need to mess with that pref.

If this is only for testing purpose (saying this because I had a quick look at test_loader_global_sharing.py), could a solution similar to what was done in bug 1328830 for mochitests be used instead of user prefs?

I've always assumed app-startup was a notification fired for code that wants to run before the profile has been selected. I don't think so close to the 57 merge is a great time to change this behavior.

::: toolkit/xre/nsAppRunner.cpp:4470
(Diff revision 1)
>        // renamed, and potentially that the selected/default profile changed.
>        mProfileSvc->Flush();
>      }
>    }
>  
> +  // Do early profile startup before notifying startup observers so user

The profile migrator will run before this. It's a JS component importing JSM files: http://searchfox.org/mozilla-central/source/browser/components/migration/ProfileMigrator.js
Wasn't the goal of this patch to ensure that prefs are read before we load JSM files? And... is it even achievable, given that profile migration needs to happen before we have a profile?

::: toolkit/xre/nsXREDirProvider.cpp:997
(Diff revision 1)
> +void
> +nsXREDirProvider::DoEarlyStartup()
> +{
> +  if (!mProfileNotified) {
> +    AutoRestore<bool> ar(mProfileNotified);
> +    mProfileNotified = true;

This feels like a hack, why do we have to temporarily flip mProfileNotified here?
Attachment #8906767 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Comment on attachment 8906767 [details]
> Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.
> 
> https://reviewboard.mozilla.org/r/178490/#review184088
> 
> (In reply to Andrew McCreight [:mccr8] from comment #1)
> > It works when the pref is set to true in all.js, but not when you manually
> > set it in about:config.
> 
> So isn't this good enough for the purpose of being able to turn off the
> "shared global for JSMs" behavior in beta if needed? I don't think users
> need to mess with that pref.

No, we need to be able to turn it on an off with an extension if necessary, which means respecting the value in the user branch. It's also useful for early testers.

> I've always assumed app-startup was a notification fired for code that wants
> to run before the profile has been selected. I don't think so close to the
> 57 merge is a great time to change this behavior.

The profile has already been selected at this point, and we've already started doing things with it. We just haven't gone as far as profile startup with things like extensions.

> ::: toolkit/xre/nsAppRunner.cpp:4470
> (Diff revision 1)
> >        // renamed, and potentially that the selected/default profile changed.
> >        mProfileSvc->Flush();
> >      }
> >    }
> >  
> > +  // Do early profile startup before notifying startup observers so user
> 
> The profile migrator will run before this. It's a JS component importing JSM
> files:
> http://searchfox.org/mozilla-central/source/browser/components/migration/
> ProfileMigrator.js
> Wasn't the goal of this patch to ensure that prefs are read before we load
> JSM files? And... is it even achievable, given that profile migration needs
> to happen before we have a profile?

I'm not especially concerned with the profile migrator. That runs in an extremely small fraction of sessions. I don't really mind if those sessions start with the default shareGlobal value as long as ordinary sessions get the user value.

That said, the other option is to initially load user preferences before the profile migrator has a chance to run, and re-read them if necessary if it does run, but I'm not convinced that's necessary.

> ::: toolkit/xre/nsXREDirProvider.cpp:997
> (Diff revision 1)
> > +void
> > +nsXREDirProvider::DoEarlyStartup()
> > +{
> > +  if (!mProfileNotified) {
> > +    AutoRestore<bool> ar(mProfileNotified);
> > +    mProfileNotified = true;
> 
> This feels like a hack, why do we have to temporarily flip mProfileNotified
> here?

Because normally we refuse to return the profile directory prior to DoStartup, but the preference service needs it in order to load preferences. It would probably be fine to make it available any time after DoEarlyStartup, but I figured it made more sense to keep closer to the current behavior for now.
Comment on attachment 8906767 [details]
Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.

https://reviewboard.mozilla.org/r/178490/#review184484

After reading your explanations in comment 6 (thanks!) and looking more at the code, I'm ok with this change and intend to r+ the next version.

::: toolkit/xre/nsXREDirProvider.cpp:993
(Diff revision 1)
>  
>    return mProfileDir->Clone(aResult);
>  }
>  
> +void
> +nsXREDirProvider::DoEarlyStartup()

Could we give this a less generic name that matches what it really does (eg. InitializeUserPrefs) to make it less tempting to use this method as a dumping ground for random other things?

::: toolkit/xre/nsXREDirProvider.cpp:997
(Diff revision 1)
> +void
> +nsXREDirProvider::DoEarlyStartup()
> +{
> +  if (!mProfileNotified) {
> +    AutoRestore<bool> ar(mProfileNotified);
> +    mProfileNotified = true;

I think a short comment in the code here to explain what you said about this in your last comment in the bug would be useful.

::: toolkit/xre/nsXREDirProvider.cpp:1000
(Diff revision 1)
> +  if (!mProfileNotified) {
> +    AutoRestore<bool> ar(mProfileNotified);
> +    mProfileNotified = true;
> +
> +    /*
> +       Setup prefs before profile-do-change to be able to use them to track

This comment needs an update.
Comment on attachment 8906767 [details]
Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.

https://reviewboard.mozilla.org/r/178490/#review184934

::: toolkit/xre/nsXREDirProvider.cpp:1025
(Diff revision 2)
>      /*
>         Setup prefs before profile-do-change to be able to use them to track
>         crashes and because we want to begin crash tracking before other code run
>         from this notification since they may cause crashes.
>      */
> -    mozilla::Preferences::InitializeUserPrefs();
> +    InitializeUserPrefs();

In which case is it useful to keep this second InitializeUserPrefs call?
Attachment #8906767 - Flags: review?(florian) → review+
Comment on attachment 8906767 [details]
Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.

https://reviewboard.mozilla.org/r/178490/#review184934

> In which case is it useful to keep this second InitializeUserPrefs call?

It's probably not, but I didn't feel comfortable expecting InitializeUserPrefs to always be called before DoStartup.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> Comment on attachment 8906767 [details]
> Bug 1398895: Part 1 - Initialize user prefs before loading any JS components.
> 
> https://reviewboard.mozilla.org/r/178490/#review184934
> 
> > In which case is it useful to keep this second InitializeUserPrefs call?
> 
> It's probably not, but I didn't feel comfortable expecting
> InitializeUserPrefs to always be called before DoStartup.

Maybe assert mPrefsInitialized is true instead then? (I don't care strongly either way)
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> Maybe assert mPrefsInitialized is true instead then? (I don't care strongly
> either way)

Yeah, that probably makes more sense. Will do.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b51e0600fdc414552dd9dd7e4eefe4534b5dd73c
Bug 1398895: Part 1 - Initialize user prefs before loading any JS components. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/97fbaf813dc6aaf4c1cb59d38268f4fe91950c54
Bug 1398895: Part 2 - Ensure component loader initialization after user prefs load. r=mccr8
Drive-by comment - I have high hopes that this will also fix the unnecessary language negotiation when using langpacks. We've been doing it because user prefs were read after the initial language negotiation happened.
You need to log in before you can comment on or make changes to this bug.