1434169 - Add-on list display broken by non-canonical locale codes

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: extensions.json

Somehow, the locale changes in bug 1428698 caused the add-on manager to stop displaying the list of installed extensions on my profile.

(I confirmed bug 1428698 is the cause via a local backout of the change.)

In the Browser Console, Add-on Manager logs messages like:

1517286721923	addons.manager	WARN	Exception calling callback: TypeError: aObj is undefined (resource://gre/modules/addons/XPIProvider.jsm:5739:7) JS Stack trace: [e-mail]:5739:7 < @XPIProvider.jsm:5831:18 < initialize/<@extensions.js:2140:11 < [e-mail]:188:5 < makeSafe/<@AddonManager.jsm:203:25 < promise callback*[e-mail]:229:3 < [e-mail]:3618:12 < [e-mail]:2130:5 < [e-mail]:729:7 < [e-mail]:165:3 < EventListener.handleEvent*@extensions.js:104:1

I have attached the extensions.json file for the profile that displays this issue.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:zibi, you landed the locale changes in bug 1428698.  Maybe you can help here?

Comment 2

[Andrew Swan [:aswan]]

FWIW, it looks like perhaps the selectedLocale getter is returning undefined:
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4851-4898

I suspect that code should probably be updated to use the Locale class instead of messing around with locale strings?

Comment 3

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

I don't think the changes should affect the code. Can you look for when selectedLocale returns undefined and paste the requestedLocales and locales from that call?

The only idea I have at this point is that we became stricter about what we return out of language negotiation. Before that we'd return any locale that we were able to parse out of available, now we canonicalize the output.

That means that if your availableLocales have `sk-sk`, we'll return `sk-SK`. Maybe that's the issue?

Comment 4

[Andrew Swan [:aswan]]

I think the normalization could be the problem.  From the attached extensions.json, the extension "OneNote Web Clipper" uses "en-gb" for instance.  If negotiatedLanguages() returns a normalized "en-GB" then this code will fail (to be precise, it will return undefined, but that would lead to the exception from the original description):
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4893-4894

Normalization is still obviously the right thing to do though, this AOM code needs to be brought up to date.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, I can confirm that "OneNote Web Clipper" is the problem extension here.

This extension has the following locales available:

["en","ja","et","te","fr","id","bg","bn","sv","cs","hu","kn","en-gb","uk","sw","vi","ar","ko","it","es-419","no","zh-cn","lt","hi","ta","th","de","mr","ca","sk","en-us","sr","am","fa","pt-br","zh-tw","ml","ms","ru","pl","sl","lv","fil","hr","fi","pt-pt","ro","gu","el","es","he","da","tr","nl"]

Current Nightly with bug 1428698 applied causes `Services.locale.negotiateLanguages` to return:

en-US

which is not one of the entries in the array above, so `get selectedLocale()` returns undefined.

(Note that I actually have this extension disabled.  Not sure if that is important for the problem.)

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(To be more specific, "en-us" is in the array, but "en-US" is not, so it sounds likes :aswan's guess in comment 4 is correct.)

Comment 7

[Andrew Swan [:aswan]]

Is the fix as simple as:

--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -4854,17 +4854,17 @@ AddonInternal.prototype = {
       return this._selectedLocale;
 
     /**
      * this.locales is a list of objects that have property `locales`.
      * It's value is an array of locale codes.
      *
      * First, we reduce this nested structure to a flat list of locale codes.
      */
-    const locales = [].concat(...this.locales.map(loc => loc.locales));
+    const locales = Intl.getCanonicalLocales([].concat(...this.locales.map(loc => loc.locales)));
 
     let requestedLocales = Services.locale.getRequestedLocales();
 
     /**
      * If en-US is not in the list, add it as the last fallback.
      */
     if (!requestedLocales.includes("en-US")) {
       requestedLocales.push("en-US");

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Andrew Swan [:aswan] from comment #7)
> Is the fix as simple as:
> 
> -    const locales = [].concat(...this.locales.map(loc => loc.locales));
> +    const locales =
> Intl.getCanonicalLocales([].concat(...this.locales.map(loc => loc.locales)));

Maybe something like that, plus also changing the `find` step:

```
this._selectedLocale = this.locales.find(loc => loc.locales.includes(bestLocale));
```

That's searching through the add-on's locales array, which uses the non-canoncial locales, so I don't think it will find anything even with the first change.

Comment 9

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

Yes, it seems to me that what you want is to create a map of non-canonical locale names to canonical ones:

```

// For this locales ['en-us', 'DE-at'] it will give you a map like:
// {
//    'en-US': 'en-us',
//    'de-AT': 'DE-at'
// }
const localesMap = new Map(Intl.getCanonicalLocales(this.locales).map((loc, i) => [loc, this.locales[i]]));

const negotiatedLocale = Services.locale.negotiateLanguages(this.locales, availableLocales, ...)[0];

this._selectedLocale = localesMap.get(negotiatedLocale); // this returns a non-canonicalized key for that locale 

```

I'm a bit torn on this whole strictness. On one hand, I believe there's a value in enforcing BCP47 normalization and I try to follow the "lax on the input, strict on the output" rules which indicates that we should accept `en-us` but return `en-US`.

On the other hand this particular API is supposed to only filter/sort the list of available locales provided to it, so it seems like it adds more work for the caller to also have to handle turning the normalized available locale to the name of the directory (which may be `EN-us`).

:jfkthame - do you have any thoughts on how we should approach language negotiation with the output normalization when the input is lax?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Andrew Swan [:aswan] from comment #4)
> I think the normalization could be the problem.  From the attached
> extensions.json, the extension "OneNote Web Clipper" uses "en-gb" for
> instance.

As I don't already have it, I just tried to install this extension in my Nightly, for testing purposes; interestingly, installation failed, with a doorhanger error message from AMO that tells me the add-on "appears to be corrupt". The browser console shows the same exception as :jryans reported in comment 0.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Yes, it seems to me that what you want is to create a map of non-canonical
> locale names to canonical ones:

That seems a bit of a heavyweight approach here. Can we just relax the search for the object that includes the "best" locale (comment 8) such that it does a case-insensitive comparison? I guess that would mean switching from .includes, which I believe does an exact comparison, to something based on .find, where we could ignore case.

However, while we could fix this here in the add-ons manager code, I'm concerned that there will be other places that run into similar problems, and in some cases we may not find out about them until after we've shipped broken behavior to all our users. :( So I think we should probably reconsider the normalization behavior, and make negotiateLanguages return locale codes in exactly the form they were found in the availableLocales input array. If client code wants normalized codes, that should be something it explicitly requests, rather than a side-effect of the negotiation.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: LocaleService::NegotiateLanguages should return locales in exactly the form they were provided in the availableLocales list, not case-normalized

I think something like this should solve the issue here, if we agree to scrap the automatic normalization. At least this allows the OneNote Web Clipper extension to load in my local build, instead of being rejected as corrupted. I've pushed a try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=28f04916dac142b9893b4879085acf2cced688be, as I expect this to break a few testcases that are currently testing the expected normalization, so they'll need to be adjusted.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Update expectations in test_localeService_negotiateLanguages.js to NOT expect case/underscore normalization

These are the adjustments needed to the negotiateLanguages testcases, if we make this change.

Comment 13

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

Comment on attachment 8947075 [details] [diff] [review]
LocaleService::NegotiateLanguages should return locales in exactly the form they were provided in the availableLocales list, not case-normalized

Review of attachment 8947075 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Thanks Jonathan!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4c512d201e
LocaleService::NegotiateLanguages should return locales in exactly the form they were provided in the availableLocales list, not case-normalized. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7aaf6d8b4a
Update expectations in test_localeService_negotiateLanguages.js to NOT expect case/underscore normalization. r=gandalf

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5e4c512d201e
https://hg.mozilla.org/mozilla-central/rev/0b7aaf6d8b4a

Comment 16

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to marius.santa from comment #16)
> Is manual testing required on this bug? If Yes, please provide some STR and
> the proper webextension(if required), if No set the “qe-verify-“ flag.

I think we are okay without it...  (But perhaps add-ons team will disagree, not sure!)  I can confirm the problem is fixed, at least for my case.