1656515 - Language on in-product UI and browser.i18n.getUILanguage() are not updating after changing runtime.settings.setLocales

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Arturo Mejia]

This is a regression in nightly GV: 80.0a1-20200727095125 from beta GV:79.0-20200717001501, every time we update runtime.settings.setLocales the web content was translated to the correct locale, for example the search box text in about:config was translated but this is not the case of nightly GV: 80.0a1-20200727095125.

I both versions beta GV:79.0-20200717001501 and GV: 80.0a1-20200727095125, I was able to confirm the pref intl.accept_languages is getting updated in both beta and nightly. But in beta the search box text about:config is getting translated but not in nightly.

More information can be found Fenix issue and Bug 1615680.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Zibi,
do you have any idea that could point us in the direction of which change could have regressed this in geckoview?
(there are some additional details also in the bugs linked in comment 0)

Comment 2

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

No changes in Gecko Intl module related to locale management have been done in 79, so I would be surprised if Gecko regressed anything here.

When the reporter says "web content was translated to the correct locale" do they mean in-product UI, or actual web content (say, alexa.com)?

Because web sites negotiate locales differently than our internal UIs.

intl.accept_languages is used for external web pages, not things like preferences.xhtml, about:memory, about:preferences and other in-product UIs.

Comment 3

[Arturo Mejia]

Thanks for the clarification Zibi,
We see a regression on in-product UI like about:config and web extension APIs see.

Comment 4

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

We see a regression on in-product UI like about:config and web extension APIs see.

In that case it would be useful to see the Intl section of about:support in "good" and "bad" case and compare.

Comment 5

[Arturo Mejia]

Attachment: Nightly GV: 80.0a1-20200727095125 (Not working version)

Comment 6

[Arturo Mejia]

Attachment: beta GV:79.0-20200717001501(Working_version).png

Comment 7

[Arturo Mejia]

I think the problem is clear now :)
In nightly we only have the en-US locale available.

Comment 8

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

That seems like it. So, something changed in how you build GV

Comment 9

[Arturo Mejia]

Pike are you familiar to why we are only shipping with en-US on GV 80 see comment #5?

Comment 10

[Axel Hecht [:Pike]]

As of bug 1605358, GV isn't supported yet.

Comment 11

[Arturo Mejia]

Does that mean the "Available Locales" section (about:support) both in comment #5 and comment #6 are not coming from GV?

Comment 12

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

Does that mean the "Available Locales" section (about:support) both in comment #5 and comment #6 are not coming from GV?

Available locales are coming from GV. Our build system packages a number of locales into the .apk and those are the locales you see as Available. Based on your screenshot something in that release chain changes between 79 and 80.

Comment 13

[Arturo Mejia]

Thanks Zibi, I'm not familiar with the GV build system and it looks like what is described on bug 1605358 is much older that this issue. I'm not sure who can help us to figure out what have changed between 79 and 80. Do you have any suggestions?

Comment 14

[Jeff Beatty [:gueroJeff]]

There currently isn't a story for GV localization. There's a bug on file, but there hasn't been progress in some time. https://bugzilla.mozilla.org/show_bug.cgi?id=1605358

Comment 15

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

I'm not sure who can help us to figure out what have changed between 79 and 80. Do you have any suggestions?

My first guess would be Axel (:Pike), but if he doesn't know, then I'd ask release drivers for Fenix/GV, maybe someone working on build system. I'm not sure who to NI from those teams, you'll have to ask on Matrix channels.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

We clearly had l10n working before 80 (via bug 1496190), so saying there's "no story" is not accurate. Callek/nalexander maybe you can take a look at what's gone wrong?

Comment 17

[Justin Wood (:Callek)]

Could this have been a regression from https://bugzilla.mozilla.org/show_bug.cgi?id=1527313 - https://hg.mozilla.org/mozilla-central/rev/328669c39fa8a31337970592d793ebed2fc71bdf ?

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #16)

We clearly had l10n working before 80 (via bug 1496190), so saying there's "no story" is not accurate. Callek/nalexander maybe you can take a look at what's gone wrong?

To #c17, I don't think there's any obvious impact of moving the l10n-central checkouts. I cracked open a Bgv multi-l10n AAR from 80 and the package structure looks like I would expect: inside assets/omni.ja, we have:

...
  -rw-r--r--       348   1-Jan-2010  00:00:00  res/multilocale.txt

with contents

