1344445 - Add LocaleService::GetRequestedLocales

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

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

So, there's an interesting consequence of part of behavior described in bug 1344355.

It seems to me (naive analysis), that the chrome registry is not just picking a locale that is provided in `general.useragent.locale`, but it does some sort of verification that this resource is actually available.

That is part of the behavior described in bug 1344355 (when chromeRegistry is called before the addon is registered, it knows that it should be `ja-JP-mac`, but since the resource for the locale is not registered yet, it falls back on `en-US` initially).

But it also adds a bit more complexity to testing.

When we're testing locales and addons, we can't just update `general.useragent.locale` and get the app locale. So the result is that at least in those two places we're manually *emulating* locale selection:

* http://searchfox.org/mozilla-central/source/toolkit/modules/Locale.jsm
* http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_locale.js

* http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm
* http://searchfox.org/mozilla-central/source/addon-sdk/source/test/test-l10n-locale.js

In both cases we do sth like this:

1) We want to test that if we switch locale to X, we get different values
2) So we set manually the `general.useragent.locale`
3) Next, we workaround the locale selection from nsChromeRegistry because it would return `en-US` since we just changed locale, we didn't register data, so we recreate the whole language selection logic from nsChromeRegistry in Locale.jsm and AddonManager.jsm but ignoring the "verify if the data exists" part
4) Profit, the tests work since our language selection works differently than nsChromeRegistry.

This feels like a very risky code flow, and it also gets in the way of me trying to unify all callsites for app locale in LocaleService.

I see two possible fixes:

1) We add a param to `nsChromeRegistryChrome::GetSelectedLocale` to skip the verification.

Then I'll add the same flag to LocaleService::GetAppLocale(s) and Locale.jsm and AddonsManager.jsm will use that flag to retrieve the locale without verifying the data.

2) We add a way for tests to mock registering the addon data so that GetSelectedLocale actually can return the new locale.

I'd prefer this approach, but I have no idea how possible it is.

Comment 1

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

:mossop, what do you think? Do you know who should take a look at this?

Comment 2

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

Actually, there's one more way we can approach it.

Basically, using the Language Negotiation terminology, we can say that what nsChromeRegistryChrome (and LocaleService::GetAppLocales) returns is "supported locales".

On the other hand those APIs:
 * Locale.jsm
 * AddonsManager.jsm
 * DirectoryLinksProvider.jsm
only change and test what we call "requested locales". Updating "general.useragent.locale" is just that - you update what locales the user requests. Then ChromeRegistryChrome "negotiates" it against the locales that are registered ("available locales") and returns the "supported".

So, what we could do, at least for now, is to move the logic used in those three APIs into LocaleService::GetAppRequestedLocales.

This would allow us to remove all the direct callsites to the prefs, unify and simplify the behavior.

