1344355 - User preferences should be read before JS Context is started (to initialize it with correct locale)

Status: ASSIGNED

(Toolkit :: Startup and Profile System, defect)

Description

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

We have a new LocaleService in intl/locale which is taking over the role of the central point where developers can retrieve the current app locale.

Behind the scenes, this API currently calls to nsChromeRegistryChrome::GetSelectedLocale.

On top of that, we're caching the negotiated locales, and we want to react with events when the locale negotiation should be triggered.

That doesn't work well with the peculiar behavior of ChromeRegistry in the scenario when used with a langpack.

In order to reproduce it, you need to:

1) Install/build Firefox in en-US
2) Install a langpack with another locale
3) Select this locale in about:config (`general.useragent.locale`)
4) Restart

In my debugging, what happens looks more or less like this:

projects/mozilla/mozilla-unified   central                                                               
▶ ./mach run
 0:00.16 /home/zbraniecki/projects/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -profile /home/zbraniecki/projects/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/tmp/scratch_user
LocaleService::ReadAppLocales starts
nsChromeRegistryChrome::GetSelectedLocale start
nsChromeRegistryChrome::GetSelectedLocale locale: en-US
nsChromeRegistryChrome::GetSelectedLocale end
LocaleService::ReadAppLocales: Chrome Locale: en-US
LocaleService::ReadAppLocales end
nsChromeRegistryChrome::GetSelectedLocale start
nsChromeRegistryChrome::GetSelectedLocale locale: ja-JP-mac
nsChromeRegistryChrome::GetSelectedLocale end
nsChromeRegistryChrome::GetSelectedLocale start
nsChromeRegistryChrome::GetSelectedLocale locale: ja-JP-mac
nsChromeRegistryChrome::GetSelectedLocale end

As you can see the initial locale reported by ChromeRegistry is "en-US". That's the call that LocaleService uses and we cache this response.

Then, some other callsite calls GetSelectedLocale again and from now on the response is "ja-JP-mac".
But since ChromeRegistry didn't call `UpdateSelectedLocale` in the middle, we didn't trigger the LocaleService::Refresh and we're stuck with 'en-US'.

:Pike says that we used to start properly with the langpack locale, but then we delayed addons initialization and now chrome registry tries to start with `ja-jp-mac`, but at that point it's an invalid resource, so it falls back on `en-US`, and only after addons are initialized it switches to `ja-JP-mac`.

I see three potential solutions:

1) Fix the addon initialization timeline, so that ChromeRegistry starts with langpack locale
2) Make ChromeRegistry call UpdateSelectedLocale when it registers the langpack so that LocaleService can refresh.
3) Remove LocaleService cache

The third option is the easiest to pull and the worst for the code quality as the "get selected locale" is called almost 20 times during app startup and it would be beneficial not to have to renegotiate it every time.
Also, the (3) doesn't allow us to call the LocaleService event that indicates that locales changed.

But if (1) or (2) will end up being too complex to fix, we can temporarily disable the cache to at least not regress when we switch callsites from ChromeRegistry to LocaleService.

Comment 1

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

:rhelmer said he wants to investigate.

Comment 2

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

I filed bug 1344445 for a somehow related problem.

Comment 3

[Robert Helmer [:rhelmer]]

Benjamin, Mossop suggested you might be able to take this and know which module/component it belongs to - let me know if not and I'll follow up.

Comment 4

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

I'm working on an unrelated patch, and was thinking about this behavior so I debugger it a bit further.

The issues seems to be that when the nsChromeRegistryChrome::SelectLocaleFromPref is called (from Init), it gets "en-US" out of prefs even when the general.useragent.locale" is set to different value.

You can easily reproduce it by printf'ing `provider` here: http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/chrome/nsChromeRegistryChrome.cpp#291

If you set your pref to a different value, the first call still returns 'en-US', and soon after, there's a second call that returns your value.

It's probably something about the order of registering prefs vs chrome that would be nice to fix.

Comment 5

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

There are actually two levels on which it fails.

1) Prefs return (default?) value for browser.useragent.locale first, and then they return the actual value. Excerpt from my log:

nsChromeRegistryChrome::Init called
nsChromeRegistryChrome::Init Got prefs in init!
nsChromeRegistryChrome::SelectLocaleFromPref called
nsChromeRegistryChrome::SelectLocaleFromPref Updating locale to provider: en-US
nsChromeRegistryChrome::SelectLocaleFromPref mSelectedLocale is: en-US
nsChromeRegistryChrome::UpdateSelectedLocale called
nsChromeRegistryChrome::SelectLocaleFromPref called
nsChromeRegistryChrome::SelectLocaleFromPref Updating locale to provider: pl
nsChromeRegistryChrome::SelectLocaleFromPref mSelectedLocale is: pl

