Changeset 259859 in webkit

Timestamp:

Apr 10, 2020, 3:50:35 AM (6 years ago)

Author:

youenn@apple.com

Message:

SWServer should not run a service worker that is terminating
​https://bugs.webkit.org/show_bug.cgi?id=210044

Reviewed by Chris Dumez.

Source/WebCore:

If a test is being terminated and we want to restart it, we were previously running it right away.
This does not work well as the service worker process might still have the terminating service worker in its map.
Also, if the service worker is not able to terminate properly, we will kill the service worker process so there is no reason
to try running this service worker in this process.
Instead, wait for the service worker to terminate (which might include terminating the service worker process).

In addition, we remove the isServiceWorkerRunning internals API since this is potentially flaky as the service worker
might be terminated and rerunning in between two isServiceWorkerRunning checks.
Instead, we introduce whenServiceWorkerIsTerminated which will resolve as soon as the service worker goes to terminated.

Covered by existing spinning tests no longer crashing.

• testing/Internals.cpp:

(WebCore::Internals::whenServiceWorkerIsTerminated):

• testing/Internals.h:
• testing/Internals.idl:
• workers/service/SWClientConnection.h:

(WebCore::SWClientConnection::whenServiceWorkerIsTerminatedForTesting):

• workers/service/server/SWServer.cpp:

(WebCore::SWServer::runServiceWorkerIfNecessary):
(WebCore::SWServer::runServiceWorker):
(WebCore::SWServer::workerContextTerminated):

• workers/service/server/SWServerWorker.cpp:

(WebCore::SWServerWorker::whenTerminated):
(WebCore::SWServerWorker::setState):

• workers/service/server/SWServerWorker.h:

(WebCore::SWServerWorker::isNotRunning const):

Source/WebKit:

Implement whenServiceWorkerIsTerminated check.

• NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:

(WebKit::WebSWServerConnection::whenServiceWorkerIsTerminatedForTesting):

• NetworkProcess/ServiceWorker/WebSWServerConnection.h:
• NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in:
• WebProcess/Storage/WebSWClientConnection.cpp:

(WebKit::WebSWClientConnection::whenServiceWorkerIsTerminatedForTesting):

• WebProcess/Storage/WebSWClientConnection.h:

LayoutTests:

• http/wpt/service-workers/resources/routines.js:

(async waitForServiceWorkerNoLongerRunning):
Use new internals API.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/wpt/service-workers/resources/routines.js (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)
• Source/WebCore/workers/service/SWClientConnection.h (modified) (1 diff)
• Source/WebCore/workers/service/server/SWServer.cpp (modified) (3 diffs)
• Source/WebCore/workers/service/server/SWServerWorker.cpp (modified) (2 diffs)
• Source/WebCore/workers/service/server/SWServerWorker.h (modified) (2 diffs)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp (modified) (1 diff)
• Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h (modified) (1 diff)
• Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in (modified) (1 diff)
• Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp (modified) (1 diff)
• Source/WebKit/WebProcess/Storage/WebSWClientConnection.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-04-10  Youenn Fablet  <youenn@apple.com>
+
+        SWServer should not run a service worker that is terminating
+        https://bugs.webkit.org/show_bug.cgi?id=210044
+
+        Reviewed by Chris Dumez.
+
+        * http/wpt/service-workers/resources/routines.js:
+        (async waitForServiceWorkerNoLongerRunning):
+        Use new internals API.
+
 2020-04-09  Peng Liu  <peng.liu6@apple.com>
 

trunk/LayoutTests/http/wpt/service-workers/resources/routines.js

@@ -34 +34 @@
         return Promise.reject("requires internals");
 
