553434 - jetpack-core modules should declare "use strict"

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Atul Varma [:atul]]

Once our platform fully supports ES5 strict mode, our code should use it.

Comment 1

[Brian Warner [:warner :bwarner]]

And we should start telling developers to write ES5-Strict -conforming code
now, even if the platform doesn't yet have a way to enforce it. And we should
find a way to enforce it in the SDK (during the packaging step), and/or in
the AMO review process, even if we don't yet have a way to enforce it as the
code is loaded into the browser.

I'm thinking that asking developers to write "use strict" at the top of all
their files is an unnecessary hassle. There's a tradeoff, of course.
Requiring it means that every jetpack developer (and reader of the code) will
be reminded that the code is supposed to be ES5-S, and no matter where that
code is run (perhaps in some other environment, imported/required as a module
by a non-Jetpack program) it will indicate to the loader that it wishes to
have ES5-S-ness enforced upon it. "Explicit is better than implicit".

Not requiring it makes the jetpack module code look cleaner, and removes
boilerplate which gets in the way of readability. And, in my opinion, it's
the package.json file which should be enforcing constraints on the language
used by the module, where a reviewer can quickly make decisions about how
easy it will be to review the code. If we come up with additional constraints
in the future, they should go in that package.json file. (imagine a field in
package.json which means "the code in my module is in language X", where X
might be "ES5", or "ES5-Strict", or something else entirely).

The SDK could somehow test for ES5-S-ness when building the XPI, and throw an
error if it doesn't conform, perhaps by passing it to a nearby xulrunner for
inspection. There may also be some limited set of checking we could do from
the python code, or from a javascript program run via xulrunner. I don't know
what the state-of-the-art is for ES5-S conformance testing.

But of course, the final goal is to have the add-on code loader enforce this
rule, since that's the right place to do it, and not all add-ons will go
through AMO.

Comment 2

[Atul Varma [:atul]]

Should we start enabling this in our code now? Bug 537873 and Bug 514574 landed on september 15, which means we can start actually getting some of ES5-Strict's benefits.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 4

[Brian Warner [:warner :bwarner]]

For grins, I tried an experiment in which securable-module.js outright injects "use strict;" into the first line of every module that it loads. Then I went through jetpack-core and addon-kit to fix up the tests that failed. They fell into three categories:

* using an octal integer literal (file.js, in chmod)
* using arguments.callee to do recursion in an anonymous function (various tabs modules)
* testing the exact contents of the evaluated code (test-securable-module.js)

They were all fairly easy to fix up. I think all tests pass against 4.0b6 (although the usual test noise makes it hard to be sure). The results are in my github branch at https://github.com/warner/addon-sdk/tree/strict .. please take a look!

Comment 5

[Brian Warner [:warner :bwarner]]

hm, I should update that branch.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/205

Pointer to Github pull-request

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Since this change makes SDK all strict, it's important to let users know that this is the case either somewhere in docs or tutorials. (CC-ing Will to see what he thinks)

Also, this may break some add-on's if they violate ES strict. Most likely that may happen by introducing unintended global vars or use of callee and caller. I think this also should be communicated to users in some way.

Probably it's a good idea to explain how to live without arguments.callee (Just use named functions and refer to it by name) and explain why `caller` was removed.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #8)
> Also, this may break some add-on's if they violate ES strict. Most likely
> that may happen by introducing unintended global vars or use of callee and
> caller. I think this also should be communicated to users in some way.

We can't do that at this point, at least not so abruptly.  The fix for this bug should focus on the original goal of the bug, which is to run the SDK's own code in strict mode, and leave aside the question of whether and how to enforce strictness in addon code (which we should tackle in discussions, then a feature page, and finally one or more new bugs).

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I have updated patch so that it only adds strict mode to stdlib.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Landed: https://github.com/mozilla/addon-sdk/commit/ac0abb11aafcb51b375abf5227c47c1ec022fd4c

warner can you please r+ here as well

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Myk reported following error:

> This landing broke tests on debug builds with exceptions like:

> ReferenceError: assignment to undeclared variable name (resource://gre/modules/XPIProvider.jsm -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/bootstrap.js -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/components/harness.js:170)
stack:
buildLoader()@resource://gre/modules/XPIProvider.jsm -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/bootstrap.js -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/components/harness.js:170
()@resource://gre/modules/XPIProvider.jsm -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/bootstrap.js -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/components/harness.js:281
Harness_load("startup")@resource://gre/modules/XPIProvider.jsm -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/bootstrap.js -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/components/harness.js:309
Harness_observe(null,"sessionstore-windows-restored","")@resource://gre/modules/XPIProvider.jsm -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/bootstrap.js -> file:///tmp/tmpt1_V80.mozrunner/extensions/[e-mail]/components/harness.js:374

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/217

Pointer to Github pull-request

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 547545 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/217

Looks good, resolves problems in my tests, r=myk and automatically merged:

https://github.com/mozilla/addon-sdk/commit/b4031514e0ddbe90a6c6a82e8e584542c1eedec5

Comment 15

[Brian Warner [:warner :bwarner]]

Comment on attachment 544515 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/205

this pull request was landed last july, forgot to clear the r? flag