Bug 182416 – When SW install fails, null out registration.installing before setting worker state to "redundant"

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

When SW install fails, null out registration.installing before setting worker state to "redundant". This does not match the spec but this is what Firefox and Chrome do. This is also what the web-platform-tests expect.

[Radar WebKit Bug Importer]

<rdar://problem/37141997>

[Chris Dumez]

Created attachment 332931 [details]
Patch

[youenn fablet]

Comment on attachment 332931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332931&action=review

> Source/WebCore/ChangeLog:10
> +        This does not match the spec but this is what Firefox and Chrome do. This is also what the

Change makes sense to me. Can we file an issue against the spec?

> LayoutTests/ChangeLog:9
> +        Add layout test coverage. I have verified that this test is passing in both Firefox and Chrome.

Since this test is useful to ensure interop with Chrome and Firefox will be kept, it would be good to make this as a WPT test.
Given how the test is simple, making it a WPT test should be straightforward.

> LayoutTests/http/tests/workers/service/resources/install-fails-worker.js:5
> +        }, 10);

I am not clear why we need to have a 10 millisecond timer there.
Cannot we just write "event.waitUntil(Promise.reject());"

[Chris Dumez]

Comment on attachment 332931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332931&action=review

>> Source/WebCore/ChangeLog:10
>> +        This does not match the spec but this is what Firefox and Chrome do. This is also what the
> 
> Change makes sense to me. Can we file an issue against the spec?

Yes. I'll do that.

>> LayoutTests/ChangeLog:9
>> +        Add layout test coverage. I have verified that this test is passing in both Firefox and Chrome.
> 
> Since this test is useful to ensure interop with Chrome and Firefox will be kept, it would be good to make this as a WPT test.
> Given how the test is simple, making it a WPT test should be straightforward.

Are you not satisfied with the existing WPT test? I wrote this one for clarity and to cover only the exact case. However, the WPT test does cover this change since it started passing.

>> LayoutTests/http/tests/workers/service/resources/install-fails-worker.js:5
>> +        }, 10);
> 
> I am not clear why we need to have a 10 millisecond timer there.
> Cannot we just write "event.waitUntil(Promise.reject());"

Ok.

[Chris Dumez]

Created attachment 332936 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 332936 [details]
Patch

Clearing flags on attachment: 332936

Committed r227997: <https://trac.webkit.org/changeset/227997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.