1095298 - Gecko-side OS locale detection is broken in Android L

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Makoto Kato [:m_kato]]

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)

Comment 1

[Makoto Kato [:m_kato]]

Aaron, do you have Android 5 device?  Although I test on Nexus 5 with L Preview and Nexus 9, this issue occurs.

Comment 2

[Richard Newman [:rnewman]]

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.

Comment 3

[Makoto Kato [: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

Comment 4

[Makoto Kato [: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)

Comment 5

[Aaron Train [:aaronmt]]

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

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[Aaron Train [:aaronmt]]

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

Comment 8

[Aaron Train [:aaronmt]]

and yes, the searches are conducted in en-US too as the accept_languages defines it

Comment 9

[Richard Newman [:rnewman]]

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

Comment 10

[Richard Newman [:rnewman]]

*sighs sadly*

Comment 11

[Aaron Train [:aaronmt]]

This needs a release note.

Comment 12

[Aaron Train [:aaronmt]]

Richard, do you have a Nexus device you can put L Preview on?

Comment 13

[Richard Newman [:rnewman]]

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

Comment 14

[Richard Newman [:rnewman]]

Investigation suggests that nsChromeRegistryChrome::getUILangCountry returns en-US regardless of the OS language setting.

Comment 15

[Richard Newman [:rnewman]]

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.

Comment 16

[Richard Newman [:rnewman]]

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

Comment 17

[Richard Newman [:rnewman]]

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.

Comment 18

[Richard Newman [:rnewman]]

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

Comment 19

[Richard Newman [:rnewman]]

Attachment: Ignore 'C' locale when initializing nsLocaleService. v1

This appears to make things work on my Android L device. Is this the right fix, smontagu?

Comment 20

[Simon Montagu :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.

Comment 21

[Masatoshi Kimura [:emk]]

I don't know much about POSIX locale.
But doesn't it break desktop Linux builds?

Comment 22

[Richard Newman [:rnewman]]

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?

Comment 23

[Richard Newman [:rnewman]]

https://tbpl.mozilla.org/?tree=Try&rev=7359778fad24

Comment 24

[Makoto Kato [:m_kato]]

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

Comment 25

[Richard Newman [:rnewman]]

Experimentally, we only ever get "C".

When does locale_name have UTF-8?

Comment 26

[Makoto Kato [:m_kato]]

(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 <[e-mail]>
Date:   Fri Jul 25 11:24:03 2014 -0700

    en_US.UTF-8 is also supported.

    Change-Id: Ic35fad3596dc5e24ee8ae35543a274a471f27bb2

Author: Elliott Hughes <[e-mail]>
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

Comment 27

[Richard Newman [:rnewman]]

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

Comment 28

[Masatoshi Kimura [:emk]]

I have no strong opinion as long as the hack is only on Android。

Comment 29

[Makoto Kato [:m_kato]]

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

Comment 30

[Richard Newman [:rnewman]]

Attachment: Ignore 'C' locale when initializing nsLocaleService. v2

This isolates the hack to MOZ_WIDGET_ANDROID only. As I recall that'll exclude B2G.

Tested on a Nexus 7 with Lollipop.

Comment 31

[Richard Newman [:rnewman]]

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

Comment 32

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/3ea280ca1f20

Comment 33

[Richard Newman [:rnewman]]

We're going to try to get this in the last beta for 34, assuming nothing broke.

Comment 34

[Richard Newman [:rnewman]]

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.

Comment 35

[Richard Newman [:rnewman]]

Build on fx-team is green, but testing is complicated because those builds are single-locale.

Testing the hard way now.

Comment 36

[Richard Newman [:rnewman]]

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 37

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8521140 [details] [diff] [review]
Ignore 'C' locale when initializing nsLocaleService. v2

We'll get this into beta8 build#3.

Beta+
Aurora+

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ea280ca1f20

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/43fd2720be09

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db92e37c83a2

Comment 41

[Flaviu Cos]

Verified as fixed in build: Firefox 34 beta 8 build 3;
Device: Nexus 7(Android 5.0).

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

No note as this got fixed in time.