Closed
Bug 1101527
Opened 10 years ago
Closed 9 years ago
Some parts of Firefox use OS locale after browser restart
Categories
(Firefox for Android Graveyard :: Locale switching and selection, defect)
Tracking
(firefox34 unaffected, firefox35+ verified, firefox36+ verified, firefox37+ verified, fennec35+)
VERIFIED
FIXED
Firefox 36
People
(Reporter: CristinaM, Assigned: rnewman)
Details
(Keywords: regression, reproducible)
Attachments
(1 file, 1 obsolete file)
2.69 KB,
patch
|
jchen
:
review+
bnicholson
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Comment 1•10 years ago
|
||
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
Keywords: regressionwindow-wanted
Assignee | ||
Updated•10 years ago
|
Summary: Firefox uses System default language after browser restart → Gecko uses OS locale after browser restart
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
Yup, I get the same thing on phone: something regressed locale restoration on startup. Aaron, a bisection would be useful.
Flags: needinfo?(aaron.train)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Aaron: did you just tell me to go find my own regression range? :D
Comment 8•10 years ago
|
||
Perchance! Can confirm that in-browser strings are set properly with a system language change.
Assignee | ||
Comment 9•10 years ago
|
||
The baffling results of my initial investigation: during early init, our locale is es_ES. When we get to oWFC, it's en_US.
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Hardware: ARM → All
Version: Trunk → Firefox 35
Comment 12•10 years ago
|
||
(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 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
(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?
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
> 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.
Assignee | ||
Comment 17•10 years ago
|
||
> 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.
Comment 18•10 years ago
|
||
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
tracking-firefox35:
--- → ?
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8525752 -
Attachment is obsolete: true
Comment 21•9 years ago
|
||
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+
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 23•9 years ago
|
||
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.
Comment 24•9 years ago
|
||
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 25•9 years ago
|
||
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+
Updated•9 years ago
|
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b4628cb58bb8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Assignee | ||
Comment 28•9 years ago
|
||
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?
Reporter | ||
Comment 29•9 years ago
|
||
Verified as fixed in Firefox for Android 36.0a1 (2014-11-26) Device: Asus Transformer Pad TF300T (Android 4.2.1)
Updated•9 years ago
|
Attachment #8527129 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-aurora/rev/64f4fbfe44fe
Assignee | ||
Comment 31•9 years ago
|
||
Target Milestone = 36 => status-firefox-37 = fixed.
Reporter | ||
Comment 32•9 years ago
|
||
Verified as fixed in Firefox for Android 35.0a2 (2014-11-27) Device: Asus Transformer Pad TF300T (Android 4.2.1)
Reporter | ||
Comment 33•9 years ago
|
||
Verified as fixed in Firefox for Android 37.0a1 (2014-12-04) Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•