[webkit-reviews] review granted: [Bug 181698] 'fetch' event may be sent to a service worker before its state is set to 'activated' : [Attachment 331418] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 181698: 'fetch' event may be sent to a service worker before its state is
set to 'activated'
https://bugs.webkit.org/show_bug.cgi?id=181698

Attachment 331418: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 331418
  --> https://bugs.webkit.org/attachment.cgi?id=331418
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)?