ServiceWorkerThread's m_extendedEvents gets destroyed on the wrong thread
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 22:35:25 +0000 (22:35 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 22:35:25 +0000 (22:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180216

Reviewed by Brady Eidson.

ServiceWorkerThread's m_extendedEvents gets destroyed on the wrong thread. Those events live on the worker
thread so we should destroy them on the worker thread, not the main thread. To address the issue, m_extendedEvents
was moved to ServiceWorkerGlobalScope, which actually lives on the right thread.

* workers/service/ServiceWorkerGlobalScope.cpp:
(WebCore::ServiceWorkerGlobalScope::updateExtendedEventsSet):
* workers/service/ServiceWorkerGlobalScope.h:
* workers/service/context/ServiceWorkerThread.cpp:
(WebCore::ServiceWorkerThread::postFetchTask):
(WebCore::ServiceWorkerThread::postMessageToServiceWorkerGlobalScope):
(WebCore::ServiceWorkerThread::updateExtendedEventsSet): Deleted.
* workers/service/context/ServiceWorkerThread.h:
(WebCore::ServiceWorkerThread::hasPendingEvents const): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225351 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMGuardedObject.cpp
Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp
Source/WebCore/workers/service/ServiceWorkerGlobalScope.h
Source/WebCore/workers/service/context/ServiceWorkerThread.cpp
Source/WebCore/workers/service/context/ServiceWorkerThread.h

index 24874cc..5f84ef3 100644 (file)
@@ -1,5 +1,26 @@
 2017-11-30  Chris Dumez  <cdumez@apple.com>
 
+        ServiceWorkerThread's m_extendedEvents gets destroyed on the wrong thread
+        https://bugs.webkit.org/show_bug.cgi?id=180216
+
+        Reviewed by Brady Eidson.
+
+        ServiceWorkerThread's m_extendedEvents gets destroyed on the wrong thread. Those events live on the worker
+        thread so we should destroy them on the worker thread, not the main thread. To address the issue, m_extendedEvents
+        was moved to ServiceWorkerGlobalScope, which actually lives on the right thread.
+
+        * workers/service/ServiceWorkerGlobalScope.cpp:
+        (WebCore::ServiceWorkerGlobalScope::updateExtendedEventsSet):
+        * workers/service/ServiceWorkerGlobalScope.h:
+        * workers/service/context/ServiceWorkerThread.cpp:
+        (WebCore::ServiceWorkerThread::postFetchTask):
+        (WebCore::ServiceWorkerThread::postMessageToServiceWorkerGlobalScope):
+        (WebCore::ServiceWorkerThread::updateExtendedEventsSet): Deleted.
+        * workers/service/context/ServiceWorkerThread.h:
+        (WebCore::ServiceWorkerThread::hasPendingEvents const): Deleted.
+
+2017-11-30  Chris Dumez  <cdumez@apple.com>
+
         SWServerToContextConnection / SWServerWorker do not need to be ThreadSafeRefCounted
         https://bugs.webkit.org/show_bug.cgi?id=180214
 
index ed8dbd1..a835903 100644 (file)
@@ -53,6 +53,7 @@ void DOMGuardedObject::clear()
         m_globalObject->guardedObjects(locker).remove(this);
     }
     m_guarded.clear();
+    m_globalObject.clear();
 }
 
 void DOMGuardedObject::contextDestroyed()
index d6a3f17..29f03ba 100644 (file)
@@ -28,6 +28,8 @@
 
 #if ENABLE(SERVICE_WORKER)
 
+#include "ExtendableEvent.h"
+#include "SWContextManager.h"
 #include "ServiceWorkerClient.h"
 #include "ServiceWorkerClients.h"
 #include "ServiceWorkerThread.h"
@@ -79,6 +81,36 @@ void ServiceWorkerGlobalScope::removeServiceWorkerClient(ServiceWorkerClient& cl
     ASSERT_UNUSED(isRemoved, isRemoved);
 }
 
+// https://w3c.github.io/ServiceWorker/#update-service-worker-extended-events-set-algorithm
+void ServiceWorkerGlobalScope::updateExtendedEventsSet(ExtendableEvent* newEvent)
+{
+    ASSERT(!isMainThread());
+    ASSERT(!newEvent || !newEvent->isBeingDispatched());
+    bool hadPendingEvents = hasPendingEvents();
+    m_extendedEvents.removeAllMatching([](auto& event) {
+        return !event->pendingPromiseCount();
+    });
+
+    if (newEvent && newEvent->pendingPromiseCount()) {
+        m_extendedEvents.append(*newEvent);
+        newEvent->whenAllExtendLifetimePromisesAreSettled([this](auto&&) {
+            updateExtendedEventsSet();
+        });
+        // Clear out the event's target as it is the WorkerGlobalScope and we do not want to keep it
+        // alive unnecessarily.
+        newEvent->setTarget(nullptr);
+    }
+
+    bool hasPendingEvents = this->hasPendingEvents();
+    if (hasPendingEvents == hadPendingEvents)
+        return;
+
+    callOnMainThread([threadIdentifier = thread().identifier(), hasPendingEvents] {
+        if (auto* connection = SWContextManager::singleton().connection())
+            connection->setServiceWorkerHasPendingEvents(threadIdentifier, hasPendingEvents);
+    });
+}
+
 } // namespace WebCore
 
 #endif // ENABLE(SERVICE_WORKER)
