1405264 - webext-langpacks register chrome entries later than old-style langpacks

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

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

While testing the new webext language packs, we noticed that in several cases the strings remain in en-US despite the langpack being installed and selected.

The same behavior cannot be reproduced in old-style language packs.

STR:

1) Build Nightly
2) `make -C objdir/browser/locales langpack-webext-de`
3) `rm -rf objdir/tmp/scratch_user`
4) Start the browser (in en-US)
5) File>Open `objdir/dist/${platform}/xpi/firefox-58.0a1.de.langpack.xpi`
6) Open Browser Console and type `Services.locale.setRequestedLocales(['de']);`
7) Restart the browser
8) Hover over "reload" button in the main toolbar

Current result:

The tooltip is in English.

Expected result:

The tooltip should be in German.


Analysis:

It seems to me that the whole `gNavigatorBundle` is in en-US, despite the fact that loading `chrome://browser/locale/browser.properties` into a new tab opens the file in german.

My hypothesis is that `gNavigatorBundle` is loaded before WebExtensions register the new chrome entries for the langpack, which makes them open in en-US.

I believe we need to move the registration of the chrome entries to be at least on par with when ChromeRegistry is adding entries in order to not regress when we switch langpacks to the new model.

Comment 1

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

Kris, I think I need your help with this one.

Comment 2

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

Actually, unless I set `nglayout.debug.disable_xul_cache=true` in about:config, even in consecutive windows most of the DTD/XUL strings are still in English.

I see two solutions:

1) Get WebExtensions registering chrome entries on par with the ChromeRegistry
2) Move the chrome registry entries adding to chrome manifests

Comment 3

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

New STR:

1) Build Nightly
2) `make -C objdir/browser/locales langpack-webext-de`
3) `rm -rf objdir/tmp/scratch_user`
4) Start the browser (in en-US)
5) File>Open `objdir/dist/${platform}/xpi/firefox-58.0a1.de.langpack.xpi`
6) Open Browser Console and type `Services.locale.setRequestedLocales(['de']);`
7) Restart the browser

Current result:

Main menu, Photon menu and URL bar are in en-US

Expected result:

Everything should be in `de`.

Notes:

When `nglayout.debug.disable_xul_cache=true` the consecutive windows opened have everything in `de` which indicates that the reason for the regression is that we cache values for en-US when using the webext-langpacks.
This is probably due to WebExtensions registering chrome entries later than ChromeRegistry registers them from its manifest files.

Comment 4

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

Note - the current langpack system is deeply non-deterministic and we *always* (both in the old and new model) are racing. The difference is, in my hypothesis, that in the old system we win the race most of the time, and in the new model we lose the race.
There's no mechanism in the old localization API to react to language change on fly, so we have to win the race to show the right locale.

One of the features of the new localization API is that it's deterministic and can dynamically react to language changes, so this will not be an issue.
But for now, we need to fix it to switch to new langpacks to start being able to transition to the new L10n API. :)

Comment 5

[Axel Hecht [:Pike]]

Yes, this is very similar to bug 1005640, where we made one of those race conditions catch notifications.

Comment 6

[Andrew Swan [:aswan]]

Capturing results of IRC discussion earlier today, Kris suggests keeping the needed chrome registry entries in addonStartup.json so we can install them synchronously during startup (as opposed to in an asynchronous task that has to do some I/O before installing them which is what happens now).
I can help with this but I would be more comfortable if we could do a quick experiment to verify that this will work.  I think we can do it by making this step synchronous:
http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/toolkit/components/extensions/Extension.jsm#1675
using something like this:
http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#393-409

That's not something we'll land of course but if that solves the problem, then I'll be confident that the addonStartup.json technique will work.

I can try that experiment later today unless you can beat me to it :gandalf?

Comment 7

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

I verified that adding a spinEventLoop hack solves the race.

Andrew volunteered to write the patch.

Comment 8

[Andrew Swan [:aswan]]