2) For langpack, the order is:

1) Init chrome
2) Get mSelectedLocale 'en-US'
3) Register packages for en-US (lots of `ChromeRegistryChrome::ManifestLocale` calls for provider `en-US`)
4) Get mSelectedLocale 'pl', but negotiate it to 'en-US' because only 'en-US' is available
5) Register packages for 'pl' (lots of `ChromeRegistryChrome::ManifestLocale` calls for provider `pl`)
6) Get mSelectedLocale 'pl', and negotiate it to 'pl' because available locales in package are ['pl', 'en-US'].

Comment 6

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

:ehsan says that (1) should be fixable by reversing the order between prefs reading and XPCLocale init.

I'll try to fix it.

Comment 7

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

To sum it up, in the following scenario:

1) Firefox built with locale "de-AT"
2) Firefox `general.useragent.locale` changed by the user to "fr-CA"

 the current order of bootstrapping of a process is:

1) xpcModuleCtor launches and triggers XPCLocale::Init which requests LocaleService::GetAppLocale
2) LocaleService::GetAppLocale reads `general.useragent.locale` from prefs which at that point returns "de-AT"
3) nsPrefs triggers even about changed pref value for "general.useragent.locale"
4) LocaleService::GetAppLocale reads `general.useragent.locale` again and this time it returns "fr-CA"
5) XPCLocale should update it's locale

This is not optimal and if we could only reorder the initialization to get the prefs read before XPCLocale::Init is launched, we would avoid this and get:

1) XPCLocale::Init called, asks for appLocale
2) LocaleService reads app locale which at this point is already "fr-CA"

:nfroyd, :ehsan pointed you out as someone who might be able to help me find where in code should I look to fix the order.

tl;dr is:

I want [2] to be launched before [1]. Where do I reverse the order?

[1] http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCModule.cpp#11
[2] http://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#652

Comment 8

[(no longer active)]

Speaking to billm on IRC about this, an alternative solution might be to try to call JS_SetDefaultLocale() a bit later in the startup process.

We should check with the JS folks if that has any negative implications, of course.

Comment 9

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

We could probably make the JSContext's defaultLocale be lazy and called on the first use (new Date, new Intl.DateTimeFormat etc.) but that puts us in a non-deterministic mode where if someone, for whatever reason, triggers it before prefs are read, the wrong language will be set.

So, I'd like to first try to see if we can improve the order of bootstrapping to get prefs before XPCLocale is called. If that won't work, I may instead go for making defaultLocale in JSContext lazy and react to app-locales-changed event.

Comment 10

[Nathan Froyd [:froydnj]]

Prefs are read somewhere around here:

http://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1128

I think trying to move that before xpconnect gets initialized, which is spun up by the layout module, which is initialized here:

http://searchfox.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#709

is...a little risky, but worth a shot.  You could try moving that Preferences::ResetAndReadUserPrefs call into XPCOM startup and see what happens...

Comment 11

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

My attempt to move ReadUserPrefs earlier has failed.

It seems that trying to execute this line:
http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/modules/libpref/Preferences.cpp#875

before xpcModuleCtor is failing and I don't know why.

My only fallback strategy is to update context's locale when it is read from prefs. That means that we'll have the following order at bootstrap:

1) Read the default `general.useragent.locale`
2) Initialize JS Context with that locale
3) Read the user prefs for `general.useragent.locale`
4) Update JS Context locale.

I'll try to tackle the manifest files being read too late (second issue in this bug) now and see if I can reorder that.

Comment 12

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

The earliest line that doesn't make the above fail is: http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/toolkit/xre/nsXREDirProvider.cpp#1126

So basically "mProfileNotified = true" is needed for that to work.

I'd like to somehow leave a note to whoever one day will be reworking the bootstrap, that prefs should be read before JS context is bootstrapped.

Comment 13

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

I have a fix for the other bug (about package registration order) in bug 1347306.

I'll leave this one open in case one day someone will decide to fix the bootstrapping of Gecko :)

Comment 14

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

Falling back on the idea from comment 8 - :waldo, is there a chance that we could set JS context locale later than in XPCJSContext::Initialize() ? Like, maybe lazily to read it the first time someone asks for it?

This way we could avoid having to set it multiple times during bootstrap as we're learning about new requested and available locales.

Comment 15

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

Since it doesn't seem like we can fix this, in bug 1356066 I'll land a workaround which basically updates the JS Context locale when language changes.