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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34- fixed, firefox35- fixed, firefox36 fixed)

Details

(Whiteboard: [mozmill])

Attachments

(2 attachments, 2 obsolete attachments)

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
Posted patch testcase patch (obsolete) — Splinter Review
Here is the xpcshell-test as requested in bug 1086238 comment 7.
Blocks: 1086238
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 
> gSearchView_showAllResultsLink@extensions.js:2478:15 
> gSearchView_updateView@extensions.js:2414:7
Attachment #8508673 - Attachment is obsolete: true
Summary: PluralForm.jsm should default to English pluralRule instead it defaults to the first one which is Chinese → PluralForm.jsm calls PluralForm.get.caller which is not allowed in "strict mode";
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.
(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.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Posted patch patch v1.0 (obsolete) — Splinter Review
Here is a fix-patch, whithout the fix, the test will fail with failure it fails in AddonsManager.
Attachment #8509300 - Attachment is obsolete: true
Attachment #8509304 - Flags: review?(l10n)
Attachment #8509304 - Flags: review?(dtownsend+bugmail)

Updated

5 years ago
Attachment #8509300 - Attachment is obsolete: false
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.
Attachment #8509304 - Flags: review?(l10n) → feedback+
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.
Attachment #8509304 - Flags: review?(dtownsend+bugmail) → review-
Also please in the future attach patches with 8 lines of context, it makes reviewing easier.
Posted patch patch v2.0Splinter Review
Thanks, that worked perfectly. Can't wait to see the bug in Addons-Manager gone.
Attachment #8509304 - Attachment is obsolete: true
Attachment #8510842 - Flags: review?(dtownsend+bugmail)
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)
Whiteboard: [mozmill]
Comment on attachment 8510842 [details] [diff] [review]
patch v2.0

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

Great!
Attachment #8510842 - Flags: review?(dtownsend+bugmail) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1eb9bb6ee090
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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 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
Attachment #8510842 - Flags: approval-mozilla-beta?
Attachment #8510842 - Flags: approval-mozilla-aurora?
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.
Attachment #8510842 - Flags: approval-mozilla-beta?
Attachment #8510842 - Flags: approval-mozilla-beta+
Attachment #8510842 - Flags: approval-mozilla-aurora?
Attachment #8510842 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.