Remove synchronous termination of service workers
https://bugs.webkit.org/show_bug.cgi?id=209666
Reviewed by Chris Dumez.
Source/WebCore:
Instead of supporting synchronous IPC to terminate a service worker, SWServerWorker will asynchronously ask for the service worker to terminate.
If it is not terminated after some time, SWServerWorker will then ask to terminate the process running the service worker.
Time is kept to 10 seconds.
We can then remove all synchronous related code related to termination.
We migrate the terminateServiceWorker internal API to be Promise based.
Covered by existing tests.
* testing/Internals.cpp:
(WebCore::Internals::terminateServiceWorker):
* testing/Internals.h:
* testing/Internals.idl:
* workers/service/ServiceWorkerProvider.h:
* workers/service/SWClientConnection.h:
* workers/service/WorkerSWClientConnection.cpp:
* workers/service/WorkerSWClientConnection.h:
* workers/service/server/SWServer.cpp:
(WebCore::SWServer::~SWServer):
(WebCore::SWServer::unregisterServiceWorkerClient):
* workers/service/server/SWServer.h:
* workers/service/server/SWServerToContextConnection.h:
* workers/service/server/SWServerWorker.cpp:
(WebCore::m_terminationTimer):
(WebCore::SWServerWorker::~SWServerWorker):
(WebCore::SWServerWorker::terminate):
(WebCore::SWServerWorker::startTermination):
(WebCore::SWServerWorker::terminateCompleted):
(WebCore::SWServerWorker::callTerminationCallbacks):
(WebCore::SWServerWorker::terminationTimerFired):
(WebCore::SWServerWorker::setState):
(WebCore::SWServerWorker::didFailHeartBeatCheck):
* workers/service/server/SWServerWorker.h:
(WebCore::SWServerWorker::terminate):
Source/WebKit:
Update IPC code according removal of synchronous termination of service worker.
Implement async-with-reply termination instead.
* NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:
(WebKit::WebSWServerConnection::terminateWorkerFromClient):
(WebKit::WebSWServerConnection::fetchTaskTimedOut):
* NetworkProcess/ServiceWorker/WebSWServerConnection.h:
* NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in:
* NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:
(WebKit::WebSWServerToContextConnection::terminateDueToUnresponsiveness):
* NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h:
* WebProcess/Storage/WebServiceWorkerProvider.cpp:
* WebProcess/Storage/WebServiceWorkerProvider.h:
* WebProcess/Storage/WebSWClientConnection.cpp:
(WebKit::WebSWClientConnection::terminateWorkerForTesting):
* WebProcess/Storage/WebSWClientConnection.h:
* WebProcess/Storage/WebSWContextManagerConnection.cpp:
* WebProcess/Storage/WebSWContextManagerConnection.h:
* WebProcess/Storage/WebSWContextManagerConnection.messages.in:
LayoutTests:
* http/tests/workers/service/resources/postmessage-after-sw-process-crash.js:
(async event):
* http/tests/workers/service/resources/postmessage-after-terminate.js:
(async event):
* http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js:
(async event):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259383 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index cc41693..38007bc 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2020-04-02 youenn fablet <youenn@apple.com>
+
+ Remove synchronous termination of service workers
+ https://bugs.webkit.org/show_bug.cgi?id=209666
+
+ Reviewed by Chris Dumez.
+
+ * http/tests/workers/service/resources/postmessage-after-sw-process-crash.js:
+ (async event):
+ * http/tests/workers/service/resources/postmessage-after-terminate.js:
+ (async event):
+ * http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js:
+ (async event):
+
2020-04-01 Ryan Haddad <ryanhaddad@apple.com>
[iOS] svg/as-background-image/tiled-background-image.html is a flaky image failure
diff --git a/LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js b/LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js
index aad2de6..10d8e70 100644
--- a/LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js
+++ b/LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js
@@ -2,7 +2,7 @@
let worker = null;
let remainingAttempts = 1000; // We try for 10 seconds before timing out.
-navigator.serviceWorker.addEventListener("message", function(event) {
+navigator.serviceWorker.addEventListener("message", async function(event) {
if (!serviceWorkerHasReceivedState) {
if (!event.data) {
log("FAIL: service worker did not receive the state");
@@ -12,7 +12,7 @@
serviceWorkerHasReceivedState = true;
log("* Simulating Service Worker process crash");
- testRunner.terminateServiceWorkers();
+ await testRunner.terminateServiceWorkers();
handle = setInterval(function() {
remainingAttempts--;
diff --git a/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js b/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js
index ac7cad7..0920433 100644
--- a/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js
+++ b/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js
@@ -1,9 +1,9 @@
var messageNumber = 1;
-navigator.serviceWorker.addEventListener("message", function(event) {
+navigator.serviceWorker.addEventListener("message", async function(event) {
log("PASS: Client received message from service worker, origin: " + event.origin);
log(event.data);
if (messageNumber == 1) {
- window.internals.terminateServiceWorker(event.source);
+ await window.internals.terminateServiceWorker(event.source);
event.source.postMessage("Message 2");
messageNumber++;
} else
diff --git a/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js b/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js
index 86a431b..fd4ad46 100644
--- a/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js
+++ b/LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js
@@ -1,12 +1,12 @@
let state = "WaitingForHang";
-navigator.serviceWorker.addEventListener("message", function(event) {
+navigator.serviceWorker.addEventListener("message", async function(event) {
log(event.data);
if (state === "WaitingForHang") {
log("PASS: ServiceWorker received message: " + event.data);
log("Service Worker should now be hung");
log("Terminating service worker...")
- internals.terminateServiceWorker(worker);
+ await internals.terminateServiceWorker(worker);
log("Terminated service worker.");
state = "WaitingForMessageAfterTerminatingHungServiceWorker"
handle = setInterval(function() {
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 8446a9b..71a9094 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,45 @@
+2020-04-02 youenn fablet <youenn@apple.com>
+
+ Remove synchronous termination of service workers
+ https://bugs.webkit.org/show_bug.cgi?id=209666
+
+ Reviewed by Chris Dumez.
+
+ Instead of supporting synchronous IPC to terminate a service worker, SWServerWorker will asynchronously ask for the service worker to terminate.
+ If it is not terminated after some time, SWServerWorker will then ask to terminate the process running the service worker.
+ Time is kept to 10 seconds.
+
+ We can then remove all synchronous related code related to termination.
+ We migrate the terminateServiceWorker internal API to be Promise based.
+
+ Covered by existing tests.
+
+ * testing/Internals.cpp:
+ (WebCore::Internals::terminateServiceWorker):
+ * testing/Internals.h:
+ * testing/Internals.idl:
+ * workers/service/ServiceWorkerProvider.h:
+ * workers/service/SWClientConnection.h:
+ * workers/service/WorkerSWClientConnection.cpp:
+ * workers/service/WorkerSWClientConnection.h:
+ * workers/service/server/SWServer.cpp:
+ (WebCore::SWServer::~SWServer):
+ (WebCore::SWServer::unregisterServiceWorkerClient):
+ * workers/service/server/SWServer.h:
+ * workers/service/server/SWServerToContextConnection.h:
+ * workers/service/server/SWServerWorker.cpp:
+ (WebCore::m_terminationTimer):
+ (WebCore::SWServerWorker::~SWServerWorker):
+ (WebCore::SWServerWorker::terminate):
+ (WebCore::SWServerWorker::startTermination):
+ (WebCore::SWServerWorker::terminateCompleted):
+ (WebCore::SWServerWorker::callTerminationCallbacks):
+ (WebCore::SWServerWorker::terminationTimerFired):
+ (WebCore::SWServerWorker::setState):
+ (WebCore::SWServerWorker::didFailHeartBeatCheck):
+ * workers/service/server/SWServerWorker.h:
+ (WebCore::SWServerWorker::terminate):
+
2020-04-02 Rob Buis <rbuis@igalia.com>
Remove FrameLoader::addExtraFieldsToMainResourceRequest
diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
index 5fbc8e4..b2af0bc 100644
--- a/Source/WebCore/testing/Internals.cpp
+++ b/Source/WebCore/testing/Internals.cpp
@@ -5152,12 +5152,11 @@
});
}
-void Internals::terminateServiceWorker(ServiceWorker& worker)
+void Internals::terminateServiceWorker(ServiceWorker& worker, DOMPromiseDeferred<void>&& promise)
{
- if (!contextDocument())
- return;
-
- ServiceWorkerProvider::singleton().serviceWorkerConnection().syncTerminateWorker(worker.identifier());
+ ServiceWorkerProvider::singleton().terminateWorkerForTesting(worker.identifier(), [promise = WTFMove(promise)]() mutable {
+ promise.resolve();
+ });
}
void Internals::isServiceWorkerRunning(ServiceWorker& worker, DOMPromiseDeferred<IDLBoolean>&& promise)
diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
index fe4b579..79f424c 100644
--- a/Source/WebCore/testing/Internals.h
+++ b/Source/WebCore/testing/Internals.h
@@ -784,7 +784,7 @@
#if ENABLE(SERVICE_WORKER)
using HasRegistrationPromise = DOMPromiseDeferred<IDLBoolean>;
void hasServiceWorkerRegistration(const String& clientURL, HasRegistrationPromise&&);
- void terminateServiceWorker(ServiceWorker&);
+ void terminateServiceWorker(ServiceWorker&, DOMPromiseDeferred<void>&&);
void isServiceWorkerRunning(ServiceWorker&, DOMPromiseDeferred<IDLBoolean>&&);
#endif
diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
index 4f9cda4e..e5c58f6 100644
--- a/Source/WebCore/testing/Internals.idl
+++ b/Source/WebCore/testing/Internals.idl
@@ -770,7 +770,7 @@
boolean audioSessionActive();
[Conditional=SERVICE_WORKER] Promise<boolean> hasServiceWorkerRegistration(DOMString scopeURL);
- [Conditional=SERVICE_WORKER] void terminateServiceWorker(ServiceWorker worker);
+ [Conditional=SERVICE_WORKER] Promise<void> terminateServiceWorker(ServiceWorker worker);
[Conditional=SERVICE_WORKER] Promise<boolean> isServiceWorkerRunning(ServiceWorker worker);
[CallWith=Document, Conditional=APPLE_PAY] readonly attribute MockPaymentCoordinator mockPaymentCoordinator;
diff --git a/Source/WebCore/workers/service/SWClientConnection.h b/Source/WebCore/workers/service/SWClientConnection.h
index b2bc44d..9b394c1 100644
--- a/Source/WebCore/workers/service/SWClientConnection.h
+++ b/Source/WebCore/workers/service/SWClientConnection.h
@@ -79,7 +79,6 @@
virtual SWServerConnectionIdentifier serverConnectionIdentifier() const = 0;
virtual bool mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData&) const = 0;
- virtual void syncTerminateWorker(ServiceWorkerIdentifier) = 0;
virtual void registerServiceWorkerClient(const SecurityOrigin& topOrigin, const ServiceWorkerClientData&, const Optional<ServiceWorkerRegistrationIdentifier>&, const String& userAgent) = 0;
virtual void unregisterServiceWorkerClient(DocumentIdentifier) = 0;
diff --git a/Source/WebCore/workers/service/ServiceWorkerProvider.h b/Source/WebCore/workers/service/ServiceWorkerProvider.h
index e2967be..0adfe94 100644
--- a/Source/WebCore/workers/service/ServiceWorkerProvider.h
+++ b/Source/WebCore/workers/service/ServiceWorkerProvider.h
@@ -41,6 +41,7 @@
static void setSharedProvider(ServiceWorkerProvider&);
virtual SWClientConnection& serviceWorkerConnection() = 0;
+ virtual void terminateWorkerForTesting(ServiceWorkerIdentifier, CompletionHandler<void()>&&) = 0;
void setMayHaveRegisteredServiceWorkers() { m_mayHaveRegisteredServiceWorkers = true; }
diff --git a/Source/WebCore/workers/service/WorkerSWClientConnection.cpp b/Source/WebCore/workers/service/WorkerSWClientConnection.cpp
index 7582575..7c6309a 100644
--- a/Source/WebCore/workers/service/WorkerSWClientConnection.cpp
+++ b/Source/WebCore/workers/service/WorkerSWClientConnection.cpp
@@ -155,14 +155,6 @@
return true;
}
-void WorkerSWClientConnection::syncTerminateWorker(ServiceWorkerIdentifier identifier)
-{
- callOnMainThread([identifier]() mutable {
- auto& connection = ServiceWorkerProvider::singleton().serviceWorkerConnection();
- connection.syncTerminateWorker(identifier);
- });
-}
-
void WorkerSWClientConnection::registerServiceWorkerClient(const SecurityOrigin& topOrigin, const ServiceWorkerClientData& data, const Optional<ServiceWorkerRegistrationIdentifier>& identifier, const String& userAgent)
{
callOnMainThread([topOrigin = topOrigin.isolatedCopy(), data = crossThreadCopy(data), identifier, userAgent = crossThreadCopy(userAgent)] {
diff --git a/Source/WebCore/workers/service/WorkerSWClientConnection.h b/Source/WebCore/workers/service/WorkerSWClientConnection.h
index 518ea2f..b417913 100644
--- a/Source/WebCore/workers/service/WorkerSWClientConnection.h
+++ b/Source/WebCore/workers/service/WorkerSWClientConnection.h
@@ -51,7 +51,6 @@
void postMessageToServiceWorker(ServiceWorkerIdentifier destination, MessageWithMessagePorts&&, const ServiceWorkerOrClientIdentifier& source) final;
SWServerConnectionIdentifier serverConnectionIdentifier() const final;
bool mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData&) const final;
- void syncTerminateWorker(ServiceWorkerIdentifier) final;
void registerServiceWorkerClient(const SecurityOrigin& topOrigin, const ServiceWorkerClientData&, const Optional<ServiceWorkerRegistrationIdentifier>&, const String& userAgent) final;
void unregisterServiceWorkerClient(DocumentIdentifier) final;
void finishFetchingScriptInServer(const ServiceWorkerFetchResult&) final;
diff --git a/Source/WebCore/workers/service/server/SWServer.cpp b/Source/WebCore/workers/service/server/SWServer.cpp
index a8151cc..e6df51a 100644
--- a/Source/WebCore/workers/service/server/SWServer.cpp
+++ b/Source/WebCore/workers/service/server/SWServer.cpp
@@ -49,8 +49,6 @@
namespace WebCore {
-static Seconds terminationDelay { 10_s };
-
SWServer::Connection::Connection(SWServer& server, Identifier identifier)
: m_server(server)
, m_identifier(identifier)
@@ -79,7 +77,7 @@
runningWorkers.append(worker.ptr());
}
for (auto& runningWorker : runningWorkers)
- terminateWorker(*runningWorker);
+ runningWorker->terminate();
allServers().remove(this);
}
@@ -307,12 +305,6 @@
m_server.removeClientServiceWorkerRegistration(*this, identifier);
}
-void SWServer::Connection::syncTerminateWorker(ServiceWorkerIdentifier identifier)
-{
- if (auto* worker = m_server.workerByID(identifier))
- m_server.syncTerminateWorker(*worker);
-}
-
SWServer::SWServer(UniqueRef<SWOriginStore>&& originStore, bool processTerminationDelayEnabled, String&& registrationDatabaseDirectory, PAL::SessionID sessionID, SoftUpdateCallback&& softUpdateCallback, CreateContextConnectionCallback&& callback)
: m_originStore(WTFMove(originStore))
, m_sessionID(sessionID)
@@ -733,43 +725,6 @@
return true;
}
-void SWServer::terminateWorker(SWServerWorker& worker)
-{
- terminateWorkerInternal(worker, Asynchronous);
-}
-
-void SWServer::syncTerminateWorker(SWServerWorker& worker)
-{
- terminateWorkerInternal(worker, Synchronous);
-}
-
-void SWServer::terminateWorkerInternal(SWServerWorker& worker, TerminationMode mode)
-{
- ASSERT(m_runningOrTerminatingWorkers.get(worker.identifier()) == &worker);
- ASSERT(worker.isRunning());
-
- RELEASE_LOG(ServiceWorker, "%p - SWServer::terminateWorkerInternal: Terminating service worker %llu", this, worker.identifier().toUInt64());
-
- worker.setState(SWServerWorker::State::Terminating);
-
- auto* contextConnection = worker.contextConnection();
- ASSERT(contextConnection);
- if (!contextConnection) {
- LOG_ERROR("Request to terminate a worker whose context connection does not exist");
- workerContextTerminated(worker);
- return;
- }
-
- switch (mode) {
- case Asynchronous:
- contextConnection->terminateWorker(worker.identifier());
- break;
- case Synchronous:
- contextConnection->syncTerminateWorker(worker.identifier());
- break;
- };
-}
-
void SWServer::markAllWorkersForRegistrableDomainAsTerminated(const RegistrableDomain& registrableDomain)
{
Vector<SWServerWorker*> terminatedWorkers;
@@ -917,7 +872,7 @@
workersToTerminate.append(worker.ptr());
}
for (auto* worker : workersToTerminate)
- terminateWorker(*worker);
+ worker->terminate();
if (!m_clientsByRegistrableDomain.contains(clientRegistrableDomain)) {
if (auto* connection = contextConnectionForRegistrableDomain(clientRegistrableDomain)) {
@@ -928,7 +883,7 @@
m_clientIdentifiersPerOrigin.remove(clientOrigin);
});
- iterator->value.terminateServiceWorkersTimer->startOneShot(m_isProcessTerminationDelayEnabled && !MemoryPressureHandler::singleton().isUnderMemoryPressure() ? terminationDelay : 0_s);
+ iterator->value.terminateServiceWorkersTimer->startOneShot(m_isProcessTerminationDelayEnabled && !MemoryPressureHandler::singleton().isUnderMemoryPressure() ? defaultTerminationDelay : 0_s);
}
auto clientsByRegistrableDomainIterator = m_clientsByRegistrableDomain.find(clientRegistrableDomain);
diff --git a/Source/WebCore/workers/service/server/SWServer.h b/Source/WebCore/workers/service/server/SWServer.h
index aef1983..2879283 100644
--- a/Source/WebCore/workers/service/server/SWServer.h
+++ b/Source/WebCore/workers/service/server/SWServer.h
@@ -102,7 +102,6 @@
WEBCORE_EXPORT void finishFetchingScriptInServer(const ServiceWorkerFetchResult&);
WEBCORE_EXPORT void addServiceWorkerRegistrationInServer(ServiceWorkerRegistrationIdentifier);
WEBCORE_EXPORT void removeServiceWorkerRegistrationInServer(ServiceWorkerRegistrationIdentifier);
- WEBCORE_EXPORT void syncTerminateWorker(ServiceWorkerIdentifier);
WEBCORE_EXPORT void whenRegistrationReady(uint64_t registrationReadyRequestIdentifier, const SecurityOriginData& topOrigin, const URL& clientURL);
WEBCORE_EXPORT void storeRegistrationsOnDisk(CompletionHandler<void()>&&);
@@ -152,8 +151,6 @@
void startScriptFetch(const ServiceWorkerJobData&, bool shouldRefreshCache);
void updateWorker(const ServiceWorkerJobDataIdentifier&, SWServerRegistration&, const URL&, const String& script, const ContentSecurityPolicyResponseHeaders&, const String& referrerPolicy, WorkerType, HashMap<URL, ServiceWorkerContextData::ImportedScript>&&);
- void terminateWorker(SWServerWorker&);
- WEBCORE_EXPORT void syncTerminateWorker(SWServerWorker&);
void fireInstallEvent(SWServerWorker&);
void fireActivateEvent(SWServerWorker&);
@@ -215,6 +212,8 @@
WEBCORE_EXPORT void handleLowMemoryWarning();
+ static constexpr Seconds defaultTerminationDelay = 10_s;
+
private:
void scriptFetchFinished(const ServiceWorkerFetchResult&);
@@ -236,12 +235,6 @@
void performGetOriginsWithRegistrationsCallbacks();
- enum TerminationMode {
- Synchronous,
- Asynchronous,
- };
- void terminateWorkerInternal(SWServerWorker&, TerminationMode);
-
void contextConnectionCreated(SWServerToContextConnection&);
HashMap<SWServerConnectionIdentifier, std::unique_ptr<Connection>> m_connections;
diff --git a/Source/WebCore/workers/service/server/SWServerToContextConnection.h b/Source/WebCore/workers/service/server/SWServerToContextConnection.h
index 05884c8..4659f9f 100644
--- a/Source/WebCore/workers/service/server/SWServerToContextConnection.h
+++ b/Source/WebCore/workers/service/server/SWServerToContextConnection.h
@@ -54,7 +54,6 @@
virtual void fireInstallEvent(ServiceWorkerIdentifier) = 0;
virtual void fireActivateEvent(ServiceWorkerIdentifier) = 0;
virtual void terminateWorker(ServiceWorkerIdentifier) = 0;
- virtual void syncTerminateWorker(ServiceWorkerIdentifier) = 0;
virtual void findClientByIdentifierCompleted(uint64_t requestIdentifier, const Optional<ServiceWorkerClientData>&, bool hasSecurityError) = 0;
virtual void matchAllCompleted(uint64_t requestIdentifier, const Vector<ServiceWorkerClientData>&) = 0;
@@ -75,6 +74,7 @@
const RegistrableDomain& registrableDomain() const { return m_registrableDomain; }
virtual void connectionIsNoLongerNeeded() = 0;
+ virtual void terminateDueToUnresponsiveness() = 0;
protected:
WEBCORE_EXPORT explicit SWServerToContextConnection(RegistrableDomain&&);
diff --git a/Source/WebCore/workers/service/server/SWServerWorker.cpp b/Source/WebCore/workers/service/server/SWServerWorker.cpp
index 400aa47f..46b74e8 100644
--- a/Source/WebCore/workers/service/server/SWServerWorker.cpp
+++ b/Source/WebCore/workers/service/server/SWServerWorker.cpp
@@ -28,6 +28,7 @@
#if ENABLE(SERVICE_WORKER)
+#include "Logging.h"
#include "SWServer.h"
#include "SWServerRegistration.h"
#include "SWServerToContextConnection.h"
@@ -58,6 +59,7 @@
, m_referrerPolicy(WTFMove(referrerPolicy))
, m_registrableDomain(m_data.scriptURL)
, m_scriptResourceMap(WTFMove(scriptResourceMap))
+ , m_terminationTimer(*this, &SWServerWorker::terminationTimerFired)
{
m_data.scriptURL.removeFragmentIdentifier();
@@ -74,6 +76,8 @@
auto taken = allWorkers().take(identifier());
ASSERT_UNUSED(taken, taken == this);
+
+ callTerminationCallbacks();
}
ServiceWorkerContextData SWServerWorker::contextData() const
@@ -83,10 +87,60 @@
return { WTF::nullopt, m_registration->data(), m_data.identifier, m_script, m_contentSecurityPolicy, m_referrerPolicy, m_data.scriptURL, m_data.type, false, m_scriptResourceMap };
}
-void SWServerWorker::terminate()
+void SWServerWorker::terminate(CompletionHandler<void()>&& callback)
{
- if (isRunning())
- m_server->terminateWorker(*this);
+ if (!m_server)
+ return callback();
+
+ switch (m_state) {
+ case State::Running:
+ startTermination(WTFMove(callback));
+ return;
+ case State::Terminating:
+ m_terminationCallbacks.append(WTFMove(callback));
+ return;
+ case State::NotRunning:
+ return callback();
+ }
+}
+
+void SWServerWorker::startTermination(CompletionHandler<void()>&& callback)
+{
+ auto* contextConnection = this->contextConnection();
+ ASSERT(contextConnection);
+ if (!contextConnection) {
+ RELEASE_LOG_ERROR(ServiceWorker, "Request to terminate a worker %llu whose context connection does not exist", identifier().toUInt64());
+ setState(State::NotRunning);
+ callback();
+ m_server->workerContextTerminated(*this);
+ return;
+ }
+
+ setState(State::Terminating);
+
+ m_terminationCallbacks.append(WTFMove(callback));
+ m_terminationTimer.startOneShot(SWServer::defaultTerminationDelay);
+
+ contextConnection->terminateWorker(identifier());
+}
+
+void SWServerWorker::terminationCompleted()
+{
+ m_terminationTimer.stop();
+ callTerminationCallbacks();
+}
+
+void SWServerWorker::callTerminationCallbacks()
+{
+ auto callbacks = WTFMove(m_terminationCallbacks);
+ for (auto& callback : callbacks)
+ callback();
+}
+
+void SWServerWorker::terminationTimerFired()
+{
+ ASSERT(isTerminating());
+ contextConnection()->terminateDueToUnresponsiveness();
}
const ClientOrigin& SWServerWorker::origin() const
@@ -252,6 +306,8 @@
callWhenActivatedHandler(false);
break;
case State::NotRunning:
+ terminationCompleted();
+
callWhenActivatedHandler(false);
// As per https://w3c.github.io/ServiceWorker/#activate, a worker goes to activated even if activating fails.
if (m_data.state == ServiceWorkerState::Activating)
@@ -267,8 +323,7 @@
void SWServerWorker::didFailHeartBeatCheck()
{
- if (m_server && isRunning())
- m_server->terminateWorker(*this);
+ terminate();
}
} // namespace WebCore
diff --git a/Source/WebCore/workers/service/server/SWServerWorker.h b/Source/WebCore/workers/service/server/SWServerWorker.h
index e02ae00..6100cd9 100644
--- a/Source/WebCore/workers/service/server/SWServerWorker.h
+++ b/Source/WebCore/workers/service/server/SWServerWorker.h
@@ -36,6 +36,8 @@
#include "ServiceWorkerIdentifier.h"
#include "ServiceWorkerRegistrationKey.h"
#include "ServiceWorkerTypes.h"
+#include "Timer.h"
+#include <wtf/CompletionHandler.h>
#include <wtf/RefCounted.h>
#include <wtf/WeakPtr.h>
@@ -60,7 +62,7 @@
SWServerWorker(const SWServerWorker&) = delete;
WEBCORE_EXPORT ~SWServerWorker();
- void terminate();
+ WEBCORE_EXPORT void terminate(CompletionHandler<void()>&& = [] { });
WEBCORE_EXPORT void whenActivated(CompletionHandler<void(bool)>&&);
@@ -126,6 +128,11 @@
void callWhenActivatedHandler(bool success);
+ void startTermination(CompletionHandler<void()>&&);
+ void terminationCompleted();
+ void terminationTimerFired();
+ void callTerminationCallbacks();
+
WeakPtr<SWServer> m_server;
ServiceWorkerRegistrationKey m_registrationKey;
WeakPtr<SWServerRegistration> m_registration;
@@ -142,6 +149,8 @@
HashMap<URL, ServiceWorkerContextData::ImportedScript> m_scriptResourceMap;
bool m_shouldSkipHandleFetch;
bool m_hasTimedOutAnyFetchTasks { false };
+ Vector<CompletionHandler<void()>> m_terminationCallbacks;
+ Timer m_terminationTimer;
};
} // namespace WebCore
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index f4cd5ba..a727d4f 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,30 @@
+2020-04-02 youenn fablet <youenn@apple.com>
+
+ Remove synchronous termination of service workers
+ https://bugs.webkit.org/show_bug.cgi?id=209666
+
+ Reviewed by Chris Dumez.
+
+ Update IPC code according removal of synchronous termination of service worker.
+ Implement async-with-reply termination instead.
+
+ * NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:
+ (WebKit::WebSWServerConnection::terminateWorkerFromClient):
+ (WebKit::WebSWServerConnection::fetchTaskTimedOut):
+ * NetworkProcess/ServiceWorker/WebSWServerConnection.h:
+ * NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in:
+ * NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:
+ (WebKit::WebSWServerToContextConnection::terminateDueToUnresponsiveness):
+ * NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h:
+ * WebProcess/Storage/WebServiceWorkerProvider.cpp:
+ * WebProcess/Storage/WebServiceWorkerProvider.h:
+ * WebProcess/Storage/WebSWClientConnection.cpp:
+ (WebKit::WebSWClientConnection::terminateWorkerForTesting):
+ * WebProcess/Storage/WebSWClientConnection.h:
+ * WebProcess/Storage/WebSWContextManagerConnection.cpp:
+ * WebProcess/Storage/WebSWContextManagerConnection.h:
+ * WebProcess/Storage/WebSWContextManagerConnection.messages.in:
+
2020-04-02 Adrian Perez de Castro <aperez@igalia.com>
[WPE][GTK] Public API should not allow trying to register a special URI scheme
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp
index d1446fe..9aad9ed 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp
@@ -456,10 +456,12 @@
m_networkProcess->parentProcessConnection()->send(Messages::NetworkProcessProxy::RegisterServiceWorkerClientProcess { identifier(), connection.webProcessIdentifier() }, 0);
}
-void WebSWServerConnection::syncTerminateWorkerFromClient(ServiceWorkerIdentifier identifier, CompletionHandler<void()>&& completionHandler)
+void WebSWServerConnection::terminateWorkerFromClient(ServiceWorkerIdentifier serviceWorkerIdentifier, CompletionHandler<void()>&& callback)
{
- syncTerminateWorker(WTFMove(identifier));
- completionHandler();
+ auto* worker = server().workerByID(serviceWorkerIdentifier);
+ if (!worker)
+ return callback();
+ worker->terminate(WTFMove(callback));
}
void WebSWServerConnection::isServiceWorkerRunning(ServiceWorkerIdentifier identifier, CompletionHandler<void(bool)>&& completionHandler)
@@ -485,8 +487,7 @@
return;
worker->setHasTimedOutAnyFetchTasks();
- if (worker->isRunning())
- server().syncTerminateWorker(*worker);
+ worker->terminate();
}
} // namespace WebKit
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h
index 1bb283c..8e4841e 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h
@@ -70,7 +70,6 @@
IPC::Connection& ipcConnection() const { return m_contentConnection.get(); }
void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
- void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);
PAL::SessionID sessionID() const;
@@ -103,7 +102,7 @@
void registerServiceWorkerClient(WebCore::SecurityOriginData&& topOrigin, WebCore::ServiceWorkerClientData&&, const Optional<WebCore::ServiceWorkerRegistrationIdentifier>&, String&& userAgent);
void unregisterServiceWorkerClient(const WebCore::ServiceWorkerClientIdentifier&);
- void syncTerminateWorkerFromClient(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&);
+ void terminateWorkerFromClient(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&);
void isServiceWorkerRunning(WebCore::ServiceWorkerIdentifier, CompletionHandler<void(bool)>&&);
void postMessageToServiceWorkerClient(WebCore::DocumentIdentifier destinationContextIdentifier, const WebCore::MessageWithMessagePorts&, WebCore::ServiceWorkerIdentifier sourceServiceWorkerIdentifier, const String& sourceOrigin) final;
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in
index 953f6c3..e25d71e 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in
@@ -41,7 +41,7 @@
RegisterServiceWorkerClient(struct WebCore::SecurityOriginData topOrigin, struct WebCore::ServiceWorkerClientData data, Optional<WebCore::ServiceWorkerRegistrationIdentifier> controllingServiceWorkerRegistrationIdentifier, String userAgent)
UnregisterServiceWorkerClient(struct WebCore::ServiceWorkerClientIdentifier identifier)
- SyncTerminateWorkerFromClient(WebCore::ServiceWorkerIdentifier workerIdentifier) -> () Synchronous
+ TerminateWorkerFromClient(WebCore::ServiceWorkerIdentifier workerIdentifier) -> () Async
SetThrottleState(bool isThrottleable)
StoreRegistrationsOnDisk() -> () Async
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
index c0f6ceb..637c8ed 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
@@ -106,10 +106,9 @@
send(Messages::WebSWContextManagerConnection::TerminateWorker(serviceWorkerIdentifier));
}
-void WebSWServerToContextConnection::syncTerminateWorker(ServiceWorkerIdentifier serviceWorkerIdentifier)
+void WebSWServerToContextConnection::terminateDueToUnresponsiveness()
{
- if (!sendSync(Messages::WebSWContextManagerConnection::SyncTerminateWorker(serviceWorkerIdentifier), Messages::WebSWContextManagerConnection::SyncTerminateWorker::Reply(), 0, 10_s, IPC::SendSyncOption::ForceDispatchWhenDestinationIsWaitingForUnboundedSyncReply))
- m_connection.networkProcess().parentProcessConnection()->send(Messages::NetworkProcessProxy::TerminateUnresponsiveServiceWorkerProcesses { webProcessIdentifier() }, 0);
+ m_connection.networkProcess().parentProcessConnection()->send(Messages::NetworkProcessProxy::TerminateUnresponsiveServiceWorkerProcesses { webProcessIdentifier() }, 0);
}
void WebSWServerToContextConnection::findClientByIdentifierCompleted(uint64_t requestIdentifier, const Optional<ServiceWorkerClientData>& data, bool hasSecurityError)
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
index fe122cf..5748164 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
@@ -87,11 +87,11 @@
void fireInstallEvent(WebCore::ServiceWorkerIdentifier) final;
void fireActivateEvent(WebCore::ServiceWorkerIdentifier) final;
void terminateWorker(WebCore::ServiceWorkerIdentifier) final;
- void syncTerminateWorker(WebCore::ServiceWorkerIdentifier) final;
void findClientByIdentifierCompleted(uint64_t requestIdentifier, const Optional<WebCore::ServiceWorkerClientData>&, bool hasSecurityError) final;
void matchAllCompleted(uint64_t requestIdentifier, const Vector<WebCore::ServiceWorkerClientData>&) final;
void connectionIsNoLongerNeeded() final;
+ void terminateDueToUnresponsiveness() final;
void connectionClosed();
diff --git a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
index b1a1ea6..c3ccc69 100644
--- a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
+++ b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
@@ -243,9 +243,9 @@
clearPendingJobs();
}
-void WebSWClientConnection::syncTerminateWorker(ServiceWorkerIdentifier identifier)
+void WebSWClientConnection::terminateWorkerForTesting(ServiceWorkerIdentifier identifier, CompletionHandler<void()>&& callback)
{
- sendSync(Messages::WebSWServerConnection::SyncTerminateWorkerFromClient { identifier }, Messages::WebSWServerConnection::SyncTerminateWorkerFromClient::Reply());
+ sendWithAsyncReply(Messages::WebSWServerConnection::TerminateWorkerFromClient { identifier }, WTFMove(callback));
}
void WebSWClientConnection::isServiceWorkerRunning(ServiceWorkerIdentifier identifier, CompletionHandler<void(bool)>&& callback)
diff --git a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h
index 76c4628..6c92e70 100644
--- a/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h
+++ b/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h
@@ -62,11 +62,11 @@
void connectionToServerLost();
- void syncTerminateWorker(WebCore::ServiceWorkerIdentifier) final;
-
bool isThrottleable() const { return m_isThrottleable; }
void updateThrottleState();
+ void terminateWorkerForTesting(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&);
+
private:
WebSWClientConnection();
diff --git a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
index 9c0b5696..3bb2021 100644
--- a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
+++ b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
@@ -255,11 +255,6 @@
SWContextManager::singleton().terminateWorker(identifier, SWContextManager::workerTerminationTimeout, nullptr);
}
-void WebSWContextManagerConnection::syncTerminateWorker(ServiceWorkerIdentifier identifier, Messages::WebSWContextManagerConnection::SyncTerminateWorker::DelayedReply&& reply)
-{
- SWContextManager::singleton().terminateWorker(identifier, SWContextManager::syncWorkerTerminationTimeout, WTFMove(reply));
-}
-
void WebSWContextManagerConnection::postMessageToServiceWorkerClient(const ServiceWorkerClientIdentifier& destinationIdentifier, const MessageWithMessagePorts& message, ServiceWorkerIdentifier sourceIdentifier, const String& sourceOrigin)
{
m_connectionToNetworkProcess->send(Messages::WebSWServerToContextConnection::PostMessageToServiceWorkerClient(destinationIdentifier, message, sourceIdentifier, sourceOrigin), 0);
diff --git a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
index 6d8c876..10ccd18 100644
--- a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
+++ b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
@@ -60,7 +60,6 @@
~WebSWContextManagerConnection();
void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
- void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&) final;
private:
void updatePreferencesStore(const WebPreferencesStore&);
@@ -91,7 +90,6 @@
void fireInstallEvent(WebCore::ServiceWorkerIdentifier);
void fireActivateEvent(WebCore::ServiceWorkerIdentifier);
void terminateWorker(WebCore::ServiceWorkerIdentifier);
- void syncTerminateWorker(WebCore::ServiceWorkerIdentifier, Messages::WebSWContextManagerConnection::SyncTerminateWorkerDelayedReply&&);
void findClientByIdentifierCompleted(uint64_t requestIdentifier, Optional<WebCore::ServiceWorkerClientData>&&, bool hasSecurityError);
void matchAllCompleted(uint64_t matchAllRequestIdentifier, Vector<WebCore::ServiceWorkerClientData>&&);
void setUserAgent(String&& userAgent);
diff --git a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in
index 4a4ddb7..5284cd4 100644
--- a/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in
+++ b/Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in
@@ -31,7 +31,6 @@
FireInstallEvent(WebCore::ServiceWorkerIdentifier identifier)
FireActivateEvent(WebCore::ServiceWorkerIdentifier identifier)
TerminateWorker(WebCore::ServiceWorkerIdentifier identifier)
- SyncTerminateWorker(WebCore::ServiceWorkerIdentifier identifier) -> () Synchronous
FindClientByIdentifierCompleted(uint64_t clientIdRequestIdentifier, Optional<WebCore::ServiceWorkerClientData> data, bool hasSecurityError)
MatchAllCompleted(uint64_t matchAllRequestIdentifier, Vector<WebCore::ServiceWorkerClientData> clientsData)
SetUserAgent(String userAgent)
diff --git a/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp b/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp
index a4626d1..8f75835 100644
--- a/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp
+++ b/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp
@@ -70,6 +70,11 @@
connection.updateThrottleState();
}
+void WebServiceWorkerProvider::terminateWorkerForTesting(WebCore::ServiceWorkerIdentifier identifier, CompletionHandler<void()>&& callback)
+{
+ WebProcess::singleton().ensureNetworkProcessConnection().serviceWorkerConnection().terminateWorkerForTesting(identifier, WTFMove(callback));
+}
+
} // namespace WebKit
#endif // ENABLE(SERVICE_WORKER)
diff --git a/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h b/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h
index aaefad3..e5ca208 100644
--- a/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h
+++ b/Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h
@@ -53,6 +53,7 @@
WebServiceWorkerProvider();
WebCore::SWClientConnection& serviceWorkerConnection() final;
+ void terminateWorkerForTesting(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&) final;
}; // class WebServiceWorkerProvider
} // namespace WebKit