I'm trying to follow the STR for testing but getting this failure:

$ make -C objdir-artifact/browser/locales  langpack-webext-de
abort: HTTP Error 404: Not Found
make[1]: *** [merge-webext-de] Error 255
make: *** [libs-webext-de] Error 2

Comment 9

[Axel Hecht [:Pike]]

That's a gnu make version thing.

Rename langpack-webext-%  to webext-langpack-%, and you should be able to build.

Comment 10

[Andrew Swan [:aswan]]

That got me a little further but then it failed with:

sed -e 's/%AB_CD%/de/' /Users/aswan/src/mozilla-unified/toolkit/locales/update.locale > ../../dist/xpi-stage/locale-de/update.locale
/bin/sh: ../../dist/xpi-stage/locale-de/update.locale: No such file or directory

I created the missing directory and the next failure is:

/Users/aswan/src/mozilla-unified/objdir-artifact/_virtualenv/bin/python -m mozbuild.action.langpack_manifest --locales de --min-app-ver 58.0a1 --max-app-ver 58.0a1 --app-name "Nightly" --l10n-basedir "/Users/aswan/.mozbuild/l10n-central" --defines /Users/aswan/.mozbuild/l10n-central/de/toolkit/defines.inc /Users/aswan/.mozbuild/l10n-central/de/browser/defines.inc --input ../../dist/xpi-stage/locale-de
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/local/Cellar/python/2.7.13_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/aswan/src/mozilla-unified/python/mozbuild/mozbuild/action/langpack_manifest.py", line 446, in <module>
    main(sys.argv[1:])
  File "/Users/aswan/src/mozilla-unified/python/mozbuild/mozbuild/action/langpack_manifest.py", line 429, in main
    os.path.join(args.input, 'chrome.manifest'), args.input, chrome_entries)
  File "/Users/aswan/src/mozilla-unified/python/mozbuild/mozbuild/action/langpack_manifest.py", line 262, in parse_chrome_manifest
    for entry in parse_manifest(None, path):
  File "/Users/aswan/src/mozilla-unified/python/mozbuild/mozpack/chrome/manifest.py", line 348, in parse_manifest
    fileobj = open(path)
IOError: [Errno 2] No such file or directory: '../../dist/xpi-stage/locale-de/chrome.manifest'

Maybe for the short term somebody could just attach a built XPI file to this bug for testing?

Comment 11

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

Attachment: firefox-58.0a1.pl.langpack.xpi

Attaching pl langpack

Comment 12

[Axel Hecht [:Pike]]

