service worker: Fix the type of an update promise reject value

service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
Committed: https://crrev.com/fc99c082b8d4f4ef333aab1abf0f953e2b304ad0
Cr-Commit-Position: refs/heads/master@{#404140}

[e_hakkinen, 2016-06-10 20:39:19]

eero.hakkinen@intel.com changed reviewers:
+ horo@chromium.org, shimazu@chromium.org

[e_hakkinen, 2016-06-10 20:39:20]

PTAL.

The SW specification has been changed so that reject value should now be a
TypeError object.

[falken, 2016-06-14 02:27:52]

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

[falken, 2016-06-14 02:27:53]

Thanks for working on this. I'm not totally sure this is the right approach,
comments inline. Also, this change will need a test.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:58: case
WebServiceWorkerError::ErrorTypeScriptEvaluateFailed:
The SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED error code can be used outside
of installation. For example, starting a worker can fail with this error code.
If we're adding this WebServiceWorkerError code, we shouldn't assume it's only
used in installation.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:44:
case WebServiceWorkerError::ErrorTypeType: {
Can you explain why are network, script evaluate, and type the ones here that
become TypeError, and the others do the default ServiceWorkerError::take?

[e_hakkinen, 2016-06-16 20:55:14]

On 2016/06/14 02:27:53, falken wrote:
> I'm not totally sure this is the right approach,
> comments inline.

What approach you would prefer?

One other approach would be to do mapping to TypeError on browser side (instead
of on renderer side) but in that case quite a much of context would be lost.

> Also, this change will need a test.

Based on https://crbug.com/602693, shimazu is working on importing W3C tests
which test this and which I used for testing this.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:58: case
WebServiceWorkerError::ErrorTypeScriptEvaluateFailed:
On 2016/06/14 02:27:53, falken wrote:
> The SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED error code can be used outside
> of installation. For example, starting a worker can fail with this error code.
> If we're adding this WebServiceWorkerError code, we shouldn't assume it's only
> used in installation.

This is mainly for not to break other callers of this functions.
Once all callers map script evaluate failures correctly to TypeError, the
ErrorTypeScriptEvaluateFailed case can be merged with the ErrorTypeType branch
which does ASSERT_NOT_REACHED.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:44:
case WebServiceWorkerError::ErrorTypeType: {
On 2016/06/14 02:27:53, falken wrote:
> Can you explain why are network, script evaluate, and type the ones here that
> become TypeError, and the others do the default ServiceWorkerError::take?

ServiceWorkerError::take returns a DOMException which cannot be represent
TypeError but can represent all other errors such as SecurityError.

According to the SW spec Update algorithm step 5, If the fetch algorithm
asynchronously completes with null, then invoke Reject Job Promise with job and
a TypeError. That is the reason for the ErrorTypeNetwork case.

According to the SW spec Update algorithm step 7.5, if an uncaught runtime
script error occurs during the above step, then invoke Reject Job Promise with
job and a TypeError. That is the reason for the ErrorTypeScriptEvaluateFailed
case.

That list is not complete but it is a start.

The case that error.errorType is equal to ErrorTypeType actually never happens,
because ServiceWorkerMsg_ServiceWorkerUpdateError IPC cannot carry ErrorTypeType
as that is greater than ErrorTypeLast.

It might make sense to move this logic to ServiceWorkerError, for instance, so
that it could be reused and that default error messages used by
ServiceWorkerError could be reused.

[falken, 2016-06-17 02:12:14]

OK I think this approach may be OK. It feels like our error handling could be
less convoluted somehow, in particular WebServiceWorkerError::take seems to not
be serving us well since it can't return a TypeError. But such refactoring could
happen later.

> Based on https://crbug.com/602693, shimazu is working on importing W3C tests
which test this and which I used for testing this.

Please add the relevant tests in this patch, so we have test coverage before the
importing is finished.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:58: case
WebServiceWorkerError::ErrorTypeScriptEvaluateFailed:
On 2016/06/16 20:55:14, e_hakkinen wrote:
> On 2016/06/14 02:27:53, falken wrote:
> > The SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED error code can be used
outside
> > of installation. For example, starting a worker can fail with this error
code.
> > If we're adding this WebServiceWorkerError code, we shouldn't assume it's
only
> > used in installation.
> 
> This is mainly for not to break other callers of this functions.
> Once all callers map script evaluate failures correctly to TypeError, the
> ErrorTypeScriptEvaluateFailed case can be merged with the ErrorTypeType branch
> which does ASSERT_NOT_REACHED.

It'd be unfortunate for each callsite to have to special case
ScriptEvaluteFailed. Fortunately, I think the only callsite that can get a
ScriptEvaluateFailed is the register promise case. So if that's changed over, we
should be able to do ASSERT_NOT_REACHED here. 

Otherwise, we should make ServiceWorkerError::take be able to create a
TypeError.

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:44:
case WebServiceWorkerError::ErrorTypeType: {
On 2016/06/16 20:55:14, e_hakkinen wrote:
> On 2016/06/14 02:27:53, falken wrote:
> > Can you explain why are network, script evaluate, and type the ones here
that
> > become TypeError, and the others do the default ServiceWorkerError::take?
> 
> ServiceWorkerError::take returns a DOMException which cannot be represent
> TypeError but can represent all other errors such as SecurityError.
> 
> According to the SW spec Update algorithm step 5, If the fetch algorithm
> asynchronously completes with null, then invoke Reject Job Promise with job
and
> a TypeError. That is the reason for the ErrorTypeNetwork case.

I see. Looks like that changed here:
https://github.com/slightlyoff/ServiceWorker/commit/e9fabe32d10ed8f2fbf1bdea4...

We have LayoutTests tests for this in
http/tests/serviceworker/resources/registration-tests.js that will need to be
changed.

It's a bit weird that the browser is sending a network error code to the
renderer, which just becomes a TypeError. I guess the advantage would be that
the renderer can use a default error message.

> 
> According to the SW spec Update algorithm step 7.5, if an uncaught runtime
> script error occurs during the above step, then invoke Reject Job Promise with
> job and a TypeError. That is the reason for the ErrorTypeScriptEvaluateFailed
> case.

I see. One slight worry is that ScriptEvaluateFailed is not always indicative of
a runtime script error. If the thread gets terminated for some reason during
startup and happens to be in the evaluate state, it sends a
OnScriptEvaluteFailed to the browser, which now would come back as a
ScriptEvaluateFailed. But that's probably not so important.

Generally our error handling is not so elegant.

> 
> That list is not complete but it is a start.
> 
> The case that error.errorType is equal to ErrorTypeType actually never
happens,
> because ServiceWorkerMsg_ServiceWorkerUpdateError IPC cannot carry
ErrorTypeType
> as that is greater than ErrorTypeLast.

Yikes, I think that's an oversight. We should fix that.

> 
> It might make sense to move this logic to ServiceWorkerError, for instance, so
> that it could be reused and that default error messages used by
> ServiceWorkerError could be reused.

Thanks for the explanation. I think we should have a comment above these cases
that the spec says network errors and runtime script errors during registration
result in a TypeError.

[e_hakkinen, 2016-06-17 11:31:29]

Patchset #2 (id:20001) has been deleted

[e_hakkinen, 2016-06-17 12:29:18]

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
(right):

https://codereview.chromium.org/2054203002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:44:
case WebServiceWorkerError::ErrorTypeType: {
On 2016/06/17 02:12:14, falken wrote:
> On 2016/06/16 20:55:14, e_hakkinen wrote:
> > According to the SW spec Update algorithm step 5, If the fetch algorithm
> > asynchronously completes with null, then invoke Reject Job Promise with job
> and
> > a TypeError. That is the reason for the ErrorTypeNetwork case.
> 
> I see. Looks like that changed here:
>
https://github.com/slightlyoff/ServiceWorker/commit/e9fabe32d10ed8f2fbf1bdea4...
> 
> We have LayoutTests tests for this in
> http/tests/serviceworker/resources/registration-tests.js that will need to be
> changed.

Yes, but that tests the register code path and not this update code path.
Therefore, that change is handled by CL https://crrev.com/2059953003.

> Thanks for the explanation. I think we should have a comment above these cases
> that the spec says network errors and runtime script errors during
registration
> result in a TypeError.

Done.

[falken, 2016-06-20 08:41:14]

Thanks, this looks good. Just some nits.

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html
(right):

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:96: 'new
installing should be set after update resolves.');
nit: it's better to make description strings unique over a test, so in case of a
failure you can see which assert failed.

In this case I recommend "installing should be set after update resolves
(throw-install)"

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:98: 'waiting
should be null after activated.');
ditto, the "activated" part also seems inaccurate in this case

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:122:
iframe.remove();
nit: this can be removed since with_iframe automatically removes the iframe at
the end of the test

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:123: return
t.done();
nit: t.done() isn't needed since this is a promise_test. Just return the promise
chain.

[e_hakkinen, 2016-07-05 18:29:00]

Back in action. Sorry for delay.

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html
(right):

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:96: 'new
installing should be set after update resolves.');
On 2016/06/20 08:41:14, falken wrote:
> nit: it's better to make description strings unique over a test, so in case of
a
> failure you can see which assert failed.
> 
> In this case I recommend "installing should be set after update resolves
> (throw-install)" 

Done.

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:98: 'waiting
should be null after activated.');
On 2016/06/20 08:41:14, falken wrote:
> ditto, the "activated" part also seems inaccurate in this case

Done.

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:122:
iframe.remove();
On 2016/06/20 08:41:14, falken wrote:
> nit: this can be removed since with_iframe automatically removes the iframe at
> the end of the test

Done.

https://codereview.chromium.org/2054203002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/update.html:123: return
t.done();
On 2016/06/20 08:41:14, falken wrote:
> nit: t.done() isn't needed since this is a promise_test. Just return the
promise
> chain.

Done.

[falken, 2016-07-07 01:43:41]

Welcome back and sorry for my delay too. lgtm!

https://codereview.chromium.org/2054203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:111: //
According to the spec, network and runtime script errors during
I think I recommended this comment, but now I see it's jarring that
ErrorTypeNotFound is not mentioned in the comment, while the other two are. I
guess we can say "According to the spec, these errors during update should
result in a TypeError".

[e_hakkinen, 2016-07-07 06:33:05]

https://codereview.chromium.org/2054203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:111: //
According to the spec, network and runtime script errors during
On 2016/07/07 01:43:41, falken wrote:
> I think I recommended this comment, but now I see it's jarring that
> ErrorTypeNotFound is not mentioned in the comment, while the other two are. I
> guess we can say "According to the spec, these errors during update should
> result in a TypeError".

Done.

[e_hakkinen, 2016-07-07 06:33:17]

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 06:33:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[e_hakkinen, 2016-07-07 06:36:49]

Description was changed from

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
==========

to

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
==========

[e_hakkinen, 2016-07-07 06:36:49]

eero.hakkinen@intel.com changed reviewers:
+ haraken@chromium.org

[e_hakkinen, 2016-07-07 06:41:01]

+@haraken: third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h

PTAL.

[commit-bot: I haz the power, 2016-07-07 07:52:56]

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

[commit-bot: I haz the power, 2016-07-07 07:52:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-07-07 08:15:10]

bindings LGTM

https://codereview.chromium.org/2054203002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp
(right):

https://codereview.chromium.org/2054203002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerError.cpp:59: //
FIXME: Introduce new ActivateError type to ExceptionCodes?

FIXME => TODO

[e_hakkinen, 2016-07-07 11:30:45]

The CQ bit was checked by eero.hakkinen@intel.com

[e_hakkinen, 2016-07-07 11:30:46]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2054203002/#ps120001
(title: "FIXME => TODO")

[commit-bot: I haz the power, 2016-07-07 11:31:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 12:44:59]

Description was changed from

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
==========

to

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
==========

[commit-bot: I haz the power, 2016-07-07 12:45:01]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-07 12:45:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 12:47:35]

Description was changed from

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881
==========

to

==========
service worker: Fix the type of an update promise reject value

This CL changes the type of the update promise reject value to be
TypeError (instead of AbortError or NetworkError) if the update promise
is rejected because an uncaught runtime script error occured during Run
Service Worker algorithm.

BUG=617881

Committed: https://crrev.com/fc99c082b8d4f4ef333aab1abf0f953e2b304ad0
Cr-Commit-Position: refs/heads/master@{#404140}
==========

[commit-bot: I haz the power, 2016-07-07 12:47:36]

Patchset 6 (id:??) landed as
https://crrev.com/fc99c082b8d4f4ef333aab1abf0f953e2b304ad0
Cr-Commit-Position: refs/heads/master@{#404140}