Closed Bug 1378501 Opened 7 years ago Closed 7 years ago

Geolocation permission popup always shows in English

Categories

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

All
Android
defect

Tracking

(fennec+, firefox55- wontfix, firefox56blocking verified, firefox57+ verified)

RESOLVED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 - wontfix
firefox56 blocking verified
firefox57 + verified

People

(Reporter: mkaply, Assigned: rnewman)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

If you switch your default language of your device to Chinese and then go to a site that requires geolocation permissions (mapquest.com), the request is in English.

This happens even if you completely close the browser and restart it.
I switched my OS to German, and it showed in German, then I switched my OS to Chinese and Firefox is in Chinese, but the popup is still in German.

I then did a Force stop, clear cache and the popup appears in English even though the browser is in Chinese.
When I navigate to

chrome://browser/locale/browser.properties

it's completely in English, even though the rest of the browser is in Chinese.
Changing the component to Locale switching and selection.
tracking-fennec: --- → ?
Component: General → Locale switching and selection
OS: Unspecified → Android
Hardware: Unspecified → All
When I mannualy set the language to Chinese( Setting -> General -> Language -> System Default=>Chinese), the door hanger will show Chinese correctly.

Although it looks like a frontend bug, doorHanger's content/config is from Gecko. we may need to investigate more.
[triage@0712] Nevin to check it from FE side first. P2+
tracking-fennec: ? → +
Flags: needinfo?(cnevinchen)
I feels like it's a P2 bug. We need this to be fixed but we have no time to fix it in next release. I'll keep the NI to remember to do this. But anyone can take it if he/she has time.
Priority: -- → P2
AT&T has escalated this issue. They'd like to see something in Firefox 56.
Hi Joe, Wesly
Please help me prioritize this.
Thanks!
Flags: needinfo?(wehuang)
Flags: needinfo?(jcheng)
let's decide during sprint planning
Flags: needinfo?(jcheng)
added into tomorrow's planning agenda.
Flags: needinfo?(wehuang)
ni Max since he is looking at it
Flags: needinfo?(cnevinchen) → needinfo?(max)
See Also: → 1389709
Relevent info (based on looking at bug 1389709)

Despite the code here:

http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1720

intl.locale.os is not being updated when the the system locale is changed.

It only gets updated on a restart of the browser.
I can't reproduce it in the current Nightly. I switched to Polish and opened `maps.google.com` which showed the request in polish. The `chrome://browser/locale/browser.properties` is also in Polish.
> I can't reproduce it in the current Nightly. I switched to Polish and opened `maps.google.com` which showed the request in polish. The `chrome://browser/locale/browser.properties` is also in Polish.

Interesting. I'll do some more testing. If we could figure out what fixed this, that would be very helpful.

See also:

bug 1389709

Maybe wherever broke that fixed this.
I still see this on nightly.

New profile.
Navigate to mapquest.com.
Geolocation popup is in English. (click outside to dismiss).
Go to settings, change system to other language (I chose Japanese).
Go back to Firefox.
Notice that all of the UI (menus, etc.) is in Japanese.
Reload on mapquest.com.
Geolocation popup is in English.
Force stop of Firefox.
Restart.
Go to mapquest.com.
Popup is in English
Force stop.
Clear data.
Restart
Go to mapquest.com
Popup is in Japanese
Go back to system settings.
Change to English
Go to mapquest and reload.
Popup is in Japanese.

I believe all of this stems from the fact that intl.locale.os is not updated correctly when the system locale is changed.

If you go to about:config when you are in English but seeing the Japanese popup, intl.locale.os is set to ja-JP.

By clearing data, you are forcing Firefox to set a new value for intl.locale.os.
And geolocation is just one symptom.

Easiest way to see the problem is navigate to chrome://global/locale/intl.properties.

It's the ja version even though the Android UI is in English.
We should have tracked this for 55 and 56 and tried to fix it for 55. It looks like we are not correctly detecting the locale or not using the correct locale for some important things. We should fix this for 56.
Maybe it broke here? https://bugzilla.mozilla.org/show_bug.cgi?id=1354055 
gandalf, rnewman, maybe you can help? 

