1049802 - enforce ES strict mode in ServiceWorkers

Status: RESOLVED WONTFIX

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

It seems likely that ES strict mode will be required in ServiceWorkers.  See:

  https://github.com/slightlyoff/ServiceWorker/issues/294

Comment 1

[Anne (:annevk)]

> Yes, it seems like this should be possible by setting 
> CompileOptions::strictOption to true if the worker IsServiceWorker().

We should also do it for importScripts().

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #1)
> We should also do it for importScripts().

Isn't importScripts() already out in the wild without this enforcement?  It would seem if we started enforcing strict here it would break compatibility with current content.

Comment 3

[Anne (:annevk)]

Well, only when it is invoked from a service worker would be my suggestion, but perhaps since the plan was to replace it with modules over time that is less needed.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #3)
> Well, only when it is invoked from a service worker would be my suggestion,
> but perhaps since the plan was to replace it with modules over time that is
> less needed.

I think it would be a bit surprising for importScripts() to enforce strict in ServiceWorkers, but not in any other workers.  That sounds confusing for developers and framework writers.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

If we are going to enforce this, documentation clearly mentioning this is paramount!

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this actually desirable?  Making it so that existing code does not work in this environment seems like a bad idea.

Comment 7

[Anne (:annevk)]

If we do this service workers can use modules without opt-in later on. That seems desirable to me.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I thought "module mode" was different from "strict mode"? I don't think you can have an 'import' statement in anything that's in strict mode.

Comment 9

[Anne (:annevk)]