an,ar,ast,az,be,bg,bn,br,bs,ca,cak,cs,cy,da,de,dsb,el,en-CA,en-GB,en-US,eo,es-AR,es-CL,es-ES,es-MX,et,eu,fa,ff,fi,fr,fy-NL,ga-IE,gd,gl,gn,gu-IN,he,hi-IN,hr,hsb,hu,hy-AM,id,is,it,ja,ka,kab,kk,kn,ko,lij,lo,lt,lv,ml,mr,ms,my,nb-NO,ne-NP,nl,nn-NO,oc,pa-IN,pl,pt-BR,pt-PT,rm,ro,ru,sk,sl,son,sq,sr,sv-SE,ta,te,th,tr,trs,uk,ur,uz,vi,wo,xh,zam,zh-CN,zh-TW

And I see entries like:

   1626:  -rw-r--r--      1660   1-Jan-2010  00:00:00  chrome/an/locale/an/browser/about.dtd
   1663:  -rw-r--r--      1727   1-Jan-2010  00:00:00  chrome/ar/locale/ar/browser/about.dtd
   1700:  -rw-r--r--      1624   1-Jan-2010  00:00:00  chrome/ast/locale/ast/browser/about.dtd
   1737:  -rw-r--r--      1641   1-Jan-2010  00:00:00  chrome/az/locale/az/browser/about.dtd
   1774:  -rw-r--r--      1894   1-Jan-2010  00:00:00  chrome/be/locale/be/browser/about.dtd
...

So there's no obvious issue with multi-locale packaging. Now, it's irritating that we don't build, say, a multi-locale GVE that we could use to bisect this problem. (Looking more closely at this, I see that we do build a multi-locale GVE, we just don't declare it as an artifact in the Bgv task! Bug 1657713.)

That suggests to me there are real changes in the l10n service. If I'm reading the release calendar correctly, we're interested in changes between 2020-07-27 and 2020-06-29. Looking at https://hg.mozilla.org/mozilla-central/log/tip/intl/locale/LocaleService.cpp I see an interesting change around the startup cache in the relevant window. Is it possible that res/multilocale.txt is not being handled appropriately by the startup cache after Bug 1627075?

Arturo: you could test if the startup cache is relevant by doing some targeted Fenix builds with Nightly GVs from before and after that ticket landed. (Be aware that it bounced several times so some care will be needed.) I don't have bandwidth to continue that investigation.

Comment 19

[Arturo Mejia]

I'm sorry, I don't have available cycles for this week, but I could take a look next week. If anyone could take a look, in the meantime.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

I tested a multi-locale GVE and about:support does not list any locales even on the first run, so I don't think the startup cache is in play.

Comment 21

[Arturo Mejia]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #20)

I tested a multi-locale GVE and about:support does not list any locales even on the first run, so I don't think the startup cache is in play.

Nick I given the result from :snorp test what should be the next steps?

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Arturo Mejia from comment #21)

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #20)

I tested a multi-locale GVE and about:support does not list any locales even on the first run, so I don't think the startup cache is in play.

Nick I given the result from :snorp test what should be the next steps?

I think bisecting would still be useful if you have time.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

I kicked off a Try build on Friday with some additional debugging messages. With that build it appears we're failing to load multilocale.txt. That's working fine for me in a local non-multi-locale build, so it's very strange. I'll dig further.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

This is interesting. Apparently the multilocale.txt contents we're getting are corrupted. Maybe it's compressed but we're not decompressing correctly? Very weird.

08-10 13:12:01.279 12842 12858 I Gecko   : SNORP: looking for update.locale
08-10 13:12:01.279 12842 12858 I Gecko   : SNORP: update.locale contents: en-US
<snip>
08-10 13:12:01.279 12842 12858 I Gecko   : SNORP: looking for res/multilocale.txt
08-10 13:12:01.279 12842 12858 I Gecko   : SNORP: res/multilocale.txt contents: ^]ÍËnÂ0^T<84>á}<9f>å÷C
08-10 13:12:01.279 12842 12858 I Gecko   : SNORP: got packaged locales ^]ÍËnÂ0^T<84>á}<9f>å÷C

Comment 25

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #24)

This is interesting. Apparently the multilocale.txt contents we're getting are corrupted. Maybe it's compressed but we're not decompressing correctly? Very weird.

Aha! This does seemto be pointing at an issue with https://bugzilla.mozilla.org/show_bug.cgi?id=1627075, which changes how we read from the omnijar! snorp, can you revert that patch locally? Or in try?

(Clearing NI from :tomprince, since the right people are already on it.)

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

It doesn't back out cleanly, so instead I've pushed a Try build using the commit before Bug 1627075 landed. We'll see how that goes.

Doug it seems likely we are having a problem here due to fallout from Bug 1627075. Reading res/multilocale.txt is corrupted only in multi-locale builds. Do you have any idea what could be going wrong here? Could the l10n repacking step be missing that makes the new omnijar stuff work?