I wonder if a bunch of the recent bugs in this component may be from the same root cause. (https://bugzilla.mozilla.org/buglist.cgi?component=Locale%20switching%20and%20selection&product=Firefox%20for%20Android&bug_status=__open__)
Flags: needinfo?(rnewman)
Flags: needinfo?(gandalf)
I can see a few issues in the LocaleService code.

* LocaleService.cpp doesn't flush the prefs file after setting. Prior to 56 (where we flush prefs every 500msec) that could cause weird stuff to happen. I suggest that LocaleService flush prefs after write.

* This comment is wrong. I don't think it's possible to do better than we do now.

//XXX: This pref is used only by Android and we use it to emulate
//     retrieving OS locale until we get proper hook into JNI in bug 1337078.
#define ANDROID_OS_LOCALE_PREF "intl.locale.os"

* This might be wrong:

http://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#91

OSPreferences appears to cache the system locale, and I'm not sure it updates it.

The locale service listens for the Android OS locale pref and refreshes everything, but as far as I can see all that happens is it ends up here, to fetch the system locale:

http://searchfox.org/mozilla-central/source/intl/locale/OSPreferences.cpp#39

which doesn't look like it reads the value of the pref!

On Android the OS locale should be exclusively controlled by that "Locale:OS" message -- the LocaleService code does not have sufficient integration with the application object to figure this out.

If this is a blocker for 56, I can set up an Android dev environment and take a look this week. Gandalf, let me know if you'd rather dig in.
Flags: needinfo?(rnewman)
I'm not sure if this specifically should block but if it is part of a bigger pattern of something going wrong with locales (like in bug 1389709) then it should.
I'll take it and try to investigate tomorrow.

I'm a bit surprised I can't reproduce it, but I'll try on another device on a custom build.

> On Android the OS locale should be exclusively controlled by that "Locale:OS" message -- the LocaleService code does not have sufficient integration with the application object to figure this out.

I'm pretty sure that that's exactly how it worked after I split it into client/service precisely for this use case. Something must have regressed since then, and I'll try to catch it and add tests.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #21)

> I'm a bit surprised I can't reproduce it, but I'll try on another device on
> a custom build.

You should be able to narrow it down pretty fast with logging in GeckoApplication's change handlers, in browser.js where we handle the message, and in LocaleService and OSPreferences where we try to dig those out. If you can reproduce it, of course :D

I give 50-50 odds that all of this code is working fine, and the problem is in the strings system…

> Something must have regressed
> since then, and I'll try to catch it and add tests.

:thumbsup:

Let me know if I can help.
Thanks a lot, Zibi and Richard!
Flags: needinfo?(max)
I just tried Firefox 50 and it has the same problem.
See Also: → 1390822
I started investigating.

First results are:

1) LocaleService does correctly follow Fennec as a client and `Services.locale.getAppLocalesAsLangTags()` returns the correct fallback chain after updates. That's good. The rest should be easier to fix :)

2) the pref intl.locale.os is not updated right after language change

3) The XUL documents seem to get reloaded with the new locales (probably because Chrome Registry follows LocaleService nicely)

4) The JS API (getStringBundle) doesn't - probably because of some cache.

The fix shouldn't be too hard :) (famous last words)
> 2) the pref intl.locale.os is not updated right after language change

Ahhhh! It's actually very good that this doesn't change. Because the OS locale didn't change. :)

All is good :)

On to the (4).
I misunderstood the bug originally. Changing the locale in Firefox Settings does work as intended.

But changing the OS locale does not trigger the `intl.locale.os` pref update which in result doesn't update the LocaleService.

So, actually, back to (2).


In my testing, this line is not fired on locale change: http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/mobile/android/chrome/content/browser.js#1725

Which means that the problem is probably located in Java. If that's the case, it's beyond my expertise to dig into the Java code.

NI on :rnewman to verify if something changed in Java's code firing "Locale:OS".
Flags: needinfo?(rnewman)
Copying and pasting from IRC:

12:53:36 
<@rnewman> OK, so next step is to add debugging in GeckoApplication
12:54:10 
<@rnewman> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#144
12:55:01 
<@rnewman> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1117
12:56:01 
<@rnewman> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2278
12:57:17 
<@rnewman> One of those three spots should be hit. My suspicion is that there's a gap there: GeckoApp.onCreate won't be called twice, but we don't do any locale checking in onResume.

