Firing a fetch event should not be blocked on main thread

https://bugs.webkit.org/show_bug.cgi?id=241096

Patch by Youenn Fablet <youennf@gmail.com> on 2022-06-01
Reviewed by Chris Dumez.

The main thread might be blocked by work done by the web page, like executing JavaScript.
This might delay fetch events handling. This can cause PLT regressions when serving content through a service worker.
To limit the perf penalty, we are now hopping to a work queue to process all WebSWContextManagerConnection messages.
For fetch events and message events, we directly go from that thread to the service worker thread.
For install/activate/push/notification events, we keep going through the main thread as they are not as perf crtical.
Also install/activate should follow the same flow as other events like updatefound which are served through WebSWClientConnection.
We change skipWaiting accordingly to remove races in case the reply would go to main thread directly instead of going through the background work queue.

We do some refactoring to allow getting a ServiceWorkerThreadProxy from a background queue.

* Source/WebCore/workers/service/context/SWContextManager.cpp:
(WebCore::SWContextManager::didSaveScriptsToDisk): Deleted.
* Source/WebCore/workers/service/context/SWContextManager.h:
* Source/WebCore/workers/service/server/SWServerToContextConnection.cpp:
(WebCore::SWServerToContextConnection::skipWaiting): Deleted.
* Source/WebCore/workers/service/server/SWServerToContextConnection.h:
* Source/WebCore/workers/service/server/SWServerWorker.h:
* Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:
(WebCore::ServiceWorkerThreadProxy::startFetch):
(WebCore::ServiceWorkerThreadProxy::convertFetchToDownload):
(WebCore::ServiceWorkerThreadProxy::continueDidReceiveFetchResponse):
(WebCore::ServiceWorkerThreadProxy::fireMessageEvent):
(WebCore::ServiceWorkerThreadProxy::didSaveScriptsToDisk):
(WebCore::ServiceWorkerThreadProxy::firePushEvent):
(WebCore::ServiceWorkerThreadProxy::firePushSubscriptionChangeEvent):
(WebCore::ServiceWorkerThreadProxy::fireNotificationEvent):
(WebCore::ServiceWorkerThreadProxy::willPostTaskToFireMessageEvent): Deleted.
* Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:
* Source/WebKit/Shared/WebPreferencesStore.h
* Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:
(WebKit::WebSWServerToContextConnection::skipWaiting):
* Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h:
* Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.messages.in:
* Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:
(WebKit::WebSWContextManagerConnection::~WebSWContextManagerConnection):
(WebKit::WebSWContextManagerConnection::cancelFetch):
(WebKit::WebSWContextManagerConnection::continueDidReceiveFetchResponse):
(WebKit::WebSWContextManagerConnection::postMessageToServiceWorker):
(WebKit::WebSWContextManagerConnection::didSaveScriptsToDisk):
(WebKit::WebSWContextManagerConnection::convertFetchToDownload):
(WebKit::WebSWContextManagerConnection::skipWaiting):
(WebKit::WebSWContextManagerConnection::skipWaitingCompleted):
* Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:
* Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in:

Canonical link: https://commits.webkit.org/251183@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295088 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/testing/ServiceWorkerInternals.cpp

@@ -51,7 +51,7 @@ ServiceWorkerInternals::~ServiceWorkerInternals() = default;
 void ServiceWorkerInternals::setOnline(bool isOnline)
 {
     callOnMainThread([identifier = m_identifier, isOnline] () {
-        if (auto* proxy = SWContextManager::singleton().workerByID(identifier))
+        if (auto* proxy = SWContextManager::singleton().serviceWorkerThreadProxy(identifier))
             proxy->notifyNetworkStateChange(isOnline);
     });
 }
