Open Bug 1344355 Opened 4 years ago Updated 3 years ago
User preferences should be read before JS Context is started (to initialize it with correct locale)
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.
:rhelmer said he wants to investigate.
I filed bug 1344445 for a somehow related problem.
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.
Flags: needinfo?(rhelmer) → needinfo?(benjamin)
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.
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'].
Summary: Improve the locale reporting from nsChromeRegistryChrome when used with langpacks → Improve the locale registration in ChromeRegistry
Component: Add-ons Manager → Startup and Profile System
:ehsan says that (1) should be fixable by reversing the order between prefs reading and XPCLocale init. I'll try to fix it.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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  to be launched before . Where do I reverse the order?  http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCModule.cpp#11  http://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#652
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.
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.
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...
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.
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.
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 :)
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.
Summary: Improve the locale registration in ChromeRegistry → User preferences should be read before JS Context is started (to initialize it with correct locale)
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.
You need to log in before you can comment on or make changes to this bug.