I'll charge an Android device and see how this looks.
But, also, bug 1397925 is needed once intl.locale.os starts reacting properly :)
Assignee: gandalf → nobody
Status: ASSIGNED → NEW
Depends on: 1397925
Attached patch Patch. v1 (obsolete) — Splinter Review
Doing this the old-fashioned way!

Flagging you for feedback, Zibi, mainly to look at my conclusions in the commit message:

---
This ensures that intl.locale.os is always set, even if the system locale changes
while Fennec is in the background.

This commit also restores Strings.flush() calls that are necessary to have Fennec's
non-Java UI reflect locale changes.

With this commit, the geolocation popup still doesn't behave correctly: when the
locale system is set to match OS locale, although the pref is set the locale doesn't
change. This applies in two scenarios: on first run (the popup is always English)
and when the locale changes at runtime (the popup uses an earlier OS locale).

Bug 1397925 should complete the fix.
---
Assignee: nobody → rnewman
Attachment #8905984 - Flags: feedback?(gandalf)
With this patch and the one I just reviewed in Bug 1397925, my Fennec build shows UI and the geo popup correctly as system locale and in-app locale change.
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
Blocks: 1398209
Comment on attachment 8905984 [details] [diff] [review]
Patch. v1

Your patch together with the patch from bug 1397925 (which I just landed) fixes the issue.

I was able to switch OS locale and after refreshing mapquest the doorhanger showed in the correct locale.
Attachment #8905984 - Flags: feedback?(gandalf) → feedback+
Comment on attachment 8905984 [details] [diff] [review]
Patch. v1

Michael, could you redirect this to someone else if you won't be able to get to it by EOD Monday?
Attachment #8905984 - Flags: review?(michael.l.comella)
Comment on attachment 8906703 [details]
Bug 1378501 - Tell Gecko when the OS locale changes, even when backgrounded.

https://reviewboard.mozilla.org/r/178440/#review183498

I'm not too familiar with the locale code but this lgtm.

To summarize, the two major changes I see here:
1) Clear string cache (`flush()`) in browser.js when receiving `Locale:*` messages (so new strings will be loaded when accessed)
2) When Fennec is running in the background, notify Gecko of OS locale changes. The code to update the OS locale on startup (`GeckoApp.onCreate`) already exists.

This both sound like reasonable steps to fix this bug though, given that I don't know the code that well, I can't guarantee we're not missing anything else (e.g. how this patch needs bug 1397925 to fix the issue).