@@ -75,7 +75,7 @@ void ServiceWorkerInternals::schedulePushEvent(const String& message, RefPtr<Def
     }
     callOnMainThread([identifier = m_identifier, data = WTFMove(data), weakThis = WeakPtr { *this }, counter]() mutable {
         SWContextManager::singleton().firePushEvent(identifier, WTFMove(data), [identifier, weakThis = WTFMove(weakThis), counter](bool result) mutable {
-            if (auto* proxy = SWContextManager::singleton().workerByID(identifier)) {
+            if (auto* proxy = SWContextManager::singleton().serviceWorkerThreadProxy(identifier)) {
                 proxy->thread().runLoop().postTaskForMode([weakThis = WTFMove(weakThis), counter, result](auto&) {
                     if (!weakThis)
                         return;
@@ -169,7 +169,7 @@ void ServiceWorkerInternals::lastNavigationWasAppInitiated(Ref<DeferredPromise>&
     ASSERT(!m_lastNavigationWasAppInitiatedPromise);
     m_lastNavigationWasAppInitiatedPromise = WTFMove(promise);
     callOnMainThread([identifier = m_identifier, weakThis = WeakPtr { *this }]() mutable {
-        if (auto* proxy = SWContextManager::singleton().workerByID(identifier)) {
+        if (auto* proxy = SWContextManager::singleton().serviceWorkerThreadProxy(identifier)) {
             proxy->thread().runLoop().postTaskForMode([weakThis = WTFMove(weakThis), appInitiated = proxy->lastNavigationWasAppInitiated()](auto&) {
                 if (!weakThis || !weakThis->m_lastNavigationWasAppInitiatedPromise)
                     return;

Source/WebCore/workers/service/ServiceWorkerClientData.h

@@ -57,8 +57,8 @@ struct ServiceWorkerClientData {
     uint64_t focusOrder { 0 };
     Vector<String> ancestorOrigins;
 
-    ServiceWorkerClientData isolatedCopy() const &;
-    ServiceWorkerClientData isolatedCopy() &&;
+    WEBCORE_EXPORT ServiceWorkerClientData isolatedCopy() const &;
+    WEBCORE_EXPORT ServiceWorkerClientData isolatedCopy() &&;
 
     WEBCORE_EXPORT static ServiceWorkerClientData from(ScriptExecutionContext&);
 

Source/WebCore/workers/service/ServiceWorkerContextData.h

@@ -102,8 +102,8 @@ struct ServiceWorkerContextData {
     template<class Encoder> void encode(Encoder&) const;
     template<class Decoder> static std::optional<ServiceWorkerContextData> decode(Decoder&);
 
-    ServiceWorkerContextData isolatedCopy() const &;
-    ServiceWorkerContextData isolatedCopy() &&;
+    WEBCORE_EXPORT ServiceWorkerContextData isolatedCopy() const &;
+    WEBCORE_EXPORT ServiceWorkerContextData isolatedCopy() &&;
 };
 
 template<class Encoder>

Source/WebCore/workers/service/ServiceWorkerData.h

@@ -41,8 +41,8 @@ struct ServiceWorkerData {
     WorkerType type;
     ServiceWorkerRegistrationIdentifier registrationIdentifier;
 
-    ServiceWorkerData isolatedCopy() const &;
-    ServiceWorkerData isolatedCopy() &&;
+    WEBCORE_EXPORT ServiceWorkerData isolatedCopy() const &;
+    WEBCORE_EXPORT ServiceWorkerData isolatedCopy() &&;
 
     template<class Encoder> void encode(Encoder&) const;
     template<class Decoder> static std::optional<ServiceWorkerData> decode(Decoder&);

Source/WebCore/workers/service/context/SWContextManager.cpp

@@ -40,9 +40,11 @@ SWContextManager& SWContextManager::singleton()
     return *sharedManager;
 }
 
-void SWContextManager::setConnection(std::unique_ptr<Connection>&& connection)
+void SWContextManager::setConnection(Ref<Connection>&& connection)
 {
     ASSERT(!m_connection || m_connection->isClosed());
+    if (m_connection)
+        m_connection->stop();
     m_connection = WTFMove(connection);
 }
 
@@ -53,12 +55,18 @@ auto SWContextManager::connection() const -> Connection*
 
 void SWContextManager::registerServiceWorkerThreadForInstall(Ref<ServiceWorkerThreadProxy>&& serviceWorkerThreadProxy)
 {
+    ASSERT(isMainThread());
+
     auto serviceWorkerIdentifier = serviceWorkerThreadProxy->identifier();
     auto jobDataIdentifier = serviceWorkerThreadProxy->thread().jobDataIdentifier();
     auto* threadProxy = serviceWorkerThreadProxy.ptr();
-    auto result = m_workerMap.add(serviceWorkerIdentifier, WTFMove(serviceWorkerThreadProxy));
-    ASSERT_UNUSED(result, result.isNewEntry);
-
+
+    {
+        Locker locker { m_workerMapLock };
+        auto result = m_workerMap.add(serviceWorkerIdentifier, WTFMove(serviceWorkerThreadProxy));
+        ASSERT_UNUSED(result, result.isNewEntry);
+    }
+
     threadProxy->thread().start([jobDataIdentifier, serviceWorkerIdentifier](const String& exceptionMessage, bool doesHandleFetch) {
         SWContextManager::singleton().startedServiceWorker(jobDataIdentifier, serviceWorkerIdentifier, exceptionMessage, doesHandleFetch);
     });
@@ -77,22 +85,21 @@ void SWContextManager::startedServiceWorker(std::optional<ServiceWorkerJobDataId
 
 ServiceWorkerThreadProxy* SWContextManager::serviceWorkerThreadProxy(ServiceWorkerIdentifier identifier) const
 {
+    ASSERT(isMainThread());
+    Locker locker { m_workerMapLock };
     return m_workerMap.get(identifier);
 }
 
-void SWContextManager::postMessageToServiceWorker(ServiceWorkerIdentifier destination, MessageWithMessagePorts&& message, ServiceWorkerOrClientData&& sourceData)
+RefPtr<ServiceWorkerThreadProxy> SWContextManager::serviceWorkerThreadProxyFromBackgroundThread(ServiceWorkerIdentifier identifier) const
 {
-    auto* serviceWorker = m_workerMap.get(destination);
-    if (!serviceWorker)
-        return;
-
-    // FIXME: We should pass valid MessagePortChannels.
-    serviceWorker->postMessageToServiceWorker(WTFMove(message), WTFMove(sourceData));
+    Locker locker { m_workerMapLock };
+    RefPtr result = m_workerMap.get(identifier);
+    return result;
 }
 
 void SWContextManager::fireInstallEvent(ServiceWorkerIdentifier identifier)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker)
         return;
 
@@ -101,7 +108,7 @@ void SWContextManager::fireInstallEvent(ServiceWorkerIdentifier identifier)
 
 void SWContextManager::fireActivateEvent(ServiceWorkerIdentifier identifier)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker)
         return;
 
@@ -110,7 +117,7 @@ void SWContextManager::fireActivateEvent(ServiceWorkerIdentifier identifier)
 
 void SWContextManager::firePushEvent(ServiceWorkerIdentifier identifier, std::optional<Vector<uint8_t>>&& data, CompletionHandler<void(bool)>&& callback)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker) {
         callback(false);
         return;
@@ -121,7 +128,7 @@ void SWContextManager::firePushEvent(ServiceWorkerIdentifier identifier, std::op
 
 void SWContextManager::firePushSubscriptionChangeEvent(ServiceWorkerIdentifier identifier, std::optional<PushSubscriptionData>&& newSubscriptionData, std::optional<PushSubscriptionData>&& oldSubscriptionData)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker)
         return;
 
@@ -130,7 +137,7 @@ void SWContextManager::firePushSubscriptionChangeEvent(ServiceWorkerIdentifier i
 
 void SWContextManager::fireNotificationEvent(ServiceWorkerIdentifier identifier, NotificationData&& data, NotificationEventType eventType, CompletionHandler<void(bool)>&& callback)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker)
         return;
 
@@ -139,7 +146,14 @@ void SWContextManager::fireNotificationEvent(ServiceWorkerIdentifier identifier,
 
 void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Seconds timeout, Function<void()>&& completionHandler)
 {
-    auto serviceWorker = m_workerMap.take(identifier);
+    RELEASE_LOG(ServiceWorker, "SWContextManager::terminateWorker");
+
+    RefPtr<ServiceWorkerThreadProxy> serviceWorker;
+    {
+        Locker locker { m_workerMapLock };
+        serviceWorker = m_workerMap.take(identifier);
+    }
+
     if (!serviceWorker) {
         if (completionHandler)
             completionHandler();
@@ -148,12 +162,6 @@ void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Secon
     stopWorker(*serviceWorker, timeout, WTFMove(completionHandler));
 }
 
-void SWContextManager::didSaveScriptsToDisk(ServiceWorkerIdentifier identifier, ScriptBuffer&& script, HashMap<URL, ScriptBuffer>&& importedScripts)
-{
-    if (auto serviceWorker = m_workerMap.get(identifier))
-        serviceWorker->didSaveScriptsToDisk(WTFMove(script), WTFMove(importedScripts));
-}
-
 void SWContextManager::stopWorker(ServiceWorkerThreadProxy& serviceWorker, Seconds timeout, Function<void()>&& completionHandler)
 {
     auto identifier = serviceWorker.identifier();
@@ -179,13 +187,14 @@ void SWContextManager::stopWorker(ServiceWorkerThreadProxy& serviceWorker, Secon
 
 void SWContextManager::forEachServiceWorker(const Function<Function<void(ScriptExecutionContext&)>()>& createTask)
 {
+    Locker locker { m_workerMapLock };
     for (auto& worker : m_workerMap.values())
         worker->thread().runLoop().postTask(createTask());
 }
 
 bool SWContextManager::postTaskToServiceWorker(ServiceWorkerIdentifier identifier, Function<void(ServiceWorkerGlobalScope&)>&& task)
 {
-    auto* serviceWorker = m_workerMap.get(identifier);
+    auto* serviceWorker = serviceWorkerThreadProxy(identifier);
     if (!serviceWorker)
         return false;
 
@@ -211,8 +220,12 @@ SWContextManager::ServiceWorkerTerminationRequest::ServiceWorkerTerminationReque
 
 void SWContextManager::stopAllServiceWorkers()
 {
-    auto serviceWorkers = WTFMove(m_workerMap);
-    for (auto& serviceWorker : serviceWorkers.values())
+    HashMap<ServiceWorkerIdentifier, Ref<ServiceWorkerThreadProxy>> workerMap;
+    {
+        Locker locker { m_workerMapLock };
+        workerMap = WTFMove(m_workerMap);
+    }
+    for (auto& serviceWorker : workerMap.values())
         stopWorker(serviceWorker, workerTerminationTimeout, [] { });
 }
 

Source/WebCore/workers/service/context/SWContextManager.h

@@ -36,6 +36,7 @@
 #include "ServiceWorkerThreadProxy.h"
 #include <wtf/CompletionHandler.h>
 #include <wtf/HashMap.h>
+#include <wtf/Lock.h>
 #include <wtf/URLHash.h>
 
 namespace WebCore {
@@ -48,7 +49,6 @@ class SWContextManager {
     WEBCORE_EXPORT static SWContextManager& singleton();
 
     class Connection {
-        WTF_MAKE_FAST_ALLOCATED;
     public:
         virtual ~Connection() { }
 
@@ -81,6 +81,10 @@ class SWContextManager {
         virtual bool isThrottleable() const = 0;
         virtual PageIdentifier pageIdentifier() const = 0;
 
+        virtual void ref() const = 0;
+        virtual void deref() const = 0;
+        virtual void stop() = 0;
+
         bool isClosed() const { return m_isClosed; }
 
     protected:
@@ -90,20 +94,19 @@ class SWContextManager {
         bool m_isClosed { false };
     };
 
-    WEBCORE_EXPORT void setConnection(std::unique_ptr<Connection>&&);
+    WEBCORE_EXPORT void setConnection(Ref<Connection>&&);
     WEBCORE_EXPORT Connection* connection() const;
 
     WEBCORE_EXPORT void registerServiceWorkerThreadForInstall(Ref<ServiceWorkerThreadProxy>&&);
     WEBCORE_EXPORT ServiceWorkerThreadProxy* serviceWorkerThreadProxy(ServiceWorkerIdentifier) const;
-    WEBCORE_EXPORT void postMessageToServiceWorker(ServiceWorkerIdentifier destination, MessageWithMessagePorts&&, ServiceWorkerOrClientData&& sourceData);
+    WEBCORE_EXPORT RefPtr<ServiceWorkerThreadProxy> serviceWorkerThreadProxyFromBackgroundThread(ServiceWorkerIdentifier) const;
     WEBCORE_EXPORT void fireInstallEvent(ServiceWorkerIdentifier);
     WEBCORE_EXPORT void fireActivateEvent(ServiceWorkerIdentifier);
     WEBCORE_EXPORT void firePushEvent(ServiceWorkerIdentifier, std::optional<Vector<uint8_t>>&&, CompletionHandler<void(bool)>&&);
     WEBCORE_EXPORT void firePushSubscriptionChangeEvent(ServiceWorkerIdentifier, std::optional<PushSubscriptionData>&& newSubscriptionData, std::optional<PushSubscriptionData>&& oldSubscriptionData);
     WEBCORE_EXPORT void fireNotificationEvent(ServiceWorkerIdentifier, NotificationData&&, NotificationEventType, CompletionHandler<void(bool)>&&);
 
     WEBCORE_EXPORT void terminateWorker(ServiceWorkerIdentifier, Seconds timeout, Function<void()>&&);
-    WEBCORE_EXPORT void didSaveScriptsToDisk(ServiceWorkerIdentifier, ScriptBuffer&&, HashMap<URL, ScriptBuffer>&& importedScripts);
 
     void forEachServiceWorker(const Function<Function<void(ScriptExecutionContext&)>()>&);
 
@@ -112,8 +115,6 @@ class SWContextManager {
     using ServiceWorkerCreationCallback = void(uint64_t);
     void setServiceWorkerCreationCallback(ServiceWorkerCreationCallback* callback) { m_serviceWorkerCreationCallback = callback; }
 
-    ServiceWorkerThreadProxy* workerByID(ServiceWorkerIdentifier identifier) { return m_workerMap.get(identifier); }
-
     WEBCORE_EXPORT void stopAllServiceWorkers();
 
     static constexpr Seconds workerTerminationTimeout { 10_s };
@@ -129,8 +130,9 @@ class SWContextManager {
 
     void stopWorker(ServiceWorkerThreadProxy&, Seconds, Function<void()>&&);
 
-    HashMap<ServiceWorkerIdentifier, Ref<ServiceWorkerThreadProxy>> m_workerMap;
-    std::unique_ptr<Connection> m_connection;
+    HashMap<ServiceWorkerIdentifier, Ref<ServiceWorkerThreadProxy>> m_workerMap WTF_GUARDED_BY_LOCK(m_workerMapLock);
+    mutable Lock m_workerMapLock;
+    RefPtr<Connection> m_connection;
     ServiceWorkerCreationCallback* m_serviceWorkerCreationCallback { nullptr };
 
     class ServiceWorkerTerminationRequest {