Later, we could migrate that to use negotiated locales (because we don't want addons to use a different locale than the app if possible), but I think that it would be a good first move.

:pike, :mossop, :jfkthame, thoughts?

Comment 3

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

Attachment: Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 4

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

So, after looking at it a bit more, I'm getting more confident that my last idea is the right path.

I'll keep the NI so that guys have time to respond, but here's the patch that adds the LocaleService::GetRequestedLocales and an observer for intl:requested-locales-changed.

This will allow me to replace logic to retrieve those locales from:

* addon-sdk/source/lib/sdk/l10n/locale.js
* browser/components/dirprovider/DirectoryProvider.cpp
* browser/components/distribution.js
* browser/extensions/pdfjs/content/PdfStreamConverter.jsm
* browser/modules/DirectoryLinksProvider.jsm
* mobile/android/chrome/content/browser.js
* mobile/android/components/DirectoryProvider.js
* toolkit/components/downloads/ApplicationReputation.cpp
* toolkit/components/search/nsSearchService.js
* toolkit/modules/Locale.jsm
* toolkit/mozapps/extensions/AddonManager.jsm
* toolkit/mozapps/extensions/nsBlocklistService.js
* toolkit/xre/nsAppRunner.cpp

Yes, at the moment we're doing a better or worse attempt to retrieve user selected locale separately in all those places.

I'm even willing to do some limited work on understanding which of those should really be asking for requested, and which should be asking for AppLocale, but since there's a lot of tests involved, I may not want to spend too much time on this at the moment.

Also, this will normalize the interactions between LocaleService and ChromeRegistry, where ChromeRegistry will ask LocaleService for what locales user wants, "negotiate" them against locales it has, and inform LocaleService on what locales the app has been negotiated to.

This is fairly close logic to what I'll want to get in LocaleService+L10nRegistry future, so I'm happy we can get close.

Comment 5

[Dave Townsend [:mossop]]

I'm not spotting any obvious red flags and I suspect you've researched this way more than I would have so this seems fine to me.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/117718/#review119664

::: intl/locale/LocaleService.cpp:13
(Diff revision 1)
> +#include "nsIPrefService.h"
> +#include "nsIPrefBranch.h"
> +#include "nsIPrefLocalizedString.h"

If you `#include "mozilla/Preferences.h"` instead, you can use its static convenience methods to simplify access to the prefs in the code below.

::: intl/locale/LocaleService.cpp:56
(Diff revision 1)
>  LocaleService::GetInstance()
>  {
>    if (!sInstance) {
>      sInstance = new LocaleService();
> +
> +    // We're going to observe for requsted languages changes which come

typo, "requested"

::: intl/locale/LocaleService.cpp:58
(Diff revision 1)
> +    nsCOMPtr<nsIPrefBranch> prefs = do_GetService("@mozilla.org/preferences-service;1");
> +    if (prefs)
> +    {
> +      prefs->AddObserver(MATCH_OS_LOCALE_PREF, sInstance, false);
> +      prefs->AddObserver(SELECTED_LOCALE_PREF, sInstance, false);
> +    }

There's a convenience function `Preferences::AddWeakObservers()` that I think you can use here; no need to explicitly `do_GetService` etc.

::: intl/locale/LocaleService.cpp:65
(Diff revision 1)
> +    {
> +      prefs->AddObserver(MATCH_OS_LOCALE_PREF, sInstance, false);
> +      prefs->AddObserver(SELECTED_LOCALE_PREF, sInstance, false);
> +    }
>      ClearOnShutdown(&sInstance);
> +    // Remove observers on shutdown?

Probably should do that in the `~LocaleService()` destructor?

::: intl/locale/LocaleService.cpp:80
(Diff revision 1)
> +LocaleService::GetRequestedLocales(nsTArray<nsCString>& aRetVal)
> +{

Shouldn't we be caching the result of this, similarly to `GetAppLocales()`, so we only do the work the first time? (And clear the cached result on pref changes.)

::: intl/locale/LocaleService.cpp:82
(Diff revision 1)
>  }
>  
> +bool
> +LocaleService::GetRequestedLocales(nsTArray<nsCString>& aRetVal)
> +{
> +  nsCString locale;

use `nsAutoCString` for local variable

::: intl/locale/LocaleService.cpp:87
(Diff revision 1)
> +  bool matchOSLocale;
> +  nsresult rv = prefs->GetBoolPref(MATCH_OS_LOCALE_PREF, &matchOSLocale);

Use the convenience function

    Preferences::GetBool(...)

instead of `do_GetService` etc.

::: intl/locale/LocaleService.cpp:103
(Diff revision 1)
> +  nsCOMPtr<nsIPrefLocalizedString> prefString;
> +  rv = prefs->GetComplexValue(SELECTED_LOCALE_PREF,
> +      NS_GET_IID(nsIPrefLocalizedString),
> +      getter_AddRefs(prefString));

Use convenience functions `Preferences::GetLocalizedCString()` and/or `GetCString()` to make all this more concise.

::: intl/locale/LocaleService.cpp:190
(Diff revision 1)
>  }
> +
> +NS_IMETHODIMP
> +LocaleService::GetRequestedLocales(uint32_t* aCount, char*** aOutArray)
> +{
> +  AutoTArray<nsCString, 100> requestedLocales;

100 seems excessively generous for the likely number of requestedLocales; for now we only actually support 1, but even when we extend that, something like 16 should be more than sufficient for most people?

::: intl/locale/LocaleService.cpp:198
(Diff revision 1)
> +  *aCount = requestedLocales.Length();
> +  *aOutArray = static_cast<char**>(moz_xmalloc(*aCount * sizeof(char*)));
> +
> +  for (uint32_t i = 0; i < *aCount; i++) {
> +    (*aOutArray)[i] = moz_xstrdup(requestedLocales[i].get());
> +  }

This looks familiar ... it's also in `GetAppLocales()`. So as I don't like repeating code, how about factoring out a local helper

    static char**
    CreateOutArray(const nsTArray<nsCString>& aArray)
    {
      uint32_t n = aArray.Length();
      char** result = static_cast<char**>(moz_xmalloc(n * sizeof(char*)));
      for (uint32_t i = 0; i < n; i++) {
        result[i] = moz_xstrdup(aArray[i].get());
      }
      return result;
    }

and then just saying

    *aOutArray = CreateOutArray(requestedLocales);
    *aCount = requestedLocales.Length();

here, and similarly for appLocales.

::: intl/locale/mozILocaleService.idl:53
(Diff revision 1)
> +   * The result is a sorted list of valid locale IDs and it should be
> +   * used as a requestedLocales input list for language negotiation.

Are the locale IDs returned here actually guaranteed to be valid? (Not AFAICS.)

Also, "a sorted list" sounds like they might be alphabetical or something. Maybe "an ordered list" would be better, as the order is significant but may not be "sorted" in any usually-recognized sense.

Comment 7

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 8

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/117718/#review119664

> Probably should do that in the `~LocaleService()` destructor?

When I try to create a non-virtual destructor the compiler errors. Do I still need it if I use WeakObserver?

> Shouldn't we be caching the result of this, similarly to `GetAppLocales()`, so we only do the work the first time? (And clear the cached result on pref changes.)

I do want to eventually cache it, but after getting hit with the bug in how we handle info from chrome registry I now want to first unify the behavior and then optimize it.

The current callsites that I'm going to replace with a call to this method do not cache anything so at worst we'll be on par. I'll file a follow-up to later cache the result.

Is that ok?

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Comment on attachment 8844181 [details]
> Bug 1344445 - Add LocaleService::GetRequestedLocales.
> 
> https://reviewboard.mozilla.org/r/117718/#review119664
> 
> > Probably should do that in the `~LocaleService()` destructor?
> 
> When I try to create a non-virtual destructor the compiler errors. Do I
> still need it if I use WeakObserver?

I think so, IIUC. Otherwise, there's a risk that (depending on the order in which things get shut down) the Observer service will still have its references after the  LocaleService has been destroyed; and if it were to try and send a notification at that point, we'd have a use-after-free, leading to all kinds of potential badness.

What you want to do is still declare the destructor as virtual in the header, but remove the empty body that it currently has there, so:

    virtual ~LocaleService();

and instead, provide a non-empty body in the .cpp file:

    LocaleService::~LocaleService()
    {
      Preferences::RemoveObservers(sInstance, kObservedPrefs);
    }

I believe that should build and work fine.

> > Shouldn't we be caching the result of this, similarly to `GetAppLocales()`, so we only do the work the first time? (And clear the cached result on pref changes.)
> 
> I do want to eventually cache it, but after getting hit with the bug in how
> we handle info from chrome registry I now want to first unify the behavior
> and then optimize it.
> 
> The current callsites that I'm going to replace with a call to this method
> do not cache anything so at worst we'll be on par. I'll file a follow-up to
> later cache the result.
> 
> Is that ok?

OK, we can do that for now.

Comment 10

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 11

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

Ok, I added a few tests to make sure that we're firing the right event and returning the right pref.

One thing I noticed is that when I used addWeakObservers, it didn't fire anything. I switched to addStrongObservers and now it works.
Let me know if that works!

I think the patch is ready.

Comment 12

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> Ok, I added a few tests to make sure that we're firing the right event and
> returning the right pref.
> 
> One thing I noticed is that when I used addWeakObservers, it didn't fire
> anything. I switched to addStrongObservers and now it works.

Oh, right - that would only work if the object implements nsISupportsWeakReference. The Preferences::AddWeakObservers call will have been returning an NS_ERROR_INVALID_ARG error, but we didn't check that. :(

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

https://reviewboard.mozilla.org/r/117718/#review120006

One last thing, please check the return values of the AddObservers and RemoveObservers calls, and MOZ_ASSERT if they don't succeed. (If that happens in a release build, we can just ignore the failures, but it'd be nice to alert us if they ever fail in debug builds -- as the WeakObservers version would have done -- as that would probably indicate something is broken.)

::: intl/locale/mozILocaleService.idl:116
(Diff revision 4)
> +
> +  /**
> +   * Returns a list of locales that the user requested the app to be
> +   * localized to.
> +   *
> +   * The result is an orderd list of locale IDs which should be

s/orderd/ordered/

Comment 15

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 16

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

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> Comment on attachment 8844181 [details]
> Bug 1344445 - Add LocaleService::GetRequestedLocales.
> 
> https://reviewboard.mozilla.org/r/117718/#review120006
> 
> One last thing, please check the return values of the AddObservers and
> RemoveObservers calls, and MOZ_ASSERT if they don't succeed. (If that
> happens in a release build, we can just ignore the failures, but it'd be
> nice to alert us if they ever fail in debug builds -- as the WeakObservers
> version would have done -- as that would probably indicate something is
> broken.)

I tried doing that,

Added

nsresult rv = Preferences::RemoveObservers(...);
MOZ_ASSERT(NS_SUCCEEEDED(rv), "...");

and it crashed on shutdown.

After searching through searchfox it seems that no one tests the result of AddObservers/RemoveObservers so it may not work as designed.

Do you want me to do something else?

For now I added assertion on AddObservers only since it seems to work.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #16)
> Added
> 
> nsresult rv = Preferences::RemoveObservers(...);
> MOZ_ASSERT(NS_SUCCEEEDED(rv), "...");
> 
> and it crashed on shutdown.

Ah, I guess that probably means the observer service had already shut down.

Fine - let's not worry about checking there.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/288bc41a2ffe
Add LocaleService::GetRequestedLocales. r=jfkthame

Comment 19

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

Backed out for bustage due to unused variable rv at LocaleService.cpp:70:14:

https://hg.mozilla.org/integration/autoland/rev/d960dec4b66a8362f00af5fe9a1e3fde1fbe988d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=288bc41a2ffe3b44e494d3a3b592cc23efa25394&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83226247&repo=autoland

[task 2017-03-11T17:54:35.281681Z] 17:54:35     INFO -  Warning: -Wunused-variable in /home/worker/workspace/build/src/modules/woff2/src/woff2_dec.cc: unused variable 'dst_offset'
[task 2017-03-11T17:54:35.281914Z] 17:54:35     INFO -  /home/worker/workspace/build/src/modules/woff2/src/woff2_dec.cc:1149:12: warning: unused variable 'dst_offset' [-Wunused-variable]
[task 2017-03-11T17:54:35.282054Z] 17:54:35     INFO -     uint64_t dst_offset = first_table_offset;
[task 2017-03-11T17:54:35.282197Z] 17:54:35     INFO -              ^

Comment 20

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

Comment on attachment 8844181 [details]
Bug 1344445 - Add LocaleService::GetRequestedLocales.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f892ef1e7f51
Add LocaleService::GetRequestedLocales. r=jfkthame

Comment 22

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

Thanks Aryx, sorry for trouble.

I got a green try opt build with a fix and relanded on autoland.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f892ef1e7f51