We should be able to terminate service workers that are unresponsive
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 00:57:42 +0000 (00:57 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 00:57:42 +0000 (00:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181563
<rdar://problem/35280031>

Reviewed by Alex Christensen.

Source/WebCore:

Test: http/tests/workers/service/postmessage-after-terminating-hung-worker.html

* workers/service/context/SWContextManager.cpp:
(WebCore::SWContextManager::terminateWorker):
Before calling WorkerThread::stop(), set a timer with the given timeout parameter.
If the worker thread has not stopped when the timer fires, forcefully exit the
service worker process. The StorageProcess will take care of relaunching the
service worker process if it exits abruptly.

(WebCore::SWContextManager::serviceWorkerFailedToTerminate):
Log error message if we failed to terminate a service worker and call exit().

(WebCore::SWContextManager::ServiceWorkerTerminationRequest::ServiceWorkerTerminationRequest):

* workers/service/context/SWContextManager.h:

Source/WebKit:

* WebProcess/Storage/WebSWContextManagerConnection.cpp:
(WebKit::WebSWContextManagerConnection::terminateWorker):
Use a 10 second timeout for forcefully exiting the service worker process when
the service worker in question fails to terminate.

(WebKit::WebSWContextManagerConnection::syncTerminateWorker):
Use a 100ms timeout for forcefully exiting the service worker process when
the service worker in question fails to terminate. This method is only called
from the layout tests, which is why we use a very short timeout.

Source/WTF:

* wtf/ObjectIdentifier.h:
(WTF::ObjectIdentifier::loggingString const):
Allow using loggingString() from RELEASE_LOG().

LayoutTests:

Add layout test coverage.

* http/tests/workers/service/postmessage-after-terminating-hung-worker-expected.txt: Added.
* http/tests/workers/service/postmessage-after-terminating-hung-worker.html: Added.
* http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js: Added.
* http/tests/workers/service/resources/postmessage-echo-worker-mayhang.js: Added.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker.html [new file with mode: 0644]
LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js [new file with mode: 0644]
LayoutTests/http/tests/workers/service/resources/postmessage-echo-worker-mayhang.js [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/ObjectIdentifier.h
Source/WebCore/ChangeLog
Source/WebCore/workers/service/context/SWContextManager.cpp
Source/WebCore/workers/service/context/SWContextManager.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp

index 3b33336..09a05c8 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-18  Chris Dumez  <cdumez@apple.com>
+
+        We should be able to terminate service workers that are unresponsive
+        https://bugs.webkit.org/show_bug.cgi?id=181563
+        <rdar://problem/35280031>
+
+        Reviewed by Alex Christensen.
+
+        Add layout test coverage.
+
+        * http/tests/workers/service/postmessage-after-terminating-hung-worker-expected.txt: Added.
+        * http/tests/workers/service/postmessage-after-terminating-hung-worker.html: Added.
+        * http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js: Added.
+        * http/tests/workers/service/resources/postmessage-echo-worker-mayhang.js: Added.
+
 2018-01-18  Joanmarie Diggs  <jdiggs@igalia.com>
 
         AX: roles-computedRoleString.html layout test should support enabling/disabling individual test cases
diff --git a/LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker-expected.txt b/LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker-expected.txt
new file mode 100644 (file)
index 0000000..e715249
--- /dev/null
@@ -0,0 +1,8 @@
+PASS: Service worker received message 'HANG'
+PASS: ServiceWorker received message: PASS: Service worker received message 'HANG'
+Service Worker should now be hung
+Terminating service worker...
+Terminated service worker.
+PASS: Service worker received message 'MessageAfterTerminatingHungServiceWorker'
+PASS: Service worker responded to message after hanging and being terminated
+
diff --git a/LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker.html b/LayoutTests/http/tests/workers/service/postmessage-after-terminating-hung-worker.html
new file mode 100644 (file)
index 0000000..da647e4
--- /dev/null
@@ -0,0 +1,9 @@
+<html>
+<head>
+<script src="resources/sw-test-pre.js"></script>
+</head>
+<body>
+
+<script src="resources/postmessage-after-terminating-hung-worker.js"></script>
+</body>
+</html>
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
new file mode 100644 (file)
index 0000000..e0f2e8f
--- /dev/null
@@ -0,0 +1,29 @@
+let state = "WaitingForHang";
+
+navigator.serviceWorker.addEventListener("message", 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);
+        log("Terminated service worker.");
+        state = "WaitingForMessageAfterTerminatingHungServiceWorker"
+        handle = setInterval(function() {
+            worker.postMessage("MessageAfterTerminatingHungServiceWorker");
+        }, 10);
+        return;
+    }
+    if (state === "WaitingForMessageAfterTerminatingHungServiceWorker") {
+        log("PASS: Service worker responded to message after hanging and being terminated");
+        state = "Done";
+        clearInterval(handle);
+        finishSWTest();
+        return;
+    }
+});
+
+navigator.serviceWorker.register("resources/postmessage-echo-worker-mayhang.js", { }).then(function(registration) {
+    worker = registration.installing;
+    worker.postMessage("HANG");
+});
diff --git a/LayoutTests/http/tests/workers/service/resources/postmessage-echo-worker-mayhang.js b/LayoutTests/http/tests/workers/service/resources/postmessage-echo-worker-mayhang.js
new file mode 100644 (file)
index 0000000..e67fb6f
--- /dev/null
@@ -0,0 +1,8 @@
+self.addEventListener("message", (event) => {
+    event.source.postMessage("PASS: Service worker received message '" + event.data + "'");
+
+    if (event.data === "HANG") {
+        while(1) { };
+    }
+});
+
index 867f7d8..e3ea84e 100644 (file)
@@ -1,3 +1,15 @@
+2018-01-18  Chris Dumez  <cdumez@apple.com>
+
+        We should be able to terminate service workers that are unresponsive
+        https://bugs.webkit.org/show_bug.cgi?id=181563
+        <rdar://problem/35280031>
+
+        Reviewed by Alex Christensen.
+
+        * wtf/ObjectIdentifier.h:
+        (WTF::ObjectIdentifier::loggingString const):
+        Allow using loggingString() from RELEASE_LOG().
+
 2018-01-18  Dan Bernstein  <mitz@apple.com>
 
         [Xcode] Streamline and future-proof target-macOS-version-dependent build setting definitions
