ServiceWorker: Registering a malformed script should fail [3/3]

ServiceWorker: Registering a malformed script should fail [3/3]

Before this series of patches, registration succeeds even if the worker script
has a syntax error because the installing sequence does not check the result of
the script evaluation.

This adds layout tests for a variety of error cases.

Patch dependency:
[1] Blink: https://codereview.chromium.org/692003002/
[2] Chromium: https://codereview.chromium.org/697593002/
[3] Blink: THIS PATCH

BUG=426344
TEST=run-webkit-tests --debug http/tests/serviceworker/

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184903

[nhiroki, 2014-11-04 03:32:31]

Patchset #1 (id:1) has been deleted

[falken, 2014-11-04 04:14:02]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2014-11-04 04:14:02]

Question: What happens with importScripts('malformed.js')? And what do we want
to happen... ?

[nhiroki, 2014-11-04 05:06:06]

On 2014/11/04 04:14:02, falken wrote:
> Question: What happens with importScripts('malformed.js')? And what do we want
> to happen... ?

Good point! I confirmed that these changes also reject a worker script that
contains importScripts('malformed.js') or importScripts('no-such-script.js')
(see the patchset 2). This behavior seems natural to me because probably
application developers don't expect import failures. Wdyt?

[nhiroki, 2014-11-04 05:14:56]

On 2014/11/04 05:06:06, nhiroki wrote:
> On 2014/11/04 04:14:02, falken wrote:
> > Question: What happens with importScripts('malformed.js')? And what do we
want
> > to happen... ?
> 
> Good point! I confirmed that these changes also reject a worker script that
> contains importScripts('malformed.js') or importScripts('no-such-script.js')
> (see the patchset 2). This behavior seems natural to me because probably
> application developers don't expect import failures. Wdyt?

Oops! I used not 'importScripts()' but 'importScript()'... I'll test them
again...

[nhiroki, 2014-11-04 05:21:15]

On 2014/11/04 05:14:56, nhiroki wrote:
> On 2014/11/04 05:06:06, nhiroki wrote:
> > On 2014/11/04 04:14:02, falken wrote:
> > > Question: What happens with importScripts('malformed.js')? And what do we
> want
> > > to happen... ?
> > 
> > Good point! I confirmed that these changes also reject a worker script that
> > contains importScripts('malformed.js') or importScripts('no-such-script.js')
> > (see the patchset 2). This behavior seems natural to me because probably
> > application developers don't expect import failures. Wdyt?
> 
> Oops! I used not 'importScripts()' but 'importScript()'... I'll test them
> again...

Okay, I made sure these changes work fine with 'importScript*s*' (patchset 3).

[falken, 2014-11-04 05:26:41]

On 2014/11/04 05:21:15, nhiroki wrote:
> On 2014/11/04 05:14:56, nhiroki wrote:
> > On 2014/11/04 05:06:06, nhiroki wrote:
> > > On 2014/11/04 04:14:02, falken wrote:
> > > > Question: What happens with importScripts('malformed.js')? And what do
we
> > want
> > > > to happen... ?
> > > 
> > > Good point! I confirmed that these changes also reject a worker script
that
> > > contains importScripts('malformed.js') or
importScripts('no-such-script.js')
> > > (see the patchset 2). This behavior seems natural to me because probably
> > > application developers don't expect import failures. Wdyt?

Sounds right to me, thanks for adding the test.

Can you also write a test for a non-parse error like:
foo.bar = 42;  // where foo is undefined

From code inspection, I think it ought to work (install shouldn't succeed).
(Still reviewing the other patches, sorry for responding piecemeal)

Also, how about one where you have an error but handle it (try/catch or listen
for ErrorEvent)?

[falken, 2014-11-04 05:47:55]

loooks good with nits

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/registration.html:95: var script =
'resources/import-no-such-script.js';
We've been naming service worker scripts as "*-worker.js"

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/registration.html:142: var scope =
'resources/registration/malformed-worker';
Be consistent with the other scopes in this file: resources/scope/script-name

[nhiroki, 2014-11-04 08:44:00]

Patchset #4 (id:80001) has been deleted

[nhiroki, 2014-11-04 08:45:35]

Patchset #4 (id:100001) has been deleted

[nhiroki, 2014-11-04 08:54:51]

Patchset #4 (id:120001) has been deleted

[nhiroki, 2014-11-04 08:56:21]

Added more layout tests for cases you mentioned. Can you take another look?
Thanks!

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/registration.html:95: var script =
'resources/import-no-such-script.js';
On 2014/11/04 05:47:55, falken wrote:
> We've been naming service worker scripts as "*-worker.js"

Done.

https://codereview.chromium.org/697833004/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/registration.html:142: var scope =
'resources/registration/malformed-worker';
On 2014/11/04 05:47:55, falken wrote:
> Be consistent with the other scopes in this file: resources/scope/script-name

Done.

[falken, 2014-11-05 01:47:44]

LGTM

[nhiroki, 2014-11-06 07:14:31]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-06 07:15:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697833004/140001

[commit-bot: I haz the power, 2014-11-06 08:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-06 08:24:58]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/29783)

[nhiroki, 2014-11-06 09:08:13]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-06 09:09:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697833004/140001

[commit-bot: I haz the power, 2014-11-06 09:43:55]

Committed patchset #4 (id:140001) as 184903