Implement "Try Clear Registration" algorithm: - https://w3c.github.io/ServiceWorker/#try-clear-registration-algorithm
Created attachment 327104 [details] Patch
Created attachment 327109 [details] Patch
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
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
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?
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
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
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.
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.
Created attachment 327121 [details] Patch
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.
Created attachment 327127 [details] Patch
Comment on attachment 327127 [details] Patch Clearing flags on attachment: 327127 Committed r224947: <https://trac.webkit.org/changeset/224947>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35603950>