(In reply to Andrew Swan [:aswan] from comment #10)

You're describing all the symptoms of trying to do a repack/langpack on top of an artifact build, which is busted :-(. That's bug 1387485

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1405264 Part 1: Add startupData to internal addon representation

wip for testing, need a better description here

Review commit: https://reviewboard.mozilla.org/r/186454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186454/

Comment 14

[Andrew Swan [:aswan]]

(In reply to Axel Hecht [:Pike] from comment #12)
> You're describing all the symptoms of trying to do a repack/langpack on top
> of an artifact build, which is busted :-(. That's bug 1387485

Oh yes indeed, I was trying with an artifact build, sorry for the noise.

I've attached a wip patch that I think works, but I'm having a little trouble testing with the attached langpack.
Specifically, if I install it and set the requested locales to ["pl"] then without the patch the browser starts up with about half the UI elements in English and the rest in Polish.  But if I try to open a new window I get the error window that is displayed when a XUL document is missing a localized entity, I assume that means the langpack just isn't fully in sync with the tip of m-c.
With the attached patch applied, after installing the langpack, setting the requested locale, and restarting, I immediately see the XML/XUL missing entity error.  So, I think that's actually progress since it means the pl langpack's resources are now in place earlier than they were before?

Can I trouble somebody more capable than me with langpacks to try the attached patch?
Also, I'm not sure if or how this can be tested in automation, I'll think about it but input is welcome on that subject.

Comment 15

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

Yep, this patch fixes the race and the browser works properly with a language pack after the patch.

Not sure how to test it in automation. You need a full build to build a langpack for your curent revision of nightly and then you can test the langpack from the given revision against the build for the same revision.

Comment 16

[Andrew Swan [:aswan]]

Attachment: Bug 1405264 Part 2: Use startupData for new langpacks

Webextension-formatted langpacks now store their list of chrome
registry resources in startupData so that those resources can be
registered early in startup.

Review commit: https://reviewboard.mozilla.org/r/186510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186510/

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8915228 [details]
Bug 1405264 Part 1: Add startupData to internal addon representation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186454/diff/1-2/

Comment 18

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

Comment on attachment 8915317 [details]
Bug 1405264 Part 2: Use startupData for new langpacks

https://reviewboard.mozilla.org/r/186510/#review191600

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8915228 [details]
Bug 1405264 Part 1: Add startupData to internal addon representation

https://reviewboard.mozilla.org/r/186454/#review192076

I think at some point we may want to make this a key-value store, or at least have it default to an empty object, to make it easier to use without conflict, but this is probably fine for now.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8915317 [details]
Bug 1405264 Part 2: Use startupData for new langpacks

https://reviewboard.mozilla.org/r/186510/#review192078

r=me with the computed chrome resources stored in the startup data rather than computed at every startup.

::: toolkit/components/extensions/Extension.jsm:1680
(Diff revision 1)
>                                        () => this._parseManifest());
>    }
>  
>    async startup(reason) {
> +    this.chromeRegistryHandle = null;
> +    const chromeResources = this.getChromeResources(this.startupData.languages);

This is nontrivially expensive. I know it doesn't seem like that big a deal, but things like this add up pretty fast during startup.

I'd rather we store the computed chrome resource entries in the startupData blob, possibly along with the languages object, possibly instead of it.

Comment 21

[Axel Hecht [:Pike]]

How do we validate that the data is still up-to-date, though?

Comment 22

[Andrew Swan [:aswan]]

(In reply to Axel Hecht [:Pike] from comment #21)
> How do we validate that the data is still up-to-date, though?

I don't understand the question.  The set of chrome resources to be registered is fixed for a given version of a given langpack.
Are you asking what happens when a langpack is updated to a new version?

Comment 23

[Kris Maglione [:kmag]]

That's a good point. We should add a test to check that the startupData doesn't persist across updates. It shouldn't, the way this is implemented, but a test would still be good.

Hm. But also, I probably should have reviewed more carefully, because I don't think the second patch is actually saving the startupData, only using it if it's found.

Comment 24

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #23)
> That's a good point. We should add a test to check that the startupData
> doesn't persist across updates. It shouldn't, the way this is implemented,
> but a test would still be good.

Across extension updates?  That's a good idea, I'd like to do it in a follow-up so we don't delay the overall langpack transition project.

> Hm. But also, I probably should have reviewed more carefully, because I
> don't think the second patch is actually saving the startupData, only using
> it if it's found.

The code to save startupData was in part 1, it only happens if the AddonInternal object has a non-null startupData property.  The change to XPIInstall.jsm in part 2 (in conjunction with the other changes) actually populate that property for langpacks.

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8915317 [details]
Bug 1405264 Part 2: Use startupData for new langpacks

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186510/diff/1-2/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b34e23227f2
Part 1: Add startupData to internal addon representation r=kmag
https://hg.mozilla.org/integration/autoland/rev/63618add0894
Part 2: Use startupData for new langpacks r=gandalf,kmag

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1b34e23227f2
https://hg.mozilla.org/mozilla-central/rev/63618add0894

Comment 28

[marius.santa]

Attachment: lang pack working.gif

I can reproduce this issue on Firefox 58.0a1 (20171003220138) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 58.0b9 (20171204150510), Firefox 59.0a1 (20171207220423) under Wind 10 64-bit and Mac OS X 10.13.