Gecko-side OS locale detection is broken in Android L

VERIFIED FIXED in Firefox 34

Status

()

defect
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: m_kato, Assigned: rnewman)

Tracking

(Blocks 1 bug, {reproducible})

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33- wontfix, firefox34+ verified, firefox35+ fixed, firefox36+ verified, fennec34+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Today I setup Nexus 9 and Firefox for Android, I notice that search plugin keeps EN-US version.  If I test on Android 4.3, this doesn't occur.

This also occurs on Nightly.


- Step
1. Setup Android 5 that locale is Japanese.
2. Install Firefox (or beta) from Google Play.
3. Tap awesome screen, type 'abdef'

- Result
searches are listed as the following

 * Yahoo US
 * Bing
 * Google
 * Amazon.com
 * Twitter
 * Wikipedia

- Expected Result
 * Google
 * Amazon.co.jp
 * bing
 * Twitter
 * Wikipedia
 * Yahoo! Japan (<-- Yahoo Japan icon is different of Yahoo US)
Aaron, do you have Android 5 device?  Although I test on Nexus 5 with L Preview and Nexus 9, this issue occurs.
This sounds like the selected locale isn't "ja", so it's falling back to en-US.

m_kato, can you see what locale is in use via `adb logcat`?

If you install Nightly, it should log something like

  D/GeckoApp( 2019): OS locale is ja, app locale is null