index 21b5993..4be0c88 100644 (file)
@@ -67,12 +67,10 @@ public:
 
     uint64_t toUInt64() const { return m_identifier; }
     
-#ifndef NDEBUG
     String loggingString() const
     {
         return String::number(m_identifier);
     }
-#endif
 
 private:
     template<typename U> friend ObjectIdentifier<U> generateObjectIdentifier();
index 4a2e561..f5c9226 100644 (file)
@@ -1,3 +1,27 @@
+2018-01-18  Chris Dumez  <cdumez@apple.com>
+
+        We should be able to terminate service workers that are unresponsive
+        https://bugs.webkit.org/show_bug.cgi?id=181563
+        <rdar://problem/35280031>
+
+        Reviewed by Alex Christensen.
+
+        Test: http/tests/workers/service/postmessage-after-terminating-hung-worker.html
+
+        * workers/service/context/SWContextManager.cpp:
+        (WebCore::SWContextManager::terminateWorker):
+        Before calling WorkerThread::stop(), set a timer with the given timeout parameter.
+        If the worker thread has not stopped when the timer fires, forcefully exit the
+        service worker process. The StorageProcess will take care of relaunching the
+        service worker process if it exits abruptly.
+
+        (WebCore::SWContextManager::serviceWorkerFailedToTerminate):
+        Log error message if we failed to terminate a service worker and call exit().
+
+        (WebCore::SWContextManager::ServiceWorkerTerminationRequest::ServiceWorkerTerminationRequest):
+
+        * workers/service/context/SWContextManager.h:
+
 2018-01-18  Youenn Fablet  <youenn@apple.com>
 
         Do not go to the storage process when loading a main resource if there is no service worker registered
