1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We're currently using a pretty old mechanism for retrieving locale from Windows.

Instead, it seems that we should use GetUserPreferredUILanguages, at least until we start distinguishing between Windows Display Locale and App Locales.

See:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd318139(v=vs.85).aspx

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review commit: https://reviewboard.mozilla.org/r/196124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196124/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

This is long overdue, and hit us in form of bug 1413866 so without any higher level rework, I'd just like to update us to use the proper API for Vista+.

Follows bug 1337067 comment 6.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196124/diff/1-2/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

I verified that the code compiles and fixes the issue reported in bug 1413866 by taking the locale used for windows UI language rather than an obsolete language code that requires some obscure UI to be launched to change.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

https://reviewboard.mozilla.org/r/196124/#review201376

This should work, AFAICS, but needs some cleanup to avoid a memory leak, see below.

::: intl/locale/windows/OSPreferences_win.cpp:20
(Diff revision 2)
>  bool
>  OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
>  {
>    MOZ_ASSERT(aLocaleList.IsEmpty());
>  
> -  WCHAR locale[LOCALE_NAME_MAX_LENGTH];
> +	ULONG numLanguages = 0;

Looks like a `<tab>` managed to sneak in here, please fix.

::: intl/locale/windows/OSPreferences_win.cpp:22
(Diff revision 2)
>  {
>    MOZ_ASSERT(aLocaleList.IsEmpty());
>  
> -  WCHAR locale[LOCALE_NAME_MAX_LENGTH];
> -  if (NS_WARN_IF(!LCIDToLocaleName(LOCALE_SYSTEM_DEFAULT, locale,
> -                                   LOCALE_NAME_MAX_LENGTH, 0))) {
> +	ULONG numLanguages = 0;
> +  DWORD cchLanguagesBuffer = 0;
> +  BOOL hr = GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &numLanguages, NULL, &cchLanguagesBuffer);

Please name this something like `BOOL ok` rather than `hr`, which sounds like it would be a Windows `HRESULT` code (and so it looks wrong in a test like `if (hr) ...` below, as `HRESULT` codes are normally tested using the `SUCCEEDED` or `FAILED` macros).

::: intl/locale/windows/OSPreferences_win.cpp:24
(Diff revision 2)
>  
> -  WCHAR locale[LOCALE_NAME_MAX_LENGTH];
> -  if (NS_WARN_IF(!LCIDToLocaleName(LOCALE_SYSTEM_DEFAULT, locale,
> -                                   LOCALE_NAME_MAX_LENGTH, 0))) {
> -    return false;
> -  }
> +	ULONG numLanguages = 0;
> +  DWORD cchLanguagesBuffer = 0;
> +  BOOL hr = GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &numLanguages, NULL, &cchLanguagesBuffer);
> +  if (hr) {
> +    WCHAR* pwszLanguagesBuffer = new WCHAR[cchLanguagesBuffer];

Rather than doing `new` and `delete` to handle the buffer here, I'd suggest we use an `AutoTArray<WCHAR,64>` (or some reasonable number likely to be larger than usually needed), so that no heap allocation needs to happen except in an extreme case where the list is abnormally long.

Then you can use `SetCapacity(cchLanguagesBuffer)` to reserve the correct amount of space, and `Elements()` to get the pointer to pass to the second `GetUserPreferredUILanguages` call.

::: intl/locale/windows/OSPreferences_win.cpp:33
(Diff revision 2)
>  
> +      // We will only take the first locale from the returned list, because
> +      // we do not support real fallback chains for RequestedLocales yet.
> -  if (CanonicalizeLanguageTag(loc)) {
> +      if (CanonicalizeLanguageTag(loc)) {
> -    aLocaleList.AppendElement(loc);
> +        aLocaleList.AppendElement(loc);
> -    return true;
> +        return true;

This would leak the buffer, as it returns without reaching the `delete`. But that issue will go away if you use an AutoTArray, as suggested above.

::: intl/locale/windows/OSPreferences_win.cpp:36
(Diff revision 2)
> -  if (CanonicalizeLanguageTag(loc)) {
> +      if (CanonicalizeLanguageTag(loc)) {
> -    aLocaleList.AppendElement(loc);
> +        aLocaleList.AppendElement(loc);
> -    return true;
> +        return true;
> -  }
> +      }
> +
> +      delete pwszLanguagesBuffer;

Putting the `delete` here would still leak the buffer in the (unlikely) case that the second `GetUserPreferredUILanguages` call failed. But again, switching to AutoTArray will avoid the whole issue, as it'll automatically go away when it goes out of scope, with no explicit deletion needed.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196124/diff/2-3/

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196124/diff/3-4/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196124/diff/4-5/

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

https://reviewboard.mozilla.org/r/196124/#review201440

Yep, that looks better, thanks.

::: intl/locale/windows/OSPreferences_win.cpp:22
(Diff revision 5)
>  {
>    MOZ_ASSERT(aLocaleList.IsEmpty());
>  
> -  WCHAR locale[LOCALE_NAME_MAX_LENGTH];
> -  if (NS_WARN_IF(!LCIDToLocaleName(LOCALE_SYSTEM_DEFAULT, locale,
> -                                   LOCALE_NAME_MAX_LENGTH, 0))) {
> +  ULONG numLanguages = 0;
> +  DWORD cchLanguagesBuffer = 0;
> +  BOOL ok = GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &numLanguages, NULL, &cchLanguagesBuffer);

Please wrap the over-long line here. Oh, and use `nullptr` in place of `NULL`.

::: intl/locale/windows/OSPreferences_win.cpp:26
(Diff revision 5)
> -  if (NS_WARN_IF(!LCIDToLocaleName(LOCALE_SYSTEM_DEFAULT, locale,
> -                                   LOCALE_NAME_MAX_LENGTH, 0))) {
> -    return false;
> -  }
> -
> -  NS_LossyConvertUTF16toASCII loc(locale);
> +  DWORD cchLanguagesBuffer = 0;
> +  BOOL ok = GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &numLanguages, NULL, &cchLanguagesBuffer);
> +  if (ok) {
> +    AutoTArray<WCHAR, 64> locBuffer;
> +    locBuffer.SetCapacity(cchLanguagesBuffer);
> +    ok = GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &numLanguages, locBuffer.Elements(), &cchLanguagesBuffer);

Also needs wrapping.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196124/diff/5-6/

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

[Tracking Requested - why for this release]: This will fix how we operate when `intl.locale.matchOS` is set to true. It makes us follow OS languages which is important for major deployments (scale of 10k) of Firefox like in bug 1413866.
It's a simple and safe change that would render several extensions that we're losing with Fx57, obsolete [0].

I recognize how late in the cycle it is, but I believe that the value for corporate deployments and simplicity of the patch makes it worth flagging release drivers to consider taking it.


[0] Example: https://addons.mozilla.org/en-US/firefox/addon/locale2mui/

Comment 12

[Justin Dolske [:Dolske]]

What's the risk of this breaking users where the old code gave the desired result, but the new one doesn't?

Can this instead be behind some temporary pref (for 57/58), so that sites can choose which .matchOS behavior they want?

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

> What's the risk of this breaking users where the old code gave the desired result, but the new one doesn't?

Very close to zero. The variable we use now is very old, dated into WinXP API days. Microsoft Intl people told me "omg, you guys still use LCID? That's crazy old!".
In terms of UI, I doubt any reasonable user would dig into:

Settings>Time&Date>Additional Settings>Region>Administration>Change System Regional Settings

which has notes such as "This preference sets locales for old applications from before the dawn of time" (ok, not really, but "from before Unicode") to change a locale and expect Firefox to follow, but would *not* expect Firefox to follow the Windows Display Language when `intl.locale.matchOS` is set to true.

Also, notice that this is affecting only two things:

 - our behavior when intl.locale.matchOS is set to true which is not exposed in any UI
 - our telemetry data on OS locales (which I'm very happy we're fixing because now it's fundamentally flawed)

> Can this instead be behind some temporary pref (for 57/58), so that sites can choose which .matchOS behavior they want?

I can write the patch for separate pref, if you think it's worth it.
:jfkthame - do you see a potential value of keeping the old behavior?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/591c22f2b3bb
Use GetUserPreferredUILanguages for OSPreferences::SystemLocales. r=jfkthame

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Zibi, could you please nominate this patch for uplift to Beta 57? I'd like to understand the risk in terms of what kind of breakage would be notice if the fix caused regressions. 

Hi Jonathan, while this isn't a new bug, I am told that 57 will make the impact far more severe. Is that true? The bar for taking fixes during RC week is to only allow fixes for bugs that could lead to a dot release. Could you weigh in on whether you think this is a must fix for 57? Or is it a low risk, nice to have? Thanks!

Comment 16

[Jonathan Kew [:jfkthame]]

I don't think this is the sort of issue that would drive a dot-release, but it will be a significant annoyance for affected users, and could hurt the acceptance of FF57, so I'd be in favor of fixing it before release if possible.

Given that we're disabling the legacy extensions that may have been used to control language choice in past versions (see bug 1413866 comment 0), I think it's true that this will be a more severe issue for 57 than it was in the past; people will no longer be able to use existing workarounds.

So while this isn't a critical security/stability issue, I think it's worth taking because it is a significant usability regression for a subset of users. The risk here seems very low to me, and in particular the risk to users outside the affected subset should be minimal.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

Approval Request Comment
[Feature/Bug causing the regression]: we never updated our WinAPI calls for retrieving languages from Windows
[User impact if declined]: any user trying to align Firefox locale with Windows locale using `intl.locale.matchOS` will not be able to. This affects dis-proportionally corporate users in multilingual organizations. Until 57 people could write extensions to workaround Firefox limitations. With those APIs gone in 57, this may have a major impact on Fx adoption in those scenarios. 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: 
 - Download Firefox 57 beta 14
 - Install a langpack for any language for Firefox 57 beta 14 [0]
 - Set `intl.locale.matchOS` to true in about:config
 - Set Windows Display Language to match the langpack locale
 - Restart Firefox
 - Firefox should use language matching Windows locale
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It is a simple switch of use of the Windows API in a very limited setup that doesn't affect any regular users (since there's no UI preference to turn intl.locale.matchOS to true)
[String changes made/needed]: None

[0] Langpacks for beta14: https://ftp.mozilla.org/pub/firefox/releases/57.0b14/win64/xpi/

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/591c22f2b3bb

Comment 19

[Jonathan Kew [:jfkthame]]

Ritu: just to follow up here, see bug 1413866 comment 15 for confirmation that this resolves the reported issues for a user dealing with a multi-user, multi-language deployment scenario.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

57 is on m-r now.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

In bug 1413866 comment 15 the user confirms that the patch fixes issue in his deployment scenario. We have a good reason to believe that this is representative of a fix for all multilingual deployments.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8924867 [details]
Bug 1414186 - Use GetUserPreferredUILanguages for OSPreferences::SystemLocales.

The fix was verified on Nightly, thanks Jonathan for weighing in on the risks, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7012da9ebb07 (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/5b953f678f35