-    let count = 100;
-    while (--count > 0 && await internals.isServiceWorkerRunning(worker)) {
-        worker.postMessage("test");
-        await new Promise(resolve => setTimeout(resolve, 50));
-    }
-    if (count === 0)
-        return Promise.reject("service worker is still running");
+    const promise = internals.whenServiceWorkerIsTerminated(worker);
+    let timer = setInterval(() => worker.postMessage("test"), 50);
+    await promise;
+    clearInterval(timer);
 }

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-10  Youenn Fablet  <youenn@apple.com>
+
+        SWServer should not run a service worker that is terminating
+        https://bugs.webkit.org/show_bug.cgi?id=210044
+
+        Reviewed by Chris Dumez.
+
+        If a test is being terminated and we want to restart it, we were previously running it right away.
+        This does not work well as the service worker process might still have the terminating service worker in its map.
+        Also, if the service worker is not able to terminate properly, we will kill the service worker process so there is no reason
+        to try running this service worker in this process.
+        Instead, wait for the service worker to terminate (which might include terminating the service worker process).
+
+        In addition, we remove the isServiceWorkerRunning internals API since this is potentially flaky as the service worker
+        might be terminated and rerunning in between two isServiceWorkerRunning checks.
+        Instead, we introduce whenServiceWorkerIsTerminated which will resolve as soon as the service worker goes to terminated.
+
+        Covered by existing spinning tests no longer crashing.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::whenServiceWorkerIsTerminated):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+        * workers/service/SWClientConnection.h:
+        (WebCore::SWClientConnection::whenServiceWorkerIsTerminatedForTesting):
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::runServiceWorkerIfNecessary):
+        (WebCore::SWServer::runServiceWorker):
+        (WebCore::SWServer::workerContextTerminated):
+        * workers/service/server/SWServerWorker.cpp:
+        (WebCore::SWServerWorker::whenTerminated):
+        (WebCore::SWServerWorker::setState):
+        * workers/service/server/SWServerWorker.h:
+        (WebCore::SWServerWorker::isNotRunning const):
+
 2020-04-10  Charlie Turner  <cturner@igalia.com>
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -5145 +5145 @@
 }
 
