Closed Bug 1656515 Opened 6 months ago Closed 5 months ago

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

Categories

(GeckoView :: General, defect, P1)

Unspecified
All

Tracking

(firefox-esr68 unaffected, firefox-esr78 unaffected, firefox79 unaffected, firefox80 fixed, firefox81 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- fixed
firefox81 --- fixed

People

(Reporter: amejiamarmol, Assigned: dthayer)

References

(Regression)

Details

Attachments

(3 files)

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.

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)

Flags: needinfo?(zbraniecki)

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.

Flags: needinfo?(zbraniecki)

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

Flags: needinfo?(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.

Flags: needinfo?(gandalf)
Summary: Language in web content is not updating after changing runtime.settings.setLocales → Language in in-product UI and browser.i18n.getUILanguage() are not updating after changing runtime.settings.setLocales
Summary: Language in in-product UI and browser.i18n.getUILanguage() are not updating after changing runtime.settings.setLocales → Language on in-product UI and browser.i18n.getUILanguage() are not updating after changing runtime.settings.setLocales

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

Flags: needinfo?(gandalf)

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

Flags: needinfo?(gandalf)

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

Flags: needinfo?(pike-bugzilla)
Flags: needinfo?(l10n)

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

Flags: needinfo?(pike-bugzilla)
Flags: needinfo?(l10n)

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

Flags: needinfo?(l10n)
Flags: needinfo?(l10n) → needinfo?(jbeatty)

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.

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?

Flags: needinfo?(gandalf)

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

Flags: needinfo?(jbeatty)

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.

Flags: needinfo?(gandalf)
Severity: -- → S2
Priority: -- → P1

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?

Flags: needinfo?(nalexander)
Flags: needinfo?(bugspam.Callek)
Flags: needinfo?(bugspam.Callek) → needinfo?(mozilla)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.

Flags: needinfo?(nalexander) → needinfo?(amejiamarmol)

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.

Flags: needinfo?(amejiamarmol)

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.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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?

Flags: needinfo?(nalexander)

(In reply to Arturo Mejia from comment #21)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.

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.

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

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.)

Flags: needinfo?(snorp)
Flags: needinfo?(nalexander)
Flags: needinfo?(mozilla)

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?

Flags: needinfo?(snorp) → needinfo?(dothayer)

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.

Flags: needinfo?(dothayer)

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.

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

Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1835ac58c617
Decompress multilocale.txt if necessary r=dthayer

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.)

Flags: needinfo?(dothayer) → needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c36827e81a7a
Decompress multilocale.txt if necessary r=dthayer
Flags: needinfo?(snorp)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 81 Branch → ---

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.)

Flags: needinfo?(dothayer)

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);
Flags: needinfo?(snorp)

(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.

Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a94d0040d2f
Decompress multilocale.txt if necessary r=dthayer
Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

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
Attachment #9169257 - Flags: approval-mozilla-beta?

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

approved for 80 rc1

Attachment #9169257 - Flags: approval-mozilla-beta? → approval-mozilla-release+
QA Whiteboard: [qa-triaged]

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.

You need to log in before you can comment on or make changes to this bug.