Bug 179822 – [Service Workers] Implement "Notify Controller Change" algorithm

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

[Chris Dumez]

Implement "Notify Controller Change" algorithm:
- https://w3c.github.io/ServiceWorker/#notify-controller-change

[Chris Dumez]

Created attachment 327193 [details]
WIP Patch

[Chris Dumez]

Created attachment 327194 [details]
Patch

[youenn fablet]

Comment on attachment 327194 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:419
> +    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this)](ScriptExecutionContext&) mutable {

I know the spec is written this way but It seems a bit weird to post a task here for dispatching an event.
It does not seem observable to dispatch the event directly without the post task, at least in the current context it is used.
Might be safer to stick to the spec though, not sure...

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:424
> +        dispatchEvent(controllerChangeEvent);

do we need controllerChangeEvent variable?

> Source/WebCore/workers/service/server/SWClientConnection.cpp:168
> +void SWClientConnection::notifyClientsOfControllerChange(const HashSet<uint64_t>& scriptExecutionContexts, const ServiceWorkerData& newController)

newController can probably be an r value.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:177
> +        client->setActiveServiceWorker(ServiceWorker::getOrCreate(*client, ServiceWorkerData { newController }));

Maybe we should assert that client has an active service worker.
Not sure whether we could also assert that we are actually changing from worker.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:235
> +    // - Set clientâs active worker to registrationâs active worker.

I see client^as here.

> LayoutTests/http/tests/workers/service/controller-change.html:38
> +    let frame = await withFrame(scopeURL);

In sw-test-pre.js, there is an interceptedFrame function that could be reused to replace register and withFrame.

[Chris Dumez]

Created attachment 327293 [details]
Patch

[Chris Dumez]

Created attachment 327294 [details]
Patch

[Chris Dumez]

Comment on attachment 327294 [details]
Patch

Clearing flags on attachment: 327294

Committed r225011: <https://trac.webkit.org/changeset/225011>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35629134>

[Matt Lewis]

This change has cause the test:
imported/w3c/web-platform-tests/service-workers/service-worker/claim-affect-other-registration.https.html
to start crashing

https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r225170%20(1241)/results.html
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/1241

See bug:
https://bugs.webkit.org/show_bug.cgi?id=180049