Closed Bug 1441135 Opened 6 years ago Closed 6 years ago

menu bar, address bar etc. in English/Firefox install language despite language pack with other language active

Categories

(Core :: Internationalization, defect, P2)

Product:
Core
Component:
Internationalization
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 blocking verified
firefox61 --- verified

People

(Reporter: aryx, Assigned: zbraniecki)

References

Details

(Keywords: regression)

Attachments

(2 files)

min-regression.patch
3.89 KB, patch
Details | Diff | Splinter Review
Bug 1441135 - Stash all data needed for langpack startup in the startupCache.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
Only affects Nightly as this is a regression from bug 1431260 which can be reproduced in the latest Nightly (did so on Windows 8.1).

Steps to reproduce:
1. Install Firefox Nightly.
2. Install a corresponding language pack for the same build. To do so, check menu "Help" > "Troubleshooting Information" and look for the Build ID which is a datetimestamp.
3. Go to https://ftp.mozilla.org/pub/firefox/nightly/2018/02/ and find the build with the folder ending with "mozilla-central-l10n".
4. Go into win64/xpi/
5. Install firefox-60.0a1.de.langpack.xpi
6. Open about:config
7. Create the preference intl.locale.requested of type String and set the value to "de" (without quotes).
8. Close Firefox.
9. Launch it again.
Actual result:
Menu bar and address bar in English. about:home and about:support in German.
Expected result:
Menu bar and address bar in German.
Flags: needinfo?(gandalf)
Reproduced. Taking.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
I'm half a day into debugging this. It's very confusing so far, and there seem to be two separate issues:

1) It seems like Activity Stream is somehow picking the wrong locale
2) It seems that ChromeRegistry is intermittently picking the wrong locale

I'm focused on debugging (2) because I expect it to be much harder, but I'm also coming up empty handed so far. It seems like everything works as expected, all the locales are set properly and returned from GetProvider for type LOCALE, and the flush happens correctly when the langpack is registered, but somehow even consecutive windows use en-US strings in the menubar (intermittently!).

I'll keep trying but may need some help debugging (2).
ugh, ok. I looked into (1) and it just seems like it's a dev papercut - activity stream uses hardcoded en-US in non production builds.