index c8dc254..7f5d879 100644 (file)
 
 #if ENABLE(SERVICE_WORKER)
 #include "SWContextManager.h"
+
+#include "Logging.h"
 #include "ServiceWorkerClientIdentifier.h"
 #include "ServiceWorkerGlobalScope.h"
+#include <unistd.h>
 
 namespace WebCore {
 
@@ -102,21 +105,28 @@ void SWContextManager::fireActivateEvent(ServiceWorkerIdentifier identifier)
     serviceWorker->thread().fireActivateEvent();
 }
 
-void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Function<void()>&& completionHandler)
+void SWContextManager::terminateWorker(ServiceWorkerIdentifier identifier, Seconds timeout, Function<void()>&& completionHandler)
 {
     auto serviceWorker = m_workerMap.take(identifier);
-    if (!serviceWorker)
+    if (!serviceWorker) {
+        if (completionHandler)
+            completionHandler();
         return;
+    }
 
     serviceWorker->setAsTerminatingOrTerminated();
 
-    serviceWorker->thread().stop([identifier, serviceWorker = WTFMove(serviceWorker), completionHandler = WTFMove(completionHandler)]() mutable {
+    m_pendingServiceWorkerTerminationRequests.add(identifier, std::make_unique<ServiceWorkerTerminationRequest>(*this, identifier, timeout));
+
+    serviceWorker->thread().stop([this, identifier, serviceWorker = WTFMove(serviceWorker), completionHandler = WTFMove(completionHandler)]() mutable {
+        m_pendingServiceWorkerTerminationRequests.remove(identifier);
+
         if (auto* connection = SWContextManager::singleton().connection())
             connection->workerTerminated(identifier);
 
         if (completionHandler)
             completionHandler();
-        
+
         // Spin the runloop before releasing the worker thread proxy, as there would otherwise be
         // a race towards its destruction.
         callOnMainThread([serviceWorker = WTFMove(serviceWorker)] { });
@@ -141,6 +151,19 @@ bool SWContextManager::postTaskToServiceWorker(ServiceWorkerIdentifier identifie
     return true;
 }
 
+NO_RETURN_DUE_TO_CRASH void SWContextManager::serviceWorkerFailedToTerminate(ServiceWorkerIdentifier serviceWorkerIdentifier)
+{
+    UNUSED_PARAM(serviceWorkerIdentifier);
+    RELEASE_LOG_ERROR(ServiceWorker, "Failed to terminate service worker with identifier %s, killing the service worker process", serviceWorkerIdentifier.loggingString().utf8().data());
+    _exit(EXIT_FAILURE);
+}
+
+SWContextManager::ServiceWorkerTerminationRequest::ServiceWorkerTerminationRequest(SWContextManager& manager, ServiceWorkerIdentifier serviceWorkerIdentifier, Seconds timeout)
+    : m_timeoutTimer([this, &manager, serviceWorkerIdentifier] { manager.serviceWorkerFailedToTerminate(serviceWorkerIdentifier); })
+{
+    m_timeoutTimer.startOneShot(timeout);
+}
+
 } // namespace WebCore
 
 #endif
index add7837..e58047e 100644 (file)
@@ -70,7 +70,7 @@ public:
     WEBCORE_EXPORT void postMessageToServiceWorker(ServiceWorkerIdentifier destination, Ref<SerializedScriptValue>&& message, ServiceWorkerOrClientData&& sourceData);
     WEBCORE_EXPORT void fireInstallEvent(ServiceWorkerIdentifier);
     WEBCORE_EXPORT void fireActivateEvent(ServiceWorkerIdentifier);
-    WEBCORE_EXPORT void terminateWorker(ServiceWorkerIdentifier, Function<void()>&&);
+    WEBCORE_EXPORT void terminateWorker(ServiceWorkerIdentifier, Seconds timeout, Function<void()>&&);
 
     void forEachServiceWorkerThread(const WTF::Function<void(ServiceWorkerThreadProxy&)>&);
 
@@ -85,10 +85,20 @@ private:
     SWContextManager() = default;
 
     void startedServiceWorker(std::optional<ServiceWorkerJobDataIdentifier>, ServiceWorkerIdentifier, const String& exceptionMessage);
+    void serviceWorkerFailedToTerminate(ServiceWorkerIdentifier);
 
     HashMap<ServiceWorkerIdentifier, RefPtr<ServiceWorkerThreadProxy>> m_workerMap;
     std::unique_ptr<Connection> m_connection;
     ServiceWorkerCreationCallback* m_serviceWorkerCreationCallback { nullptr };
+
+    class ServiceWorkerTerminationRequest {
+    public:
+        ServiceWorkerTerminationRequest(SWContextManager&, ServiceWorkerIdentifier, Seconds timeout);
+
+    private:
+        Timer m_timeoutTimer;
+    };
+    HashMap<ServiceWorkerIdentifier, std::unique_ptr<ServiceWorkerTerminationRequest>> m_pendingServiceWorkerTerminationRequests;
 };
 
 } // namespace WebCore
index 3c9ded9..1887ab3 100644 (file)
@@ -1,3 +1,21 @@
+2018-01-18  Chris Dumez  <cdumez@apple.com>
+
+        We should be able to terminate service workers that are unresponsive
+        https://bugs.webkit.org/show_bug.cgi?id=181563
+        <rdar://problem/35280031>
+
+        Reviewed by Alex Christensen.
+
+        * WebProcess/Storage/WebSWContextManagerConnection.cpp:
+        (WebKit::WebSWContextManagerConnection::terminateWorker):
+        Use a 10 second timeout for forcefully exiting the service worker process when
+        the service worker in question fails to terminate.
+
+        (WebKit::WebSWContextManagerConnection::syncTerminateWorker):
+        Use a 100ms timeout for forcefully exiting the service worker process when
+        the service worker in question fails to terminate. This method is only called
+        from the layout tests, which is why we use a very short timeout.
+
 2018-01-18  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, suppress deprecation warnings to fix the build with a newer SDK.
index affc5ac..a71955d 100644 (file)
@@ -62,6 +62,9 @@ using namespace WebCore;
 
 namespace WebKit {
 
+static const Seconds asyncWorkerTerminationTimeout { 10_s };
+static const Seconds syncWorkerTerminationTimeout { 100_ms }; // Only used by layout tests.
+
 class ServiceWorkerFrameLoaderClient final : public EmptyFrameLoaderClient {
 public:
     ServiceWorkerFrameLoaderClient(WebSWContextManagerConnection& connection, PAL::SessionID sessionID, uint64_t pageID, uint64_t frameID, const String& userAgent)
@@ -196,12 +199,12 @@ void WebSWContextManagerConnection::fireActivateEvent(ServiceWorkerIdentifier id
 
 void WebSWContextManagerConnection::terminateWorker(ServiceWorkerIdentifier identifier)
 {
-    SWContextManager::singleton().terminateWorker(identifier, nullptr);
+    SWContextManager::singleton().terminateWorker(identifier, asyncWorkerTerminationTimeout, nullptr);
 }
 
 void WebSWContextManagerConnection::syncTerminateWorker(ServiceWorkerIdentifier identifier, Ref<Messages::WebSWContextManagerConnection::SyncTerminateWorker::DelayedReply>&& reply)
 {
-    SWContextManager::singleton().terminateWorker(identifier, [reply = WTFMove(reply)] {
+    SWContextManager::singleton().terminateWorker(identifier, syncWorkerTerminationTimeout, [reply = WTFMove(reply)] {
         reply->send();
     });
 }