- Revision
- 227070
- Author
- [email protected]
- Date
- 2018-01-17 10:07:32 -0800 (Wed, 17 Jan 2018)
Log Message
'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
<rdar://problem/36554856>
Reviewed by Youenn Fablet.
'fetch' event may be sent to a service worker before its state is set to 'activated'.
When the registration's active worker needs to intercept a load, and its state is 'activating',
we queue the request to send the fetch event in SWServerWorker::m_whenActivatedHandlers.
Once the SWServerWorker::setState() is called with 'activated' state, we then call the
handlers in m_whenActivatedHandlers to make send the fetch event now that the worker is
activated. The issue is that even though the worker is activated and its state was set to
'activated' on Storage process side, we had not yet notified the ServiceWorker process
of the service worker's new state yet.
To address the issue, we now make sure that SWServerWorker::m_whenActivatedHandlers are
called *after* we've sent the IPC to the ServiceWorker process to update the worker's
state to 'activated'. Also, we now call ServiceWorkerFetch::dispatchFetchEvent()
asynchronously in a postTask() as the service worker's state is also updated asynchronously
in a postTask. This is as per specification [1], which says to "queue a task" to fire
the fetch event.
[1] https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm (step 18)
No new tests, covered by imported/w3c/web-platform-tests/service-workers/service-worker/fetch-waits-for-activate.https.html
which hits the new assertion without the fix.
* workers/service/context/ServiceWorkerFetch.cpp:
(WebCore::ServiceWorkerFetch::dispatchFetchEvent):
Add assertions to make sure that we dispatch the fetch event on the right worker and
that the worker is in 'activated' state.
* workers/service/context/ServiceWorkerThread.cpp:
(WebCore::ServiceWorkerThread::postFetchTask):
Queue a task to fire the fetch event as per:
- https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm (step 18)
We need to match the specification exactly here or things will happen in the wrong
order. In particular, things like "update registration state" and "update worker state"
might happen *after* firing the fetch event, even though the IPC for "update registration/worker
state" was sent before the "fire fetch event" one, because the code for updating a registration/
worker state already queues a task, as per the specification.
* workers/service/server/SWServerRegistration.cpp:
(WebCore::SWServerRegistration::updateWorkerState):
* workers/service/server/SWServerRegistration.h:
* workers/service/server/SWServerWorker.cpp:
(WebCore::SWServerWorker::setState):
Move code to send the IPC to the Service Worker process whenever the service worker's state
needs to be updated from SWServerRegistration::updateWorkerState() to SWServerWorker::setState().
This way, we can make sure the IPC is sent *before* we call the m_whenActivatedHandlers handlers,
as they may also send IPC to the Service Worker process, and we need to make sure this IPC happens
after so that the service worker is in the right state.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (227069 => 227070)
--- trunk/Source/WebCore/ChangeLog 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/ChangeLog 2018-01-17 18:07:32 UTC (rev 227070)
@@ -1,3 +1,58 @@
+2018-01-17 Chris Dumez <[email protected]>
+
+ '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
+ <rdar://problem/36554856>
+
+ Reviewed by Youenn Fablet.
+
+ 'fetch' event may be sent to a service worker before its state is set to 'activated'.
+ When the registration's active worker needs to intercept a load, and its state is 'activating',
+ we queue the request to send the fetch event in SWServerWorker::m_whenActivatedHandlers.
+ Once the SWServerWorker::setState() is called with 'activated' state, we then call the
+ handlers in m_whenActivatedHandlers to make send the fetch event now that the worker is
+ activated. The issue is that even though the worker is activated and its state was set to
+ 'activated' on Storage process side, we had not yet notified the ServiceWorker process
+ of the service worker's new state yet.
+
+ To address the issue, we now make sure that SWServerWorker::m_whenActivatedHandlers are
+ called *after* we've sent the IPC to the ServiceWorker process to update the worker's
+ state to 'activated'. Also, we now call ServiceWorkerFetch::dispatchFetchEvent()
+ asynchronously in a postTask() as the service worker's state is also updated asynchronously
+ in a postTask. This is as per specification [1], which says to "queue a task" to fire
+ the fetch event.
+
+ [1] https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm (step 18)
+
+ No new tests, covered by imported/w3c/web-platform-tests/service-workers/service-worker/fetch-waits-for-activate.https.html
+ which hits the new assertion without the fix.
+
+ * workers/service/context/ServiceWorkerFetch.cpp:
+ (WebCore::ServiceWorkerFetch::dispatchFetchEvent):
+ Add assertions to make sure that we dispatch the fetch event on the right worker and
+ that the worker is in 'activated' state.
+
+ * workers/service/context/ServiceWorkerThread.cpp:
+ (WebCore::ServiceWorkerThread::postFetchTask):
+ Queue a task to fire the fetch event as per:
+ - https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm (step 18)
+ We need to match the specification exactly here or things will happen in the wrong
+ order. In particular, things like "update registration state" and "update worker state"
+ might happen *after* firing the fetch event, even though the IPC for "update registration/worker
+ state" was sent before the "fire fetch event" one, because the code for updating a registration/
+ worker state already queues a task, as per the specification.
+
+ * workers/service/server/SWServerRegistration.cpp:
+ (WebCore::SWServerRegistration::updateWorkerState):
+ * workers/service/server/SWServerRegistration.h:
+ * workers/service/server/SWServerWorker.cpp:
+ (WebCore::SWServerWorker::setState):
+ Move code to send the IPC to the Service Worker process whenever the service worker's state
+ needs to be updated from SWServerRegistration::updateWorkerState() to SWServerWorker::setState().
+ This way, we can make sure the IPC is sent *before* we call the m_whenActivatedHandlers handlers,
+ as they may also send IPC to the Service Worker process, and we need to make sure this IPC happens
+ after so that the service worker is in the right state.
+
2018-01-17 Stephan Szabo <[email protected]>
Page.cpp only sees forward declaration of ApplicationStateChangeListener when ENABLE(VIDEO) is off
Modified: trunk/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp (227069 => 227070)
--- trunk/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp 2018-01-17 18:07:32 UTC (rev 227070)
@@ -34,6 +34,7 @@
#include "FetchRequest.h"
#include "FetchResponse.h"
#include "ResourceRequest.h"
+#include "ServiceWorker.h"
#include "ServiceWorkerClientIdentifier.h"
#include "WorkerGlobalScope.h"
@@ -98,6 +99,10 @@
bool isNavigation = options.mode == FetchOptions::Mode::Navigate;
bool isNonSubresourceRequest = WebCore::isNonSubresourceRequest(options.destination);
+ ASSERT(globalScope.registration().active());
+ ASSERT(globalScope.registration().active()->identifier() == globalScope.thread().identifier());
+ ASSERT(globalScope.registration().active()->state() == ServiceWorkerState::Activated);
+
auto* formData = request.httpBody();
std::optional<FetchBody> body;
if (formData && !formData->isEmpty()) {
Modified: trunk/Source/WebCore/workers/service/context/ServiceWorkerThread.cpp (227069 => 227070)
--- trunk/Source/WebCore/workers/service/context/ServiceWorkerThread.cpp 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/workers/service/context/ServiceWorkerThread.cpp 2018-01-17 18:07:32 UTC (rev 227070)
@@ -96,7 +96,9 @@
// FIXME: instead of directly using runLoop(), we should be using something like WorkerGlobalScopeProxy.
// FIXME: request and options come straigth from IPC so are already isolated. We should be able to take benefit of that.
runLoop().postTaskForMode([client = WTFMove(client), clientId, request = request.isolatedCopy(), referrer = referrer.isolatedCopy(), options = options.isolatedCopy()] (ScriptExecutionContext& context) mutable {
- ServiceWorkerFetch::dispatchFetchEvent(WTFMove(client), downcast<ServiceWorkerGlobalScope>(context), clientId, WTFMove(request), WTFMove(referrer), WTFMove(options));
+ context.postTask([client = WTFMove(client), clientId, request = WTFMove(request), referrer = WTFMove(referrer), options = WTFMove(options)] (ScriptExecutionContext& context) mutable {
+ ServiceWorkerFetch::dispatchFetchEvent(WTFMove(client), downcast<ServiceWorkerGlobalScope>(context), clientId, WTFMove(request), WTFMove(referrer), WTFMove(options));
+ });
}, WorkerRunLoop::defaultMode());
}
Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp (227069 => 227070)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.cpp 2018-01-17 18:07:32 UTC (rev 227070)
@@ -102,10 +102,6 @@
LOG(ServiceWorker, "Updating worker %p state to %i (%p)", &worker, (int)state, this);
worker.setState(state);
-
- forEachConnection([&](auto& connection) {
- connection.updateWorkerStateInClient(worker.identifier(), state);
- });
}
void SWServerRegistration::setUpdateViaCache(ServiceWorkerUpdateViaCache updateViaCache)
Modified: trunk/Source/WebCore/workers/service/server/SWServerRegistration.h (227069 => 227070)
--- trunk/Source/WebCore/workers/service/server/SWServerRegistration.h 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/workers/service/server/SWServerRegistration.h 2018-01-17 18:07:32 UTC (rev 227070)
@@ -92,9 +92,9 @@
void tryActivate();
void didFinishActivation(ServiceWorkerIdentifier);
-private:
void forEachConnection(const WTF::Function<void(SWServer::Connection&)>&);
+private:
void activate();
void handleClientUnload();
Modified: trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp (227069 => 227070)
--- trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp 2018-01-17 17:36:19 UTC (rev 227069)
+++ trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp 2018-01-17 18:07:32 UTC (rev 227070)
@@ -175,6 +175,14 @@
m_data.state = state;
+ auto* registration = m_server.getRegistration(m_registrationKey);
+ ASSERT(registration || state == ServiceWorkerState::Redundant);
+ if (registration) {
+ registration->forEachConnection([&](auto& connection) {
+ connection.updateWorkerStateInClient(identifier(), state);
+ });
+ }
+
if (state == ServiceWorkerState::Activated || state == ServiceWorkerState::Redundant)
callWhenActivatedHandler(state == ServiceWorkerState::Activated);
}