-void Internals::isServiceWorkerRunning(ServiceWorker& worker, DOMPromiseDeferred<IDLBoolean>&& promise)
-{
-    return ServiceWorkerProvider::singleton().serviceWorkerConnection().isServiceWorkerRunning(worker.identifier(), [promise = WTFMove(promise)](bool result) mutable {
-        promise.resolve(result);
+void Internals::whenServiceWorkerIsTerminated(ServiceWorker& worker, DOMPromiseDeferred<void>&& promise)
+{
+    return ServiceWorkerProvider::singleton().serviceWorkerConnection().whenServiceWorkerIsTerminatedForTesting(worker.identifier(), [promise = WTFMove(promise)]() mutable {
+        promise.resolve();
     });
 }

trunk/Source/WebCore/testing/Internals.h

@@ -785 +785 @@
     void hasServiceWorkerRegistration(const String& clientURL, HasRegistrationPromise&&);
     void terminateServiceWorker(ServiceWorker&, DOMPromiseDeferred<void>&&);
-    void isServiceWorkerRunning(ServiceWorker&, DOMPromiseDeferred<IDLBoolean>&&);
+    void whenServiceWorkerIsTerminated(ServiceWorker&, DOMPromiseDeferred<void>&&);
 #endif
 

trunk/Source/WebCore/testing/Internals.idl

@@ -771 +771 @@
     [Conditional=SERVICE_WORKER] Promise<boolean> hasServiceWorkerRegistration(DOMString scopeURL);
     [Conditional=SERVICE_WORKER] Promise<void> terminateServiceWorker(ServiceWorker worker);
-    [Conditional=SERVICE_WORKER] Promise<boolean> isServiceWorkerRunning(ServiceWorker worker);
+    [Conditional=SERVICE_WORKER] Promise<void> whenServiceWorkerIsTerminated(ServiceWorker worker);
 
     [CallWith=Document, Conditional=APPLE_PAY] readonly attribute MockPaymentCoordinator mockPaymentCoordinator;

trunk/Source/WebCore/workers/service/SWClientConnection.h

@@ -87 +87 @@
 
     virtual void storeRegistrationsOnDiskForTesting(CompletionHandler<void()>&& callback) { callback(); }
-    virtual void isServiceWorkerRunning(ServiceWorkerIdentifier, CompletionHandler<void(bool)>&& callback) { callback(false); }
+    virtual void whenServiceWorkerIsTerminatedForTesting(ServiceWorkerIdentifier, CompletionHandler<void()>&& callback) { callback(); }
 
     WEBCORE_EXPORT void registerServiceWorkerClients();

trunk/Source/WebCore/workers/service/server/SWServer.cpp

@@ -686 +686 @@
     }
 
+    if (worker->isTerminating()) {
+        worker->whenTerminated([this, weakThis = makeWeakPtr(this), identifier, callback = WTFMove(callback)]() mutable {
+            if (!weakThis)
+                return callback(nullptr);
+            runServiceWorkerIfNecessary(identifier, WTFMove(callback));
+        });
+        return;
+    }
+
     if (!contextConnection) {
         auto& serviceWorkerRunRequestsForOrigin = m_serviceWorkerRunRequests.ensure(worker->registrableDomain(), [] {
@@ -713 +722 @@
         return false;
 
-    auto addResult = m_runningOrTerminatingWorkers.add(identifier, *worker);
-    ASSERT_UNUSED(addResult, addResult.isNewEntry || worker->isTerminating());
+    ASSERT(!worker->isTerminating());
+    ASSERT(!m_runningOrTerminatingWorkers.contains(identifier));
+    m_runningOrTerminatingWorkers.add(identifier, *worker);
 
     worker->setState(SWServerWorker::State::Running);
@@ -739 +749 @@
 void SWServer::workerContextTerminated(SWServerWorker& worker)
 {
-    worker.setState(SWServerWorker::State::NotRunning);
-
-    if (auto* jobQueue = m_jobQueues.get(worker.registrationKey()))
-        jobQueue->cancelJobsFromServiceWorker(worker.identifier());
-
     // At this point if no registrations are referencing the worker then it will be destroyed,
     // removing itself from the m_workersByID map.
     auto result = m_runningOrTerminatingWorkers.take(worker.identifier());
     ASSERT_UNUSED(result, result && result->ptr() == &worker);
+
+    worker.setState(SWServerWorker::State::NotRunning);
+
+    if (auto* jobQueue = m_jobQueues.get(worker.registrationKey()))
+        jobQueue->cancelJobsFromServiceWorker(worker.identifier());
 }
 

trunk/Source/WebCore/workers/service/server/SWServerWorker.cpp

@@ -105 +105 @@
 }
 
+void SWServerWorker::whenTerminated(CompletionHandler<void()>&& callback)
+{
+    ASSERT(isRunning() || isTerminating());
+    m_terminationCallbacks.append(WTFMove(callback));
+}
+
 void SWServerWorker::startTermination(CompletionHandler<void()>&& callback)
 {
@@ -297 +303 @@
 {
     ASSERT(state != State::Running || m_registration);
+    ASSERT(state != State::Running || m_state != State::Terminating);
     m_state = state;
 

trunk/Source/WebCore/workers/service/server/SWServerWorker.h

@@ -64 +64 @@
 
     WEBCORE_EXPORT void terminate(CompletionHandler<void()>&& = [] { });
+    WEBCORE_EXPORT void whenTerminated(CompletionHandler<void()>&&);
 
     WEBCORE_EXPORT void whenActivated(CompletionHandler<void(bool)>&&);
@@ -74 +75 @@
     bool isRunning() const { return m_state == State::Running; }
     bool isTerminating() const { return m_state == State::Terminating; }
+    bool isNotRunning() const { return m_state == State::NotRunning; }
     void setState(State);
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-04-10  Youenn Fablet  <youenn@apple.com>
+
+        SWServer should not run a service worker that is terminating
+        https://bugs.webkit.org/show_bug.cgi?id=210044
+
+        Reviewed by Chris Dumez.
+
+        Implement whenServiceWorkerIsTerminated check.
+
+        * NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:
+        (WebKit::WebSWServerConnection::whenServiceWorkerIsTerminatedForTesting):
+        * NetworkProcess/ServiceWorker/WebSWServerConnection.h:
+        * NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in:
+        * WebProcess/Storage/WebSWClientConnection.cpp:
+        (WebKit::WebSWClientConnection::whenServiceWorkerIsTerminatedForTesting):
+        * WebProcess/Storage/WebSWClientConnection.h:
+
 2020-04-10  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp

@@ -465 +465 @@
 }
 
-void WebSWServerConnection::isServiceWorkerRunning(ServiceWorkerIdentifier identifier, CompletionHandler<void(bool)>&& completionHandler)
+void WebSWServerConnection::whenServiceWorkerIsTerminatedForTesting(WebCore::ServiceWorkerIdentifier identifier, CompletionHandler<void()>&& completionHandler)
 {
     auto* worker = SWServerWorker::existingWorkerForIdentifier(identifier);
-    completionHandler(worker ? worker->isRunning() : false);
+    if (!worker || worker->isNotRunning())
+        return completionHandler();
+    worker->whenTerminated(WTFMove(completionHandler));
 }
 

trunk/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.h

@@ -104 +104 @@
     void unregisterServiceWorkerClient(const WebCore::ServiceWorkerClientIdentifier&);
     void terminateWorkerFromClient(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&);
-    void isServiceWorkerRunning(WebCore::ServiceWorkerIdentifier, CompletionHandler<void(bool)>&&);
+    void whenServiceWorkerIsTerminatedForTesting(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&);
 
     void postMessageToServiceWorkerClient(WebCore::DocumentIdentifier destinationContextIdentifier, const WebCore::MessageWithMessagePorts&, WebCore::ServiceWorkerIdentifier sourceServiceWorkerIdentifier, const String& sourceOrigin) final;

trunk/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.messages.in

@@ -43 +43 @@
 
     TerminateWorkerFromClient(WebCore::ServiceWorkerIdentifier workerIdentifier) -> () Async
+    WhenServiceWorkerIsTerminatedForTesting(WebCore::ServiceWorkerIdentifier workerIdentifier) -> () Async
 
     SetThrottleState(bool isThrottleable)
     StoreRegistrationsOnDisk() -> () Async
-    IsServiceWorkerRunning(WebCore::ServiceWorkerIdentifier workerIdentifier) -> (bool isRunning) Async
 }
 

trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp

@@ -249 +249 @@
 }
 
-void WebSWClientConnection::isServiceWorkerRunning(ServiceWorkerIdentifier identifier, CompletionHandler<void(bool)>&& callback)
-{
-    sendWithAsyncReply(Messages::WebSWServerConnection::IsServiceWorkerRunning { identifier }, WTFMove(callback));
+void WebSWClientConnection::whenServiceWorkerIsTerminatedForTesting(ServiceWorkerIdentifier identifier, CompletionHandler<void()>&& callback)
+{
+    sendWithAsyncReply(Messages::WebSWServerConnection::WhenServiceWorkerIsTerminatedForTesting { identifier }, WTFMove(callback));
 }
 

trunk/Source/WebKit/WebProcess/Storage/WebSWClientConnection.h

@@ -87 +87 @@
 
     void getRegistrations(WebCore::SecurityOriginData&& topOrigin, const URL& clientURL, GetRegistrationsCallback&&) final;
-    void isServiceWorkerRunning(WebCore::ServiceWorkerIdentifier, CompletionHandler<void(bool)>&&) final;
+    void whenServiceWorkerIsTerminatedForTesting(WebCore::ServiceWorkerIdentifier, CompletionHandler<void()>&&) final;
 
     void didResolveRegistrationPromise(const WebCore::ServiceWorkerRegistrationKey&) final;