Bug 181698 – 'fetch' event may be sent to a service worker before its state is set to 'activated'

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Chris Dumez]

'fetch' event may be sent to a service worker before its state is set to 'activated'.

[Radar WebKit Bug Importer]

<rdar://problem/36554856>

[Chris Dumez]

Created attachment 331418 [details]
Patch

[youenn fablet]

Comment on attachment 331418 [details]
Patch

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

> Source/WebCore/ChangeLog:32
> +        Add assertions to make sure that the we dispatch the fetch event on a the right worker and

s/a the/the/

> Source/WebCore/ChangeLog:40
> +        order.

Can you detail which things will happen in the wrong order? Is it causing some flakiness in the current tests?
We are already queuing a task when going from main thread to worker thread loop so we are already respecting what the spec says.
We are doing this queue-and-requeue for install/activate events but I  do not think they are racy with fetch event.
Ideally, I think we would like to not need this queue-and-requeue for activate/install events.

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:-108
> -    });

The other approach is probably to put worker.setState(state) after the forEach.
That would probably remove the need for the registration null check but the intent is probably clearer.
Or maybe a comment in this method would be sufficiently clear.

> Source/WebCore/workers/service/server/SWServerWorker.cpp:178
> +    if (auto* registration = m_server.getRegistration(m_registrationKey)) {

I guess we could write something like ASSERT(registration || state == terminatingOrRedundant)?

[Chris Dumez]

Comment on attachment 331418 [details]
Patch

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

>> Source/WebCore/ChangeLog:40
>> +        order.
> 
> Can you detail which things will happen in the wrong order? Is it causing some flakiness in the current tests?
> We are already queuing a task when going from main thread to worker thread loop so we are already respecting what the spec says.
> We are doing this queue-and-requeue for install/activate events but I  do not think they are racy with fetch event.
> Ideally, I think we would like to not need this queue-and-requeue for activate/install events.

The issue is with the "update registration state" [1] and "update worker state" [2]. Both say to "queue a task" in the spec. The IPC I moved *before* calling the handlers is to update the worker state. Moving the IPC to update the worker state before calling the activation handlers (which fire the fetch event) would make things flaky if dispatching the fetch event would not "queue a task", because the code to update the worker state (and fire the state change event) already queued a task, as per spec.

[1] https://w3c.github.io/ServiceWorker/#update-registration-state-algorithm
[2] https://w3c.github.io/ServiceWorker/#update-state-algorithm

[Chris Dumez]

Created attachment 331501 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331501 [details]
Patch

Clearing flags on attachment: 331501

Committed r227070: <https://trac.webkit.org/changeset/227070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Bug 242229 – [mac] ASSERTION FAILED: globalScope.registration().active()->state() == ServiceWorkerState::Activated: imported/w3c/web-platform-tests/service-workers/service-worker/claim-with-redirect.https.html is randomly crashing