Bug 179791 – [Service Worker] Implement "Try Clear Registration" algorithm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Implement "Try Clear Registration" algorithm:
- https://w3c.github.io/ServiceWorker/#try-clear-registration-algorithm

[Chris Dumez]

Created attachment 327104 [details]
Patch

[Chris Dumez]

Created attachment 327109 [details]
Patch

[EWS Watchlist]

Comment on attachment 327109 [details]
Patch

Attachment 327109 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5264453

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html

[EWS Watchlist]

Created attachment 327115 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 327109 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:550
> +    uint64_t contextIdentifier = is<Document>(*this) ? downcast<Document>(*this).identifier() : 0;

It might be better to exit early if we are not a Document.
I am not sure calling ServiceWorkerProvider::singleton() will work properly on worker thread.

> Source/WebCore/workers/service/server/SWClientConnection.h:75
> +    virtual void serviceWorkerStartedControllingClient(ServiceWorkerIdentifier, uint64_t scriptExecutionContextIdentifier) = 0;

Would be nice to have a ScriptExcutionContextIdentifier at some point :)

> Source/WebCore/workers/service/server/SWServer.cpp:333
> +    auto* registration = m_registrations.get(serviceWorker->registrationKey());

Are there cases where we have a worker but no longer a registration?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:156
> +    auto it = m_clientsUsingRegistration.find(clientIdentifier.serverConnectionIdentifier);

iterator

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:158
> +        return;

Should we assert that it is not the end?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:160
> +    it->value.remove(clientIdentifier.scriptExecutionContextIdentifier);

Should we assert remove returned true?
We could also do below test only if remove returned true.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https-expected.txt:4
> +FAIL Verify matchAll() with {window, sharedworker, worker} client types promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"

Test probably flaky or maybe regressing?

[EWS Watchlist]

Comment on attachment 327109 [details]
Patch

Attachment 327109 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5264461

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html

[EWS Watchlist]

Created attachment 327118 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 327109 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServer.cpp:333
>> +    auto* registration = m_registrations.get(serviceWorker->registrationKey());
> 
> Are there cases where we have a worker but no longer a registration?

Yes, I saw it in the tests. This can happen when a document that had an active service worker gets destroyed but the registrations got cleared on storage process side.

[Chris Dumez]

Comment on attachment 327109 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https-expected.txt:4
>> +FAIL Verify matchAll() with {window, sharedworker, worker} client types promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"
> 
> Test probably flaky or maybe regressing?

Will take a look.

[Chris Dumez]

Created attachment 327121 [details]
Patch

[Chris Dumez]

Comment on attachment 327109 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServerRegistration.cpp:158
>> +        return;
> 
> Should we assert that it is not the end?

Done.

>>> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https-expected.txt:4
>>> +FAIL Verify matchAll() with {window, sharedworker, worker} client types promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"
>> 
>> Test probably flaky or maybe regressing?
> 
> Will take a look.

What happens is that when the test unregisters, the registration no longer gets cleared because it still has a user. The reason it has a user is because the previous subtest (Verify matchAll() with window client type) creates an iframe controlled by the registration. That iframe is supposed to be removed at the end of the subtest in question but we do not get there because we abort early (due to not supporting passing MessagePort objects to postMessage). If I comment out the previous subtest, then this last subtest fails with "ReferenceError: Can't find variable: SharedWorker", as expected.

[Chris Dumez]

Created attachment 327127 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327127 [details]
Patch

Clearing flags on attachment: 327127

Committed r224947: <https://trac.webkit.org/changeset/224947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35603950>