Get rid of ServiceWorker::allWorkers() hashmap
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Nov 2017 00:28:42 +0000 (00:28 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Nov 2017 00:28:42 +0000 (00:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180111

Reviewed by Brady Eidson.

Get rid of ServiceWorker::allWorkers() hashmap as it is not thread safe and we'll soon have
ServiceWorker objects living in various service worker threads.

Instead, we now have a per-ScriptExecutionContext map, which is inherently thread-safe.

No new tests, no web-facing behavior change.

* dom/ScriptExecutionContext.cpp:
(WebCore::ScriptExecutionContext::registerServiceWorker):
(WebCore::ScriptExecutionContext::unregisterServiceWorker):
* dom/ScriptExecutionContext.h:
(WebCore::ScriptExecutionContext::serviceWorker):
* workers/service/ServiceWorker.cpp:
(WebCore::ServiceWorker::getOrCreate):
(WebCore::ServiceWorker::ServiceWorker):
(WebCore::ServiceWorker::~ServiceWorker):
(WebCore::ServiceWorker::stop):
* workers/service/ServiceWorker.h:
* workers/service/server/SWClientConnection.cpp:
(WebCore::SWClientConnection::updateWorkerState):

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

Source/WebCore/ChangeLog
Source/WebCore/dom/ScriptExecutionContext.cpp
Source/WebCore/dom/ScriptExecutionContext.h
Source/WebCore/workers/service/ServiceWorker.cpp
Source/WebCore/workers/service/ServiceWorker.h
Source/WebCore/workers/service/server/SWClientConnection.cpp

index 56566f0..0c6483f 100644 (file)
@@ -1,3 +1,31 @@
+2017-11-28  Chris Dumez  <cdumez@apple.com>
+
+        Get rid of ServiceWorker::allWorkers() hashmap
+        https://bugs.webkit.org/show_bug.cgi?id=180111
+
+        Reviewed by Brady Eidson.
+
+        Get rid of ServiceWorker::allWorkers() hashmap as it is not thread safe and we'll soon have
+        ServiceWorker objects living in various service worker threads.
+
+        Instead, we now have a per-ScriptExecutionContext map, which is inherently thread-safe.
+
+        No new tests, no web-facing behavior change.
+
+        * dom/ScriptExecutionContext.cpp:
+        (WebCore::ScriptExecutionContext::registerServiceWorker):
+        (WebCore::ScriptExecutionContext::unregisterServiceWorker):
+        * dom/ScriptExecutionContext.h:
+        (WebCore::ScriptExecutionContext::serviceWorker):
+        * workers/service/ServiceWorker.cpp:
+        (WebCore::ServiceWorker::getOrCreate):
+        (WebCore::ServiceWorker::ServiceWorker):
+        (WebCore::ServiceWorker::~ServiceWorker):
+        (WebCore::ServiceWorker::stop):
+        * workers/service/ServiceWorker.h:
+        * workers/service/server/SWClientConnection.cpp:
+        (WebCore::SWClientConnection::updateWorkerState):
+
 2017-11-28  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         [CG] PostScript images should be supported if they are sub-resource images
index b9eec93..2199d4a 100644 (file)
@@ -560,6 +560,17 @@ void ScriptExecutionContext::setActiveServiceWorker(RefPtr<ServiceWorker>&& serv
         connection.serviceWorkerStartedControllingClient(m_activeServiceWorker->identifier(), m_activeServiceWorker->registrationIdentifier(), downcast<Document>(*this).identifier());
 }
 
+void ScriptExecutionContext::registerServiceWorker(ServiceWorker& serviceWorker)
+{
+    auto addResult = m_serviceWorkers.add(serviceWorker.identifier(), &serviceWorker);
+    ASSERT_UNUSED(addResult, addResult.isNewEntry);
+}
+
+void ScriptExecutionContext::unregisterServiceWorker(ServiceWorker& serviceWorker)
+{
+    m_serviceWorkers.remove(serviceWorker.identifier());
+}
+
 ServiceWorkerContainer* ScriptExecutionContext::serviceWorkerContainer()
 {
     NavigatorBase* navigator = nullptr;
index 6f61ca3..18dd9c7 100644 (file)
@@ -30,6 +30,7 @@
 #include "ActiveDOMObject.h"
 #include "DOMTimer.h"
 #include "SecurityContext.h"
+#include "ServiceWorkerIdentifier.h"
 #include <heap/HandleTypes.h>
 #include <runtime/ConsoleTypes.h>
 #include <wtf/CrossThreadTask.h>
@@ -240,6 +241,10 @@ public:
     ServiceWorker* activeServiceWorker() const;
     void setActiveServiceWorker(RefPtr<ServiceWorker>&&);
 
+    void registerServiceWorker(ServiceWorker&);
+    void unregisterServiceWorker(ServiceWorker&);
+    ServiceWorker* serviceWorker(ServiceWorkerIdentifier identifier) { return m_serviceWorkers.get(identifier); }
+
     ServiceWorkerContainer* serviceWorkerContainer();
 #endif
 
@@ -313,6 +318,7 @@ private:
 
 #if ENABLE(SERVICE_WORKER)
     RefPtr<ServiceWorker> m_activeServiceWorker;
+    HashMap<ServiceWorkerIdentifier, ServiceWorker*> m_serviceWorkers;
 #endif
 };
 
index 3b98435..12970ea 100644 (file)
 
 namespace WebCore {
 
-const HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& ServiceWorker::allWorkers()
-{
-    return mutableAllWorkers();
-}
-
-HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& ServiceWorker::mutableAllWorkers()
-{
-    // FIXME: Once we support service workers from workers, this will need to change.
-    RELEASE_ASSERT(isMainThread());
-    
-    static NeverDestroyed<HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>> allWorkersMap;
-    return allWorkersMap;
-}
-
 Ref<ServiceWorker> ServiceWorker::getOrCreate(ScriptExecutionContext& context, ServiceWorkerData&& data)
 {
-    auto it = allWorkers().find(data.identifier);
-    if (it != allWorkers().end()) {
-        for (auto& worker : it->value) {
-            if (worker->scriptExecutionContext() == &context) {
-                ASSERT(!worker->m_isStopped);
-                return *worker;
-            }
-        }
-    }
+    if (auto existingServiceWorker = context.serviceWorker(data.identifier))
+        return *existingServiceWorker;
     return adoptRef(*new ServiceWorker(context, WTFMove(data)));
 }
 
@@ -75,10 +54,7 @@ ServiceWorker::ServiceWorker(ScriptExecutionContext& context, ServiceWorkerData&
 {
     suspendIfNeeded();
 
-    auto result = mutableAllWorkers().ensure(identifier(), [] {
-        return HashSet<ServiceWorker*>();
-    });
-    result.iterator->value.add(this);
+    context.registerServiceWorker(*this);
 
     relaxAdoptionRequirement();
     updatePendingActivityForEventDispatch();
@@ -86,13 +62,8 @@ ServiceWorker::ServiceWorker(ScriptExecutionContext& context, ServiceWorkerData&
 
 ServiceWorker::~ServiceWorker()
 {
-    auto iterator = mutableAllWorkers().find(identifier());
-
-    ASSERT(iterator->value.contains(this));
-    iterator->value.remove(this);
-
-    if (iterator->value.isEmpty())
-        mutableAllWorkers().remove(iterator);
+    if (auto* context = scriptExecutionContext())
+        context->unregisterServiceWorker(*this);
 }
 
 void ServiceWorker::scheduleTaskToUpdateState(State state)
@@ -178,6 +149,7 @@ bool ServiceWorker::canSuspendForDocumentSuspension() const
 void ServiceWorker::stop()
 {
     m_isStopped = true;
+    scriptExecutionContext()->unregisterServiceWorker(*this);
     updatePendingActivityForEventDispatch();
 }
 
index d36ab74..e771b1a 100644 (file)
@@ -64,11 +64,8 @@ public:
     using RefCounted::ref;
     using RefCounted::deref;
 
-    static const HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& allWorkers();
-
 private:
     ServiceWorker(ScriptExecutionContext&, ServiceWorkerData&&);
-    static HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& mutableAllWorkers();
     void updatePendingActivityForEventDispatch();
 
     EventTargetInterface eventTargetInterface() const final;
index e860403..2528751 100644 (file)
@@ -154,8 +154,11 @@ void SWClientConnection::updateRegistrationState(ServiceWorkerRegistrationIdenti
 
 void SWClientConnection::updateWorkerState(ServiceWorkerIdentifier identifier, ServiceWorkerState state)
 {
-    for (auto* worker : ServiceWorker::allWorkers().get(identifier))
-        worker->scheduleTaskToUpdateState(state);
+    // FIXME: We should iterate over all service worker clients, not only documents.
+    for (auto* document : Document::allDocuments()) {
+        if (auto* serviceWorker = document->serviceWorker(identifier))
+            serviceWorker->scheduleTaskToUpdateState(state);
+    }
 }
 
 void SWClientConnection::fireUpdateFoundEvent(ServiceWorkerRegistrationIdentifier identifier)