They are compatible given that this is an asynchronous context. We do not have any context that is both asynchronous and strict at the moment. (There's no context at the moment that starts out strict, even.)

Comment 10

[Jefry Lagrange]

I would like to work on this bug, but I'm not sure where to do the modifications. As I understand when the ServiceWorker is created it has a URL associated with it that contains the script. I suppose you want this to be parsed in strict mode. Where does this parsing take place? Spidermonkey?

Comment 11

[Josh Matthews [:jdm]]

The parsing takes place in SpiderMonkey; the actual runtime settings are decided in CreateJSContextForWorker (http://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#789) and you probably want to modify the strict mode setting (http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#1484) if the worker is a ServiceWorker.

Comment 12

[Jefry Lagrange]

Attachment: enforces strict mode for serviceworkers

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8504980 [details] [diff] [review]
enforces strict mode for serviceworkers

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

Please add tests and upload the new patch.
Mochitests can be added in dom/workers/test/serviceworkers. You want to create a worker script that does something not allowed in strict mode, then try to register that as a serviceworker and it should fail (I believe the way the SW spec currently specifies things, the Promise returned from navigator.serviceWorker.register() should reject).
Patch looks good.

Comment 14

[Jefry Lagrange]

For me, registration always succeeds even though there is a violation of the strict mode. You can test it by putting "use strict" in the global scope of a serviceworker file and then violate strict mode. Registration does not fail. 

Is there an alternative way to test this? All I can think of is using the response of a fetch to communicate. But I'm having problems with that as well...

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Can you see if ServiceWorkerManager::HandleError() is ever reached using gdb when you run the script? If it does, then does it eventually reach the RejectRegisterPromises at the end of HandleError?

Comment 16

[Jefry Lagrange]

Attachment: Enforces strict mode for ServiceWorkers

Enforces strict mode for ServiceWorkers along with the mochitest to test whether a ServiceWorkers are in strict mode or not.

Comment 17

[Jefry Lagrange]

The reason why registration was always succeeding for me is that if the new Serviceworker has an error, it doesn't get registered. If it is ServiceWorker that is updating and old one and this one has an error, then it doesn't get registered and the old ServiceWorker keeps working.

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8505869 [details] [diff] [review]
Enforces strict mode for ServiceWorkers

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

I'd like to take another look with the following changes. Also, please push your changes to try server and post a link to the run in the comments.
https://wiki.mozilla.org/Build:TryServer
If you do not have level 1 access, you can either apply (encouraged) or I can push to try for you.

::: dom/workers/test/serviceworkers/test_bug1049802.html
@@ +16,5 @@
> +    <body>
> +        <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1049802">Mozilla Bug 1049802</a>
> +        <p id="display"></p>
> +        <div id="content" style="display: none">
> +              

Nit: Trailing whitespace

@@ +27,5 @@
> +
> +  function runTest() {
> +    navigator.serviceWorker.register('test_bug1049802.js', {
> +    scope: '/'
> +    }).then(function() {

Nit: Please write this as:
navigator.serviceWorker.register('...', { scope: '/' }).then(function() {
}, function() {
});

using the two arg form of then().

In both resolve and reject handlers you will have to call SimpleTest.finish() to indicate that the asynchronous test is finished since you used waitForExplicitFinish() below.

@@ +28,5 @@
> +  function runTest() {
> +    navigator.serviceWorker.register('test_bug1049802.js', {
> +    scope: '/'
> +    }).then(function() {
> +    // registration worked

Comment not needed.

@@ +29,5 @@
> +    navigator.serviceWorker.register('test_bug1049802.js', {
> +    scope: '/'
> +    }).then(function() {
> +    // registration worked
> +     ok(false, "test failed: ServiceWorkers are not in strict mode");

The convention is that the message is always 'positive' and expresses the intent (so should, instead of are), and the false indicates the failure. You also don't need to put "test failed" since the harness prints something similar. So this will become:
ok(false, "ServiceWorker scripts should be valid strict mode JavaScript");

@@ +31,5 @@
> +    }).then(function() {
> +    // registration worked
> +     ok(false, "test failed: ServiceWorkers are not in strict mode");
> +    }).catch(function(error) {
> +     ok(true, "test passed: ServiceWorkers are in strict mode");

and then here you'd use the same text as above but with false changed to true.

@@ +32,5 @@
> +    // registration worked
> +     ok(false, "test failed: ServiceWorkers are not in strict mode");
> +    }).catch(function(error) {
> +     ok(true, "test passed: ServiceWorkers are in strict mode");
> +    // registration failed

Ditto.

::: dom/workers/test/serviceworkers/test_bug1049802.js
@@ +1,1 @@
> +

can you remove all the empty lines?

@@ +1,3 @@
> +
> +
> +// try to violate strict mode

Nit: T uppercase and period at the end of the sentence.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just want to get feedback from dherman that this isn't a horrible idea. I.e. is the inconsistency that this introduces (different workers will default to different modes) worse than the fact that we'll get more content using strict mode?

Comment 20

[Jefry Lagrange]

Attachment: Enforces strict mode for ServiceWorkers

Enforces strict mode for ServiceWorkers. Mochitest included.

Link to try server results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=55e56d5cfc7f

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8506480 [details] [diff] [review]
Enforces strict mode for ServiceWorkers

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

r=me with the following changes. Please don't land until we've reached a decision here https://github.com/slightlyoff/ServiceWorker/issues/294.

::: dom/workers/test/serviceworkers/test_bug1049802.html
@@ +24,5 @@
> +/** Test for Bug 1049802 **/
> +// Testing whether the ServiceWorkers are in strict mode
> +
> +  function runTest() {
> +    navigator.serviceWorker.register('test_bug1049802.js', { scope: '/' })

Set the scope to './foobar' or something instead of '/'. That way, on the off-chance that this succeeds we don't have every subsequent test going through the ServiceWorker.

@@ +28,5 @@
> +    navigator.serviceWorker.register('test_bug1049802.js', { scope: '/' })
> +    .then(function() {
> +      ok(false, "ServiceWorker scripts should be valid strict mode JavaScript");
> +      SimpleTest.finish();
> +    }, function() { 

trailing whitespace

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8506480 [details] [diff] [review]
Enforces strict mode for ServiceWorkers

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

r=me with the following changes. Please don't land until we've reached a decision here https://github.com/slightlyoff/ServiceWorker/issues/294.

::: dom/workers/test/serviceworkers/test_bug1049802.html
@@ +24,5 @@
> +/** Test for Bug 1049802 **/
> +// Testing whether the ServiceWorkers are in strict mode
> +
> +  function runTest() {
> +    navigator.serviceWorker.register('test_bug1049802.js', { scope: '/' })

Set the scope to './foobar' or something instead of '/'. That way, on the off-chance that this succeeds we don't have every subsequent test going through the ServiceWorker.

@@ +28,5 @@
> +    navigator.serviceWorker.register('test_bug1049802.js', { scope: '/' })
> +    .then(function() {
> +      ok(false, "ServiceWorker scripts should be valid strict mode JavaScript");
> +      SimpleTest.finish();
> +    }, function() { 

trailing whitespace

Comment 23

[Jefry Lagrange]

Attachment: Enforces strict mode for ServiceWorkers

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 8506507 [details] [diff] [review]
Enforces strict mode for ServiceWorkers

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

Regarding having module scripts not have a global object, I think this is something the JS engine could support, in some sense in the "right" way, maybe in short-ish order, for what it's worth.  I'd need to know more about the peculiarities of what that entails, tho, before I could commit to it.  (I'm only popping in here after seeing some idle discussion in #jslang and being asked to comment.  :-) )

::: dom/workers/test/serviceworkers/test_bug1049802.js
@@ +1,3 @@
> +// Try to violate strict mode.
> +
> +undeclared = 0;

In addition to this violation, it would be nice if other attempts were made.  Try violating it inside Function("...") code, in eval("...") code, and in (1, eval)("...") indirect eval code, to ensure that various "derivative" ways to run code, also have strict mode restrictions in place.  I'm fairly sure the runtime setting affects all of these, but best to be sure.

Comment 25

[Jefry Lagrange]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> In addition to this violation, it would be nice if other attempts were made.
> Try violating it inside Function("...") code, in eval("...") code, and in
> (1, eval)("...") indirect eval code, to ensure that various "derivative"
> ways to run code, also have strict mode restrictions in place.  I'm fairly
> sure the runtime setting affects all of these, but best to be sure.

Since the test uses the fact that a particular js file was succesfully or unsuccesfully registered as a service worker, adding more tests would mean adding more files. I don't see any other way this could be done. If I add them all in the same file, only the first violation would be checked.

Would creating 3 more js files be desirable?

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Jefry Lagrange from comment #25)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> > In addition to this violation, it would be nice if other attempts were made.
> > Try violating it inside Function("...") code, in eval("...") code, and in
> > (1, eval)("...") indirect eval code, to ensure that various "derivative"
> > ways to run code, also have strict mode restrictions in place.  I'm fairly
> > sure the runtime setting affects all of these, but best to be sure.
> 
> Since the test uses the fact that a particular js file was succesfully or
> unsuccesfully registered as a service worker, adding more tests would mean
> adding more files. I don't see any other way this could be done. If I add
> them all in the same file, only the first violation would be checked.
> 
> Would creating 3 more js files be desirable?

Not a problem. More tests are always welcome.

Comment 27

[Jefry Lagrange]

I think I found something while testing the following code:

>"use strict";
>
>function bad_function () {
>  var repeated_key = { x : 1, x : 2 };
>}
>
>bad_function();

If a ServiceWorker file is registered with the code above, it will successfully register without throwing an error. The expected error is: "SyntaxError: property name x appears more than once in object literal".

Comment 28

[Jeff Walden [:Waldo]]

> "use strict";
>
> function bad_function () {
>   var repeated_key = { x : 1, x : 2 };
> }
>
> bad_function();

That's a change in ES6: repeated property names in an object literal are no longer an error.  (This because of computed property names, e.g. |var x = "foo"; var obj = { [x]: 17, "foo" 42 };| making it impossible to generally prevent conflicts.)  SpiderMonkey implemented this change within the last cycle or two.  So no actual issue there.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #26)
> > Would creating 3 more js files be desirable?
> 
> Not a problem. More tests are always welcome.

As far as that goes, I think you only need one more.  You need a separate test for testing strict mode errors that are *early* errors, that occur before any script runs.  But these others can be attempted, and exceptions caught, in series within a single script file.

Comment 29

[Jefry Lagrange]

Attachment: Enforces strict mode for ServiceWorkers

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #28)
> As far as that goes, I think you only need one more.  You need a separate
> test for testing strict mode errors that are *early* errors, that occur
> before any script runs.  But these others can be attempted, and exceptions
> caught, in series within a single script file.

By the time I read your comment, I have already written it into separate files. 

Link to the mochitest results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7548abb09e8

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Jefry, the test has failed in the link you posted in comment 29.

Comment 31

[Jefry Lagrange]

Attachment: strict_serviceworkers.patch

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #30)
> Jefry, the test has failed in the link you posted in comment 29.

Yea, sorry about that. 

Here is the new link: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e7f51c835644

Some tests are failing but I think they are unrelated and were failing before.

Comment 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8508224 [details] [diff] [review]
strict_serviceworkers.patch

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

Just fix the one comment and upload a new patch. No need to ask for review again. Also put a link to a try build.

::: dom/workers/test/serviceworkers/test_bug1049802.html
@@ +29,5 @@
> +    .then(function() {
> +      ok(false, "ServiceWorker scripts should be valid strict mode JavaScript");
> +    }, function(error) {
> +      ok(true, "ServiceWorker scripts should be valid strict mode JavaScript");
> +      console.log('Registration failed with ' + error);

Remove the log.

Comment 33

[Jefry Lagrange]

Attachment: Enforces strict mode for ServiceWorkers

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=765164e5741e

Comment 34

[Josh Matthews [:jdm]]

What's the status here, Nikhil?

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The spec hasn't moved to consensus. I don't think we are doing this.

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Keeping it around for now, but I doubt we are going to enforce strict mode.

Comment 37

[Anne (:annevk)]

To be clear, if we don't make service workers JavaScript module compatible we'll need some kind of separate opt in for that later on. Perhaps it is okay that all workers require such a separate opt in, but it seems kind of wasteful given that workers are already asynchronous.