index 8926264..a7db0cc 100644 (file)
@@ -35,6 +35,7 @@
 namespace WebCore {
 
 class DeferredPromise;
+class ExtendableEvent;
 struct ServiceWorkerClientData;
 class ServiceWorkerClient;
 class ServiceWorkerClients;
@@ -64,13 +65,18 @@ public:
     void addServiceWorkerClient(ServiceWorkerClient&);
     void removeServiceWorkerClient(ServiceWorkerClient&);
 
+    void updateExtendedEventsSet(ExtendableEvent* newEvent = nullptr);
+
 private:
     ServiceWorkerGlobalScope(const ServiceWorkerContextData&, const URL&, const String& identifier, const String& userAgent, bool isOnline, ServiceWorkerThread&, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy*, SocketProvider*, PAL::SessionID);
 
+    bool hasPendingEvents() const { return !m_extendedEvents.isEmpty(); }
+
     ServiceWorkerContextData m_contextData;
     Ref<ServiceWorkerRegistration> m_registration;
     Ref<ServiceWorkerClients> m_clients;
     HashMap<ServiceWorkerClientIdentifier, ServiceWorkerClient*> m_clientMap;
+    Vector<Ref<ExtendableEvent>> m_extendedEvents;
 };
 
 } // namespace WebCore
index 059a974..1e50883 100644 (file)
@@ -99,8 +99,9 @@ void ServiceWorkerThread::postFetchTask(Ref<ServiceWorkerFetch::Client>&& client
     // 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([this, client = WTFMove(client), clientId, request = request.isolatedCopy(), options = options.isolatedCopy()] (ScriptExecutionContext& context) mutable {
-        auto fetchEvent = ServiceWorkerFetch::dispatchFetchEvent(WTFMove(client), downcast<WorkerGlobalScope>(context), clientId, WTFMove(request), WTFMove(options));
-        updateExtendedEventsSet(fetchEvent.ptr());
+        auto& serviceWorkerGlobalScope = downcast<ServiceWorkerGlobalScope>(context);
+        auto fetchEvent = ServiceWorkerFetch::dispatchFetchEvent(WTFMove(client), serviceWorkerGlobalScope, clientId, WTFMove(request), WTFMove(options));
+        serviceWorkerGlobalScope.updateExtendedEventsSet(fetchEvent.ptr());
     }, WorkerRunLoop::defaultMode());
 }
 
@@ -114,7 +115,7 @@ void ServiceWorkerThread::postMessageToServiceWorkerGlobalScope(Ref<SerializedSc
         auto messageEvent = ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message), sourceOrigin->toString(), { }, ExtendableMessageEventSource { source });
         serviceWorkerGlobalScope.dispatchEvent(messageEvent);
         serviceWorkerGlobalScope.thread().workerObjectProxy().confirmMessageFromWorkerObject(serviceWorkerGlobalScope.hasPendingActivity());
-        updateExtendedEventsSet(messageEvent.ptr());
+        serviceWorkerGlobalScope.updateExtendedEventsSet(messageEvent.ptr());
     });
     runLoop().postTask(WTFMove(task));
 }
@@ -164,36 +165,6 @@ void ServiceWorkerThread::fireActivateEvent()
     runLoop().postTask(WTFMove(task));
 }
 
-// https://w3c.github.io/ServiceWorker/#update-service-worker-extended-events-set-algorithm
-void ServiceWorkerThread::updateExtendedEventsSet(ExtendableEvent* newEvent)
-{
-    ASSERT(!isMainThread());
-    ASSERT(!newEvent || !newEvent->isBeingDispatched());
-    bool hadPendingEvents = hasPendingEvents();
-    m_extendedEvents.removeAllMatching([](auto& event) {
-        return !event->pendingPromiseCount();
-    });
-
-    if (newEvent && newEvent->pendingPromiseCount()) {
-        m_extendedEvents.append(*newEvent);
-        newEvent->whenAllExtendLifetimePromisesAreSettled([this](auto&&) {
-            updateExtendedEventsSet();
-        });
-        // Clear out the event's target as it is the WorkerGlobalScope and we do not want to keep it
-        // alive unnecessarily.
-        newEvent->setTarget(nullptr);
-    }
-
-    bool hasPendingEvents = this->hasPendingEvents();
-    if (hasPendingEvents == hadPendingEvents)
-        return;
-
-    callOnMainThread([identifier = this->identifier(), hasPendingEvents] {
-        if (auto* connection = SWContextManager::singleton().connection())
-            connection->setServiceWorkerHasPendingEvents(identifier, hasPendingEvents);
-    });
-}
-
 } // namespace WebCore
 
 #endif // ENABLE(SERVICE_WORKER)
index 11e73a5..ea51de7 100644 (file)
@@ -74,12 +74,8 @@ protected:
 private:
     WEBCORE_EXPORT ServiceWorkerThread(const ServiceWorkerContextData&, PAL::SessionID, WorkerLoaderProxy&, WorkerDebuggerProxy&);
 
-    void updateExtendedEventsSet(ExtendableEvent* newEvent = nullptr);
-    bool hasPendingEvents() const { return !m_extendedEvents.isEmpty(); }
-
     ServiceWorkerContextData m_data;
     WorkerObjectProxy& m_workerObjectProxy;
-    Vector<Ref<ExtendableEvent>> m_extendedEvents;
 };
 
 } // namespace WebCore