Closed Bug 1101527 Opened 5 years ago Closed 5 years ago

Some parts of Firefox use OS locale after browser restart

Categories

(Firefox for Android :: Locale switching and selection, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- unaffected
firefox35 + verified
firefox36 + verified
firefox37 + verified
fennec 35+ ---

People

(Reporter: CristinaM, Assigned: rnewman)

Details

(Keywords: regression, reproducible)

Attachments

(1 file, 1 obsolete file)

Environment:
Device: Nexus 4(Android 4.4.4)
Build: Firefox for Android 36.0a1 (2014-11-19) 

Steps to reproduce:
1. Go to Settings -> Language and set browser to Spanish;
2. Close Fennec (Force stop or swipe Fennec from Recent apps);
3. Relaunch Fennec.

Actual result:
about:home page, the context menu and the custom menu are in English.

Expected result:
All strings are in Spanish.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
M-C; pulling inbound in a followup

Last good revision: a255a234946e (2014-10-28)
First bad revision: 7e3c85754d32 (2014-10-29)

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a255a234946e&tochange=7e3c85754d32
Summary: Firefox uses System default language after browser restart → Gecko uses OS locale after browser restart
That pushlog doesn't seem like it includes any smoking guns. I would have expected Bug 1045053, which was ten+ days before your bisection point.
I see the exact opposite symptom on a Nexus 7. If I relaunch and load about:firefox I get Spanish content and page title, but the UI is in English.

This might be a newtablet regression. Testing on phone now.
Summary: Gecko uses OS locale after browser restart → Some parts of Firefox use OS locale after browser restart
Yup, I get the same thing on phone: something regressed locale restoration on startup.

Aaron, a bisection would be useful.
Flags: needinfo?(aaron.train)
11-19 10:36:09.923 D/GeckoApp( 8969): OS locale is en_US, app locale is fr
...
11-19 10:36:10.153 D/GeckoBrowserApp( 8969): onLocaleReady: fr

so the locale switcher code seems to be working correctly.
Seems like we only store en-US and not multi builds on integration FTP so perhaps this might be easier with one whom has a local environment already setup with locales available for packaging
Flags: needinfo?(aaron.train)
Aaron: did you just tell me to go find my own regression range? :D
Perchance!

Can confirm that in-browser strings are set properly with a system language change.
The baffling results of my initial investigation: during early init, our locale is es_ES. When we get to oWFC, it's en_US.
And here's the culprit:

Locale.setDefault(Locale) line: 1599	
GeckoThread.initGeckoEnvironment() line: 119	
GeckoThread.run() line: 171	

120037:     private String initGeckoEnvironment() {
120037:         // At some point while loading the gecko libs our default locale gets set
120037:         // so just save it to locale here and reset it as default after the join
120037:         Locale locale = Locale.getDefault();

this hasn't been changed in a while, so I imagine this is a timing-related change.

Probably the right thing to do is delete all of that code from initGeckoEnvironment.



nchen, how much of your reordering stuff stuck?

It looks like the backout of Bug 1075644 might be what regressed this.
With this patch, an fx-team build correctly takes on its saved locale and loads pages.
Attachment #8525752 - Flags: review?(nchen)
Attachment #8525752 - Flags: review?(bnicholson)
Hardware: ARM → All
Version: Trunk → Firefox 35
(In reply to Richard Newman [:rnewman] from comment #10)
> nchen, how much of your reordering stuff stuck?
> 
> It looks like the backout of Bug 1075644 might be what regressed this.

The load-Gecko-early stuff all stuck. Bug 1075644 should really only impact timing on single-core devices.
Comment on attachment 8525752 [details] [diff] [review]
Don't futz with Locale during GeckoThread startup. v1

Review of attachment 8525752 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoThread.java
@@ -88,5 @@
>      }
>  
>      private String initGeckoEnvironment() {
> -        // At some point while loading the gecko libs our default locale gets set
> -        // so just save it to locale here and reset it as default after the join

Assuming this comment is still relevant, and loading Gecko libs changes our default locale, shouldn't we still restore the default locale to the user setting right after we load Gecko libs?

@@ -94,5 @@
>  
> -        if (locale.toString().equalsIgnoreCase("zh_hk")) {
> -            locale = Locale.TRADITIONAL_CHINESE;
> -            Locale.setDefault(locale);
> -        }

I guess we still want "zh_HK" to map to "zh_TW", unless the locale switcher does that already?
Attachment #8525752 - Flags: review?(nchen)
(In reply to Jim Chen [:jchen] from comment #13)

> Assuming this comment is still relevant, and loading Gecko libs changes our
> default locale, shouldn't we still restore the default locale to the user
> setting right after we load Gecko libs?

I don't believe this comment is still relevant.

Empirically, testing indicates that it's false.

Theoretically, we eliminated the Gecko -> Java locale stuff a long time ago. In each release we stomp it closer to the ground -- the only remaining Gecko-side locale detection is inside nsLocaleService, which doesn't really do much. We set all of Gecko's locale prefs from Java-supplied data.


> I guess we still want "zh_HK" to map to "zh_TW", unless the locale switcher
> does that already?

It doesn't.

My understanding is that this is a manual fallback, analogous to Bug 933315 for es -- we don't ship 'es' or 'zh', so if you pick a different variant (i.e., not es-MX, es-ES, es-AR, zh-TW, or zh-CN) there's no sibling variant selection logic, and it'll fall back to en-US.

Is my understanding correct?

If so, either (re)implementing this mapping should be a separate bug -- because this definitely isn't the correct place to include that logic -- or it shouldn't be done at all; see Axel's comment in Bug 933315 Comment 7, which suggests some caution:

    Not for pt-* and zh-*, because we don't know which to fall back
    to. Or I don't for pt-*, and I think for zh-* it's different
    per region.

The approach taken in Bug 933315 was to move towards shipping 'es'.

As far as I know, 'zh' doesn't make any sense (there's no default!), but should we be shipping a zh-HK?
(In reply to Richard Newman [:rnewman] from comment #14)
> > I guess we still want "zh_HK" to map to "zh_TW", unless the locale switcher
> > does that already?
> 
> It doesn't.
> 
> My understanding is that this is a manual fallback, analogous to Bug 933315
> for es -- we don't ship 'es' or 'zh', so if you pick a different variant
> (i.e., not es-MX, es-ES, es-AR, zh-TW, or zh-CN) there's no sibling variant
> selection logic, and it'll fall back to en-US.
> 
> Is my understanding correct?

IIRC the issue was (on some devices?) zh-HK falls back to zh-CN, which uses simplified characters, instead of zh-TW, which uses traditional characters. A lot of zh-HK users are averse to seeing simplified characters for cultural reasons, so we put in a workaround to force zh-HK to map to zh-TW.
> A lot of zh-HK users are averse to seeing simplified characters for cultural
> reasons, so we put in a workaround to force zh-HK to map to zh-TW.

This was added in Bug 839380.

It looks like the implicit logic prior to that workaround was:

  * Java-side: zh-HK => en-US (the fallback I described in Comment 14)
  * Gecko-side: zh-HK => zh-CN (which Bug 839883 was filed to fix).

Obviously we took a slightly different tack in Bug 839380 by deliberately introducing a mapping, rather than 'fixing' things and having a fallback to en-US.

Assuming that that reasoning hasn't been invalidated, I'll file a bug to add an explicit mapping stage to BLM. That alone might also propagate correctly to Gecko, but if not we'll augment the OS locale handling in browser.js.
> Assuming that that reasoning hasn't been invalidated, I'll file a bug to add
> an explicit mapping stage to BLM. That alone might also propagate correctly
> to Gecko, but if not we'll augment the OS locale handling in browser.js.

Actually, might as well do it in this bug, 'cos they'll need to land together.
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
Comment on attachment 8525752 [details] [diff] [review]
Don't futz with Locale during GeckoThread startup. v1

Review of attachment 8525752 [details] [diff] [review]:
-----------------------------------------------------------------

Had the same questions as jchen. Removing r? for now since it sounds like an updated (or additional?) patch is on the way.
Attachment #8525752 - Flags: review?(bnicholson)
This alone fixes the bug for me. The second application of the locale is unnecessary, because the Gecko thread startup now no longer modifies our locale. The only step needed is to potentially remap the locale, and this can occur anywhere. We leave it here for simplicity's sake.

However, it's worth noting that the current fix is broken: whenever a configuration change happens in the app, it'll reset us back to zh-HK, because the external locale will be reapplied.

My devices don't allow me to choose zh-HK, so I can't test that the remapping still works, but I'd be surprised if this patch caused a regression in that part.

I'll file a follow-up to address the remapping issue more thoroughly in the 36/37 cycle.

I'm on PTO until Wednesday, so assuming this passes muster -- both review and testing from someone able to reproduce the original remapping (Jim, do you have any suitable devices?) -- then I ask that you land it and uplift it on my behalf.

An APK to test is at http://people.mozilla.org/~rnewman/fennec/hk.apk.

Thanks!
Attachment #8527129 - Flags: review?(nchen)
Attachment #8527129 - Flags: review?(bnicholson)
Attachment #8525752 - Attachment is obsolete: true
Comment on attachment 8527129 [details] [diff] [review]
Don't set locales after Gecko startup. v2

Review of attachment 8527129 [details] [diff] [review]:
-----------------------------------------------------------------

I'll test the build later today.
Attachment #8527129 - Flags: review?(nchen) → review+
Hmm when I set locale to zh-HK, the Java UI uses zh-TW, but Gecko uses en-US. "Accept-Language" header shows "en-US,zh-TW". Regular multi-locale Nightly doesn't have this problem.
Accept-Language is computed by Fennec; not an indication of what locale Gecko is using.

I would be slightly surprised if this patch changed behavior in this way, so I'm inclined to chalk this up to my local build approach.
So after I killed your build and restarted it, everything is correct again. Maybe the first time it was already running when I made the locale switch. Ship it! :D
Comment on attachment 8527129 [details] [diff] [review]
Don't set locales after Gecko startup. v2

Review of attachment 8527129 [details] [diff] [review]:
-----------------------------------------------------------------

Just a cursory r+ from me since I haven't tested this, nor am I deeply familiar with our locale handling. But the comments here make sense, and I trust jchen's testing!
Attachment #8527129 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/b4628cb58bb8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment on attachment 8527129 [details] [diff] [review]
Don't set locales after Gecko startup. v2

Approval Request Comment
[Feature/regressing bug #]:
  Seems to be a timing-related change in 35+, perhaps something that Bug 1075644 was trying to fix.

  The broken code here is a remnant of an old workaround.

[User impact if declined]:
  Fennec's UI locale will be wrong.

[Describe test coverage new/current, TBPL]:
  Manual testing. Our automated test environment doesn't allow for testing of locales, particularly when OS locale switching is required.

[Risks and why]: 
  In theory this is low risk: this code can't have been having an effect for most users, and this is the most minimal fix, preserving existing minor bugginess to avoid having to make lots of changes.

  In practice: well, this is Fennec. There's always some risk.

[String/UUID change made/needed]:
  None.
Attachment #8527129 - Flags: approval-mozilla-aurora?
Verified as fixed in Firefox for Android 36.0a1 (2014-11-26)
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Attachment #8527129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone = 36 => status-firefox-37 = fixed.
Verified as fixed in Firefox for Android 35.0a2 (2014-11-27)
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Verified as fixed in Firefox for Android 37.0a1 (2014-12-04)
Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.