::: mobile/android/base/java/org/mozilla/gecko/BrowserLocaleManager.java:314
(Diff revision 1)
>  
>          res.updateConfiguration(config, null);
>      }
>  
>      private SharedPreferences getSharedPreferences(Context context) {
> +        // We should be using per-profile prefs here, because we're tracking against

nit: is there an issue for this bug? Can we include the bug number in the comment?

::: mobile/android/chrome/content/browser.js:1747
(Diff revision 1)
>          let appLocale = this.getUALocalePref();
>  
>          this.computeAcceptLanguages(languageTag, appLocale);
> +
> +        // Rebuild strings, in case we're mirroring OS locale.
> +        Strings.flush();

fwiw, the [IDL definition](http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/intl/strres/nsIStringBundle.idl#87) mentions:

```
at some point, we might want to make this flush all the bundles,
because any bundles that are floating around when the locale changes
will suddenly contain bad data
```

Looking at [the implementation](http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/intl/strres/nsStringBundle.cpp#542), this caveat isn't mentioned. Perhaps it was fixed but the comment wasn't updated?

Anyway, perhaps this is likely beyond the scope of this patch.

::: mobile/android/chrome/content/browser.js:1752
(Diff revision 1)
> +        Strings.flush();
>          break;
>        }
>  
>        case "Locale:Changed": {
> +        console.log("Gecko Locale:Changed: " + data);

nit: This log doesn't seem very useful:

`GeckoConsole: Gecko Locale:Changed: [object Object]`

That being said, I'm concerned that printing an object could print sensitive data – is there something specific you're trying to get out of this object? Could we log that instead?
Attachment #8906703 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8905984 [details] [diff] [review]
Patch. v1

This duplicates the mozreview request - cancelling r? and obsoleting.
Attachment #8905984 - Attachment is obsolete: true
Attachment #8905984 - Flags: review?(michael.l.comella)
OK. Should/can we uplift this along with the patch in bug 1397925 to beta 56?  It will miss beta 11 but there is one more build for mobile next Monday.
Flags: needinfo?(rnewman)
Flags: needinfo?(gandalf)
(In reply to Michael Comella (:mcomella) from comment #35)

> I'm not too familiar with the locale code but this lgtm.

Thanks for the review!


> To summarize, the two major changes I see here:
> 1) Clear string cache (`flush()`) in browser.js when receiving `Locale:*`
> messages (so new strings will be loaded when accessed)

Yup. This is a simple regression -- those lines were removed by accident.


> This both sound like reasonable steps to fix this bug though, given that I
> don't know the code that well, I can't guarantee we're not missing anything
> else (e.g. how this patch needs bug 1397925 to fix the issue).

That bug makes the Gecko-side locale stuff respond to changes in the pref. Without that we'll update the pref… and then we'll still get the old locale's strings until we restart.


> >      private SharedPreferences getSharedPreferences(Context context) {
> > +        // We should be using per-profile prefs here, because we're tracking against
> 
> nit: is there an issue for this bug? Can we include the bug number in the
> comment?

Bug 940575, Bug 873166. This is old stuff. :/


> That being said, I'm concerned that printing an object could print sensitive
> data – is there something specific you're trying to get out of this object?
> Could we log that instead?

I'll trim down the logging before landing; I figured it would be helpful for review.
Pushed by rnewman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ef39e821b67
Tell Gecko when the OS locale changes, even when backgrounded. r=mcomella
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> OK. Should/can we uplift this along with the patch in bug 1397925 to beta
> 56?  It will miss beta 11 but there is one more build for mobile next Monday.

If it doesn't burn Nightly, then I think so.
Flags: needinfo?(rnewman)
Comment on attachment 8906703 [details]
Bug 1378501 - Tell Gecko when the OS locale changes, even when backgrounded.

https://reviewboard.mozilla.org/r/178440/#review183526
Attachment #8906703 - Flags: review?(gandalf) → review+
Comment on attachment 8906703 [details]
Bug 1378501 - Tell Gecko when the OS locale changes, even when backgrounded.

Approval Request Comment
[Feature/Bug causing the regression]:
  Unclear.

[User impact if declined]:
  OS locale switches won't be correctly picked up until the next cold start.

[Is this code covered by automated tests?]:
  No; our test infrastructure doesn't allow for OS locale switches.

[Has the fix been verified in Nightly?]:
  Not yet; just autolanded. Flippin' the flags while I have the tab open!

[Needs manual test from QE? If yes, steps to reproduce]: 
  Yes. See earlier comments in the bug.

[List of other uplifts needed for the feature/fix]:
  Bug 1397925.

[Is the change risky?]:
  Not particularly.

[Why is the change risky/not risky?]:
  It reports routine information in one additional place. The additional risk comes from two things: 
    1. It's possible for some of these code paths to get into loops due to Android configuration changes re-propagating. We don't touch those paths in this commit, but there is some voodoo here.
    2. We flush string bundles again as a result of these changes, and so it's possible that unexpected perf impact could occur.

[String changes made/needed]:
  None.
Attachment #8906703 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/0ef39e821b67
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
mkaply, could you re-test this (and Bug 1389709 while you're there) in the next Nightly?

This merged 6 hours ago, so that might be today's Nightly.
Flags: needinfo?(mozilla)
I cannot reproduce this bug anymore in today's m-c build.
Flags: needinfo?(gandalf)
Great. I will uplift the patches from here and bug 1397925 then. Thanks for the fast turnaround Zibi!
Comment on attachment 8906703 [details]
Bug 1378501 - Tell Gecko when the OS locale changes, even when backgrounded.

Verified in nightly, fix for regression from 55 that affects several other bugs.
Attachment #8906703 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I just verified that all is good in nightly for this bug.

en-CA is still mapping to en-ZA though (bug 1389709).
Flags: needinfo?(mozilla)
Verified as fixed on the latest beta build 56.0b13 by following the reproduction steps @ comm 1 an d testing it on several languages. Now, the geolocation prompt switches automatically with the current language setting, no restart required.
This issue was tested on a Nexus 6P with Android 8.0
Marking it verified on Nightly also based on Mike's previous comment
Product: Firefox for Android → Firefox for Android Graveyard

Removing regressionwindow-wanted keyword because this bug has been resolved.

Removing regressionwindow-wanted keyword because this bug has been resolved.

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