'fetch' event may be sent to a service worker before its state is set to 'activated'.
<rdar://problem/36554856>
Created attachment 331418 [details] Patch
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)?
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
Created attachment 331501 [details] Patch
Comment on attachment 331501 [details] Patch Clearing flags on attachment: 331501 Committed r227070: <https://trac.webkit.org/changeset/227070>
All reviewed patches have been landed. Closing bug.
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