when you launch.
Flags: needinfo?(m_kato)
(In reply to Richard Newman [:rnewman] from comment #2)
> This sounds like the selected locale isn't "ja", so it's falling back to
> en-US.
> 
> m_kato, can you see what locale is in use via `adb logcat`?
> 
> If you install Nightly, it should log something like
> 
>   D/GeckoApp( 2019): OS locale is ja, app locale is null
> 
> when you launch.

D/GeckoApp(24981): OS locale is ja_JP, app locale is null
Flags: needinfo?(m_kato)
I test by zh-TW or fr_FR, this occur too.  Also about:downloads is always EN-US page. (I think that this is same root cause)
[Tracking Requested - why for this release]:

Yes.

I just so happened to unbox a Nexus 9 today, applied the 400MB update and installed Nightly (11/07), switched the device language to Japanese and see this. The browser is translated, but the search engines specific to the locale used are not introduced.

It seems that system wide locale changes are broken, *but our in-browser language selection *works fine*, in that e.g, switching from default to Japanese introduces the appropriate search engines fine.

Tested this across all channels, all affected (including 33.1).
Blocks: android-l
Severity: normal → major
tracking-fennec: --- → ?
Keywords: reproducible
Summary: Search plugin keeps EN-US version even if Android is Japanese locale on Android 5 → Search plugins use EN-US version even if Android system language is set differently in Android 5.0
Aaron: and you're confident that this is limited to Lollipop?

If so, that implies one of two things:

* The non-Java approach that Gecko uses to determine the surrounding locale is broken in L.
* L is giving us different configuration changed events, and somehow that messes up the chain.

Fortunately, the logging in Comment 3 indicates that BrowserLocaleManager and GeckoApp are working together as expected, so this is something we can work around.
Summary: Search plugins use EN-US version even if Android system language is set differently in Android 5.0 → Gecko-side OS locale detection is broken in Android L
Comparing Nightly against my Nexus 5 (4.4.4) against this Nexus 9, the Nexus 9 on Lollipop is affected.

Nexus 9, first-launch

D/GeckoApp(11048): OS locale is ja_JP, app locale is null
D/GeckoBrowserApp(11048): onLocaleReady: ja_JP
I/GeckoConsole(11048): Locale:OS: ja-JP
I/GeckoConsole(11048): New OS locale.
I/GeckoConsole(11048): Default intl.accept_languages = en-US, en
I/GeckoConsole(11048): Setting intl.accept_languages to en-us,ja-jp,en

Nexus 7, first-launch

D/GeckoApp(27892): OS locale is ja_JP, app locale is null
D/GeckoBrowserApp(27892): onLocaleReady: ja_JP
I/GeckoConsole(27892): Locale:OS: ja-JP
I/GeckoConsole(27892): New OS locale.
I/GeckoConsole(27892): Default intl.accept_languages = ja, en-us, en
I/GeckoConsole(27892): Setting intl.accept_languages to ja,ja-jp,en-us,en

There are differences
and yes, the searches are conducted in en-US too as the accept_languages defines it
(In reply to Aaron Train [:aaronmt] from comment #7)

> I/GeckoConsole(11048): Default intl.accept_languages = en-US, en
> I/GeckoConsole(11048): Setting intl.accept_languages to en-us,ja-jp,en

Those two lines mean that the very low level Gecko locale -- what it's picking up from the OS -- is wrong.

That you see ja-jp in there is because our locale-aware code is swizzling in the detected OS locale, even though it's missing from Gecko's list.

In other words: this log shows exactly what would happen if you ran an en-US-only build on a Japanese device.
*sighs sadly*
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
This needs a release note.
Keywords: relnote
Richard, do you have a Nexus device you can put L Preview on?
(In reply to Aaron Train [:aaronmt] from comment #12)
> Richard, do you have a Nexus device you can put L Preview on?

I do. Thanks for checking.
Investigation suggests that nsChromeRegistryChrome::getUILangCountry returns en-US regardless of the OS language setting.
Indeed, something like that is occurring:

11-10 12:32:49.536 I/GeckoConsole(  671): XXX getenv(LANG) en_GB
11-10 12:32:49.536 I/GeckoConsole(  671): XXX UI language en-US


The odd thing is: on my 4.4 device, intl.locale.matchOS is somehow ending up as false immediately after launch.

On 5.0, it's true.

Same build, same circumstances.

That would explain this bug: with matchOS turned off, we can't get the wrong locale from the environment.
Two lines of inquiry, then:

1. Why is matchOS being set to false pre-L? It shouldn't be false unless you've picked a specific UI in Settings.

2. Why is getUILangCountry -- which is to say, nsLocaleService.GetLocaleComponentForUserAgent -- returning the wrong locale (when matchOS is true)?
Component: General → Internationalization
Product: Firefox for Android → Core
Despite 4.4 having apparently a mis-set pref, it does not suffer from this bug (when forced to run the right code path):

I/GeckoConsole(16694): XXX getenv(LANG) es_US
I/GeckoConsole(16694): XXX UI language es-US

Compare to Comment 15.
This looks like the major difference in the locale service:

I/GeckoConsole( 8981): XXX new nsLocale
I/GeckoConsole( 8981): XXX using getenv
I/GeckoConsole( 8981): XXX used getenv en_GB
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX lc_temp C
I/GeckoConsole( 8981): XXX getenv(LANG) en_GB
I/GeckoConsole( 8981): XXX UI language en-US


I/GeckoConsole(20024): XXX new nsLocale
I/GeckoConsole(20024): XXX using getenv
I/GeckoConsole(20024): XXX used getenv es_US
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX lc_temp (null)
I/GeckoConsole(20024): XXX getenv(LANG) es_US
I/GeckoConsole(20024): XXX UI language es-US
This appears to make things work on my Android L device. Is this the right fix, smontagu?
Attachment #8520357 - Flags: review?(smontagu)
Comment on attachment 8520357 [details] [diff] [review]
Ignore 'C' locale when initializing nsLocaleService. v1

Masatoshi, can you look at this? My review queue is overflowing after 2 weeks away.
Attachment #8520357 - Flags: review?(smontagu) → review?(VYV03354)
I don't know much about POSIX locale.
But doesn't it break desktop Linux builds?
I doubt that the C locale yields a useful locale for headers, which is what this class is for, but I might be wrong about where it's used.

Do you know who would know?
setlocale on old android such as kitkat always return null as not implemented.  But Android L will return "C" or "C.UTF-8".

If locale_name has UTF-8, it will return "C.UTF-8".  Otherwise, it returns "C".
Experimentally, we only ever get "C".

When does locale_name have UTF-8?
(In reply to Richard Newman [:rnewman] from comment #25)
> Experimentally, we only ever get "C".
> 
> When does locale_name have UTF-8?

For mb/wc support, Bionic team of Google adds "C", "C.UTF-8", "en_US.UTF8" and "POSIX" locale name support for setlocale.  But C.UTF-8 won't be used from Android framework.

Maybe, we should not trust return value of setlocale on Android...


commit 1abb8bd21d64c2bd21258469b688483f821974d5
Author: Dan Albert <danalbert@google.com>
Date:   Fri Jul 25 11:24:03 2014 -0700

    en_US.UTF-8 is also supported.

    Change-Id: Ic35fad3596dc5e24ee8ae35543a274a471f27bb2

Author: Elliott Hughes <enh@google.com>
Date:   Wed Apr 30 22:03:12 2014 -0700

    Switch to a working UTF-8 mb/wc implementation.

    Although glibc gets by with an 8-byte mbstate_t, OpenBSD uses 12 bytes (of
    the 128 bytes it reserves!).

    We can actually implement UTF-8 encoding/decoding with a 0-byte mbstate_t
    which means we can make things work on LP32 too, as long as we accept the
    limitation that the caller needs to present us with a complete sequence
    before we'll process it.

    Our behavior is fine when going from characters to bytes; we just
    update the source wchar_t** to say how far through the input we got.

    I'll come back and use the 4 bytes we do have to cope with byte sequences
    split across multiple input buffers. The fact that we don't support
    UTF-8 sequences longer than 4 bytes plus the fact that the first byte of
    a UTF-8 sequence encodes the length means we shouldn't need the other
    fields OpenBSD used (at the cost of some recomputation in cases where a
    sequence is split across buffers).

    This patch also makes the minimal changes necessary to setlocale(3) to
    make us behave like glibc when an app requests UTF-8. (The difference
    being that our "C" locale is the same as our "C.UTF-8" locale.)

    Change-Id: Ied327a8c4643744b3611bf6bb005a9b389ba4c2f
(In reply to Makoto Kato (:m_kato) from comment #26)

Thanks for the feedback.

> Maybe, we should not trust return value of setlocale on Android...

This sounds like you'd suggest one of three avenues:

* Applying my current hack (i.e., ignore lc_tmp == "C") only for Android.
* Skip the setlocale call altogether on Android.
* Ignore its return value, again only on Android.

m_kato, emk: could you indicate which of these options is most acceptable, and any particular QA validation steps you need taken?

I'd like to get a fix in ASAP, because this fundamentally breaks Fennec in non-English locales (the majority of our users) on Lollipop.
Flags: needinfo?(m_kato)
Flags: needinfo?(VYV03354)
I have no strong opinion as long as the hack is only on Android。
Flags: needinfo?(VYV03354)
(In reply to Richard Newman [:rnewman] from comment #27)
> (In reply to Makoto Kato (:m_kato) from comment #26)
> 
> Thanks for the feedback.
> 
> > Maybe, we should not trust return value of setlocale on Android...
> 
> This sounds like you'd suggest one of three avenues:
> 
> * Applying my current hack (i.e., ignore lc_tmp == "C") only for Android.
> * Skip the setlocale call altogether on Android.
> * Ignore its return value, again only on Android.
> 
> m_kato, emk: could you indicate which of these options is most acceptable,
> and any particular QA validation steps you need taken?
> 
> I'd like to get a fix in ASAP, because this fundamentally breaks Fennec in
> non-English locales (the majority of our users) on Lollipop.

"C"/"POSIX" locale is often used by generic UNIX.  So if not Android, we should use "en_US" as UNIX standard.

If Android, we should ignore return value of setlocale.  Then we should keep lc_temp is as null.

* Applying my current hack (i.e., ignore lc_tmp == "C") only for Android.

I recommend this.  But you should use __ANDROID__ macro for android only.

I think we should change nsILocaleService using Android Framework / JNI in future.  We have a lot of locale bugs on Android Firefox such as bug 864753.
Flags: needinfo?(m_kato)
This isolates the hack to MOZ_WIDGET_ANDROID only. As I recall that'll exclude B2G.

Tested on a Nexus 7 with Lollipop.
Attachment #8521140 - Flags: review?(VYV03354)
Attachment #8520357 - Attachment is obsolete: true
Attachment #8520357 - Flags: review?(VYV03354)
(In reply to Makoto Kato (:m_kato) from comment #29)

> I think we should change nsILocaleService using Android Framework / JNI in
> future.  We have a lot of locale bugs on Android Firefox such as bug 864753.

That would be great. Bug 936756 shifted 'ownership' of the locale in Fennec to the Java side, so we should have some more leeway than we used to.
Attachment #8521140 - Flags: review?(VYV03354) → review+
We're going to try to get this in the last beta for 34, assuming nothing broke.
tracking-fennec: ? → 34+
Comment on attachment 8521140 [details] [diff] [review]
Ignore 'C' locale when initializing nsLocaleService. v2

Might as well get this in the queue while we wait for the trees to go green and have more manual verification.

Approval Request Comment
[Feature/regressing bug #]:
  None; Android L regression.

[User impact if declined]:
  Users in locales other than en-US will have a partly non-localized browser, including search engines, error pages, and context menus.

[Describe test coverage new/current, TBPL]:
  None; we have no L devices, nor do we have a good way to test locale variations on infra. Manually tested on L.

[Risks and why]: 
  Relatively low risk; this specifically ignores a "C" return from a system call on MOZ_WIDGET_ANDROID only. This is the smallest possible change.

[String/UUID change made/needed]:
  None.
Attachment #8521140 - Flags: approval-mozilla-beta?
Attachment #8521140 - Flags: approval-mozilla-aurora?
Build on fx-team is green, but testing is complicated because those builds are single-locale.

Testing the hard way now.
To verify manually:

* Set OS locale to es-ES.
* Connect with the remote debugger to Nightly.
* Run

  Cc["@mozilla.org/intl/nslocaleservice;1"].getService(Ci["nsILocaleService"]).QueryInterface(Ci["nsILocaleService"]).getLocaleComponentForUserAgent();

* Before fix: returns "en-US".
* After fix: returns "es-ES".

I verified that fx-team build.
Comment on attachment 8521140 [details] [diff] [review]
Ignore 'C' locale when initializing nsLocaleService. v2

We'll get this into beta8 build#3.

Beta+
Aurora+
Attachment #8521140 - Flags: approval-mozilla-beta?
Attachment #8521140 - Flags: approval-mozilla-beta+
Attachment #8521140 - Flags: approval-mozilla-aurora?
Attachment #8521140 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/3ea280ca1f20
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Verified as fixed in build: Firefox 34 beta 8 build 3;
Device: Nexus 7(Android 5.0).
Status: RESOLVED → VERIFIED
No note as this got fixed in time.
relnote-firefox: ? → ---
Duplicate of this bug: 1106500
You need to log in before you can comment on or make changes to this bug.