1086527 - PluralForm.jsm calls PluralForm.get.caller which is not allowed in "strict mode";

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

If we don't have a rule for this locale it should default to English and not to Chinese, this will throw a TypeError.
For exaple in bug 1086238 for CSB locale it won't show the "showAllSearchResults" lable in AddonsManager when searching for addons because the rule for getting the plural form is broken.

https://hg.mozilla.org/releases/mozilla-aurora/file/b79894a533bc/intl/locale/PluralForm.jsm#l39
https://hg.mozilla.org/releases/mozilla-aurora/file/b79894a533bc/intl/locale/PluralForm.jsm#l116

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: testcase patch

Here is the xpcshell-test as requested in bug 1086238 comment 7.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8508673 [details] [diff] [review]
testcase patch

Sorry for that it seems that I misunderstood why it failed the plural rule is set correctly for CSB locale, the only issue her is that we call "caller" in strict mode. 

https://hg.mozilla.org/releases/mozilla-aurora/file/b79894a533bc/intl/locale/PluralForm.jsm#l138
> 1413892505715	addons.manager	WARN	Exception calling callback: TypeError: access to strict mode caller function is censored (resource://gre/modules/PluralForm.jsm:138:12) JS Stack trace:
> this.PluralForm.makeGetter/
> @PluralForm.jsm:138:13 
> [e-mail]:2478:15 
> [e-mail]:2414:7

Comment 3

[Axel Hecht [:Pike]]

resummarizing, to make this clearer:

In a browser context in scratchpad, 

Components.utils.import("resource://gre/modules/PluralForm.jsm");
[getPlurals, numForms] = PluralForm.makeGetter(9);
numForms()

getPlurals(5, 'one;many')

results in

PluralForm.jsm: Index #2 of 'one;many' for value 5 is invalid -- plural rule #9; called by top

in the browser console. It also returns "one" as result. Which is the first entry in the list, and thus always there (it's not the chinese plural form, though).

The culprit which makes the tests fail in bug 1086238 seems to be that the "called by top" part is

TypeError: access to strict mode caller function is censored (resource://gre/modules/PluralForm.jsm:138:12)

in mozmill tests, though. We should make the code not fail this hard if possible.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: Screenshot from 2014-10-22 10:15:00.png

(In reply to Axel Hecht [:Pike] from comment #3)
> The culprit which makes the tests fail in bug 1086238 seems to be that the
> "called by top" part is
> 
> TypeError: access to strict mode caller function is censored
> (resource://gre/modules/PluralForm.jsm:138:12)
> 
> in mozmill tests, though. We should make the code not fail this hard if
> possible.
This has been caught by mozmill, but has nothing to do with the mozmill code itself, it reproduces manually, and it's a real Firefox bug as you can see in the print-screen attached.

In AddonsManager when it creates the allResultsLink, it calls PluralForm.get from strict mode, this will try to access the function callers name, this throws an TypeError exception and brakes the code, resulting in a  failure of displaying the allResultsLink and dispatching the ViewChanged event.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js?rev=8d3304b7e0e0#2480

What I suggest is to try/catch the calling of PluralForm.get.caller.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Here is a fix-patch, whithout the fix, the test will fail with failure it fails in AddonsManager.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8509304 [details] [diff] [review]
patch v1.0

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

I don't feel more confident than giving an f+, but this looks good to me.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8509304 [details] [diff] [review]
patch v1.0

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

::: intl/locale/PluralForm.jsm
@@ +138,5 @@
> +        try {
> +          var caller = PluralForm.get.caller ? PluralForm.get.caller.name : "top";
> +        } catch (e if e instanceof TypeError) {
> +          var caller = "top";
> +        }

Using Components.stack.caller.name instead should allow this to work even in strict mode.

Comment 8

[Dave Townsend [:mossop]]

Also please in the future attach patches with 8 lines of context, it makes reviewing easier.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

Thanks, that worked perfectly. Can't wait to see the bug in Addons-Manager gone.

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

If this is affecting 34 too, it might be good to uplift.  It seems low risk but will fix a problem in the Add-ons Manager for several locales on Ubuntu. Cosmin, if you think it's a good idea, can you request uplift to 34? (and 35, since I think this would otherwise ride the trains from 36 onward)

Also, was this discovered from mozmill/jenkins tests?  For bugs we uncover using it, should we tag them (I think just tagging it [mozmill] in the whiteboard (I could be wrong here, check with whimboo)

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8510842 [details] [diff] [review]
patch v2.0

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

Great!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb9bb6ee090

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eb9bb6ee090

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

As Liz mentioned in comment 10 it would be nice to have this uplifted up to 34, it's a small fix that fixes a bug in Beta.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8510842 [details] [diff] [review]
patch v2.0

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
User can't see the "Show all add-ons link" in addon-manager in certain locales, see the screen-shot from attachment 
[Describe test coverage new/current, TBPL]:
A new xpcshell test has been made to cover this regression 
[Risks and why]: 
Low risk, we just call a different function that works in "strict mode" so it won't throw
[String/UUID change made/needed]: none

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8510842 [details] [diff] [review]
patch v2.0

This doesn't need to be tracked, but we'll take the low risk uplift.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0901daef7f2c
https://hg.mozilla.org/releases/mozilla-beta/rev/363ef26f9bee