Comment 27

[Doug Thayer [:dthayer] (he/him)]

Interesting. So, to clarify, bug 1627075 routes omnijar accesses through the startup cache. If this is a problem with the file contents being corrupted in the startup cache, I would expect this to only manifest on the second run of a build? (Since on the first run of a build, we haven't built the startup cache and saved it to the user's profile) Is this showing up on the first run / for a fresh profile? It could still be bug 1627075 either way, but it helps narrow things down.

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ok I've reproduced this on a local build by using setting MOZ_CHROME_MULTILOCALE to a large-ish string in order to get more data into multilocale.txt. This causes it to be compressed, and we are in fact getting the compressed data in LocaleService::GetGREFileContents() for multilocale.txt.

It does appear to be happening on the first start as well as subsequent ones.

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, I think I see the problem. The CacheAwareZipReader is using nsZipArchive::GetData(), which AFAICT does not decompress anything.

https://searchfox.org/mozilla-central/source/xpcom/build/Omnijar.cpp#268-279

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1656515 - Decompress multilocale.txt if necessary

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1835ac58c617
Decompress multilocale.txt if necessary r=dthayer

Comment 32

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=1835ac58c6174ae3a12fcb539a6b6de37667ad03&failure_classification_id=2&searchStr=Linux%2C18.04%2Cx64%2Casan%2Copt

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312648184&repo=autoland&lineNumber=2073

Backout: https://hg.mozilla.org/integration/autoland/rev/3bfe3b28bf50c6ce1199b9e1d409b19f93047596

Comment 33

[Doug Thayer [:dthayer] (he/him)]

Oops - sorry I didn't catch - looks like you forgot the [] in MakeUnique<uint8_t[]>(item->RealSize());. (In case you're still scratching your head, because I was for a bit.)

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c36827e81a7a
Decompress multilocale.txt if necessary r=dthayer

Comment 35

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c36827e81a7a

Comment 36

[Atila Butkovits]

Backed out for causing Bug 1659045.

Backout link: https://hg.mozilla.org/integration/autoland/rev/383073b9e168de132b6bed85e00d31c41e0e4caf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuperseded%2Cusercancel%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=c36827e81a7acdc660fe7772e419e0b4d32ab1f3

Comment 37

[Doug Thayer [:dthayer] (he/him)]

Note: if it becomes critical we can just bypass the startupcache entirely for this until we sort out why it's failing (by just using the old code and CacheAwareZipReader's mZip field.)

Comment 38

[Doug Thayer [:dthayer] (he/him)]

Eugh - sorry again. I still am not exactly sure what's accounting for the differences in reproducibility in the linked bug, but the problem is the confusing nature of the nsZipCursor API, inherited by CacheAwareZipCursor. You have to use the buffer returned by cursor.Read(...), and not the one you pass in. That buffer is only used if we had to decompress the contents.

So it should look something like this:

    uint32_t count;
    uint8_t* buf = cursor.Read(&count);

    if (count != item->RealSize()) {
      return false;
    }

    aOutString->Assign(reinterpret_cast<const char*>(buf), count);

Comment 39

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #38)

Eugh - sorry again. I still am not exactly sure what's accounting for the differences in reproducibility in the linked bug, but the problem is the confusing nature of the nsZipCursor API, inherited by CacheAwareZipCursor. You have to use the buffer returned by cursor.Read(...), and not the one you pass in. That buffer is only used if we had to decompress the contents.

Oof. Thanks.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a94d0040d2f
Decompress multilocale.txt if necessary r=dthayer

Comment 41

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9a94d0040d2f

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9169257 [details]
Bug 1656515 - Decompress multilocale.txt if necessary

Beta/Release Uplift Approval Request

• User impact if declined: No localizations
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: It's difficult because right now we don't publish an example app from the multilocale bulids, but on a Fenix or Reference Browser built against it you should observe more locales than just "en-US" available in about:support.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium because it bounced once after merging to mozilla-central. I think we have the kinks worked out now, though.
• String changes made/needed: None

Comment 43

[Julien Cristau [:jcristau]]

Comment on attachment 9169257 [details]
Bug 1656515 - Decompress multilocale.txt if necessary

approved for 80 rc1

Comment 44

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/7f437cdb68e0

Comment 45

[Ryan VanderMeulen [:RyanVM]]

FYI, bug 1627075 was reverted in 80.0rc2. We should still verify that things work as expected with this bug, however. We should also try to verify that we didn't somehow cause bug 1659045 to regress along the way.