Still no progress on (2) - LocaleService seems to operate properly, ChromeRegistry seems to be reporting the right providers and locale selections, and flushes cache after langpack is registered (in result of intl:app-locales-changed), but somehow the XUL is displayed in en-US :(
I'm giving up. The longer I stare at the debug output the more confused I am.

What I know is that:
 - LocaleService startup logic seems all right. It starts with 'en-US', and then AddonsManager adds 'pl' from langpack. requestedLocales are also all right. Starts with none, and then recognizes user's prefs.js for 'pl'.
 - ChromeRegistry also seems to be reporting right locales. First just en-US, then 'en-US' and 'pl'
 - The bug is intermittent although on my machine easy to reproduce

For the longest time I thought it's some weird condition race - like, if we register the L10nRegistry entries first, which triggers locale change, but at that moment ChromeRegistry doesn't yet have its new resources, so it negotiates down to en-US or sth like this.

But what's fascinating is that most of the time I can even reproduce it for opening a consecutive window!

In other words I can start Firefox, test that all the settings in LocaleService, L10nRegistry and ChromeRegistry are in order, all `GetBase`, `GetProvider` etc. return `pl` locale, and the caches have been flushed (I can even flush them again by firing `intl-app-locales-changed`), and then, when I open *next* window, it uses en-US.

I also looked through my whole patch from bug 1431260 and I don't see anything that could cause this regression there except of some condition race that I don't understand.

:jfkthame, sorry to bother you, but I'm wondering if you may have time to take a look at what may be going on here? If we can't figure it out we may want to consider backing out that patch from 60 :(
Flags: needinfo?(jfkthame)
On macOS, with Nightly and a langpack ("de") installed, I'm seeing an odd mixture of languages in the UI. Most of the menus are in English, although the File menu, for example, contains a single German item ("Tab schließen") while the rest of it is English. The Edit menu is mostly German, until we get to "Switch Text Direction". And the Window and Help menu titles are German (Fenster, Hilfe), along with their content.

Opening a new tab, I get German within the new tab page, but the placeholder in the URL bar ("Search or enter address") remains English. (OTOH, the placeholder in the Search field is German: "Suchen".)

The hamburger menu is entirely English, but its Help and Web Developer submenus are German. Tooltips for the various icons in the toolbar are a mixture... e.g. the Back and Forward arrows have English tooltips, but the Reload button is German.

Overall, a real mish-mash. Is the langpack really this incomplete? Or are we failing to apply its translations consistently? It seems hard to assess whether we're resolving locales properly if half the langpack is missing.... but is it, or is this buggy behavior? So far, the behavior seems to be reliably reproducible for me -- it looks like a half-done localization. Is it?

(Note that lots of the untranslated strings I am seeing are ones that have been in the product for many releases, so I wouldn't expect them to be missing just because they're recent additions that the localizers haven't finished yet.)
Flags: needinfo?(jfkthame) → needinfo?(gandalf)
The language pack has those strings, e.g. for the latest Nightly's language pack, go to https://ftp.mozilla.org/pub/firefox/nightly/2018/02/2018-02-27-10-01-26-mozilla-central-l10n/win32/xpi/ and download firefox-60.0a1.de.langpack.xpi.

Unzip it and open firefox-60.0a1.de.langpack.xpi\browser\chrome\de\locale\browser\browsert.dtd and seach for "Datei" and "File" (including the quotes). Only "Datei" is found which is the localization of the "File" menu item.
I tried installing the German language pack on Nightly in Italian. Is is expected to have still 'it' as first language in a couple of places in "Application Settings" (in about:support)?

Available Locales: ["it","en-US","de"]
Default Locale: it
One thing I noticed (on macOS), not sure if helpful: when I quite the app, app menus flashes from Italian to German right before quitting.
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #6)
> The language pack has those strings, e.g. for the latest Nightly's language
> pack, go to
> https://ftp.mozilla.org/pub/firefox/nightly/2018/02/2018-02-27-10-01-26-
> mozilla-central-l10n/win32/xpi/ and download firefox-60.0a1.de.langpack.xpi.
> 
> Unzip it and open
> firefox-60.0a1.de.langpack.xpi\browser\chrome\de\locale\browser\browsert.dtd
> and seach for "Datei" and "File" (including the quotes). Only "Datei" is
> found which is the localization of the "File" menu item.

Yeah, I just checked that's true for the macOS langpack, too. So then why do I get most (but not all) of the menubar staying in English? Seems like something's not working....


(In reply to Francesco Lodolo [:flod] from comment #7)

> I tried installing the German language pack on Nightly in Italian. Is is
> expected to have still 'it' as first language in a couple of places in
> "Application Settings" (in about:support)?
> 
> Available Locales: ["it","en-US","de"]
> Default Locale: it

Did you set intl.locale.requested to "de"?
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> > I tried installing the German language pack on Nightly in Italian. Is is
> > expected to have still 'it' as first language in a couple of places in
> > "Application Settings" (in about:support)?
> > 
> > Available Locales: ["it","en-US","de"]
> > Default Locale: it
> 
> Did you set intl.locale.requested to "de"?

Yes. 

As in your test, most of the main menus (but not every element), search placeholder and hamburger menu remain in Italian, while preferences and other parts seem correctly localized in German (including Activity Stream and Onboarding).
(In reply to Francesco Lodolo [:flod] from comment #7)
> I tried installing the German language pack on Nightly in Italian. Is is
> expected to have still 'it' as first language in a couple of places in
> "Application Settings" (in about:support)?
> 
> Available Locales: ["it","en-US","de"]
> Default Locale: it

Available Locales list is not ordered, so yeah, that's totally fine. The Requested and Supported lists should be ordered and have "de" first.

The values returned into XUL are very confusing as I said. I have no idea why since the paths seems to be correct, resources are there localized, and ChromeRegistry seems to have the right locales in the right places.

If it was about some language negotiation in ChromeRegistry (like, LanguagesMatch or GetProvider) it would be more consistent...

And the consecutive window behavior indicates it's not a condition race. Still lost.
Flags: needinfo?(gandalf)
Priority: -- → P2
Some progress. I was able to reduce the patch needed to revert the regression to a one page patch spanning between ChromeRegistry, nsBrowserGlue.js and LocaleService.

It's still confusing as to why the regression happens but at least I was able to reject many esoteric hypothesis and it seems like it's something confusing within our code.

I'll continue investigating but dumping the patch in case anyone wants to take a look.
Making progress on understanding what's going on. It *is* a condition race between LocaleService+L10nRegistry and ChromeRegistry.

Still not sure what exactly is going wrong with chrome registry, but at least I know that if we'll have to back it out, we'll just back out a small portion of the patch.
Aaaand we're into the Promise bug world - bug 1193394.

The exact cause of this bug is due to the fact that our weird Promises don't use microtasks. This in return means that when we register a langpack, we actually delay things much, much longer in [0].

Before bug 1431260 that wasn't symptomatic because the chrome registration happened before the async call and it was that call that registered the langpack locales into LocaleService.

With bug 1431260 landing, we now only register the new locale when L10nRegistry gets the new resources and that happens after [0].

I'm building central with the patch from bug 1193394 to test if fixing Promises will fix it. If it does, we're in a happy land since :hiro is pushing hard to land it in 60.

If even the microtask delay still causes issues we may have to do some trickery and maybe report available locales update synchronously in Extension.jsm.

But I'm going to bet $100 on bug 1193394 fixing it.


[0] https://searchfox.org/mozilla-central/rev/056a4057575029f58eb69ae89bc7b6c3c11e2e28/toolkit/components/extensions/Extension.jsm#1821
Depends on: 1193394
Let me know where to put the $100.

Bug 1193394 is unfortunately not enough. At startup, langpacks still read manifest data which is async.

That in turn pushes langpack locale registration into LocaleService past the point when ChromeRegistry is resolving UI.

I fixed it by moving all data necessary for langpack startup into install step and stashing it into the cache. I think it's overall a good choice, since installation is rare and it's ok to pay with some cache data stored for when the user is actually using the langpack.
This should have no cost on non-langpack users.

The benefit is that startup is now sync and we don't need the manifest. I confirmed that it fixes the issue.

Andrew - this is a pretty urgent since it is blocking 60. Let me know if you'll have time to review it.
Comment on attachment 8954632 [details]
Bug 1441135 - Stash all data needed for langpack startup in the startupCache.

https://reviewboard.mozilla.org/r/223710/#review230002

::: toolkit/components/extensions/Extension.jsm:636
(Diff revision 1)
>            .map(path => path.replace(/^\/*/, "/")));
>        }
>      } else if (this.type == "langpack") {
> -      // Compute the chrome resources to be registered for this langpack
> -      // and stash them in startupData
> +      // Langpack startup is performance critical, so we want to compute as much
> +      // as possible here to make startup not trigger async DB reads.
> +      // We'll store the three items below in the startupData.

nit: there are actually 4 items below

::: toolkit/components/extensions/Extension.jsm:1812
(Diff revision 1)
>          this.localeData.messages.set(locale, result);
>        });
>    }
>  
>    async _parseManifest() {
>      let data = await super.parseManifest();

This can collapse to just `return super.parseManifest();`.  In fact, you can even just roll it all right into `parseManifest()` right below.
Attachment #8954632 - Flags: review?(aswan) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f027c3d45c47
Stash all data needed for langpack startup in the startupCache. r=aswan
Backed out for xpcshell failures on test_webextension_langpack.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=165056478&repo=autoland&lineNumber=2433
Flags: needinfo?(gandalf)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95b662a55a1e
Backed out 1 changesets for xpcshell failures on test_webextension_langpack.js. CLOSED TREE
The reason for the xpcshell test failures was because now the startup is firing the event synchronously, while the test only set the listener for the `webextension-langpack-startup` once the startup kicked off.

I turned that around making sure to make the test work irrelevant of if the startup is sync or async.
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db4c74ac01a5
Stash all data needed for langpack startup in the startupCache. r=aswan
https://hg.mozilla.org/mozilla-central/rev/db4c74ac01a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: qe-verify+
I was able to reproduce this issue with Firefox 60.0a1 (2018-02-26) under Windows 10 x64, by following the steps mentioned in comment 0. I repeat the same steps on Firefox 61.0a1 (2018-03-26) and Firefox 60.0b6 (20180322152034) under Windows 10 x64, Ubuntu 16.04 x32 and macOS x10.13 using "de" and "zh-CN" language packs and I can confirm that this bug is fixed.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: