MessagePort is not always destroyed on the right thread
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Mar 2018 18:21:10 +0000 (18:21 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Mar 2018 18:21:10 +0000 (18:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183619
<rdar://problem/38204711>

Reviewed by Chris Dumez.

Source/WebCore:

Add assertion to ensure MessagePort is destroyed in the right thread.
Modify methods taking a ref in a lambda to rely on weak pointers and refing the WorkerThread if in a worker context.
It is safe to ref the WorkerThread since it is thread safe ref counted and we are passing the ref to the main thread
where the WorkerThread is expected to be destroyed.

Test: http/tests/workers/worker-messageport-2.html

* dom/MessagePort.cpp:
(WebCore::MessagePort::~MessagePort):
(WebCore::MessagePort::dispatchMessages):
(WebCore::MessagePort::updateActivity):
(WebCore::MessagePort::hasPendingActivity const):
* dom/MessagePort.h:

LayoutTests:

* TestExpectations:
* http/tests/workers/worker-messageport-2-expected.txt: Added.
* http/tests/workers/worker-messageport-2.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/http/tests/workers/worker-messageport-2-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/workers/worker-messageport-2.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/MessagePort.cpp
Source/WebCore/dom/MessagePort.h

index c370479..13aad06 100644 (file)
@@ -1,3 +1,15 @@
+2018-03-15  Youenn Fablet  <youenn@apple.com>
+
+        MessagePort is not always destroyed on the right thread
+        https://bugs.webkit.org/show_bug.cgi?id=183619
+        <rdar://problem/38204711>
+
+        Reviewed by Chris Dumez.
+
+        * TestExpectations:
+        * http/tests/workers/worker-messageport-2-expected.txt: Added.
+        * http/tests/workers/worker-messageport-2.html: Added.
+
 2018-03-15  Ms2ger  <Ms2ger@igalia.com>
 
         [GTK][WPE] Enable service workers
index 5560d7b..e1fd8cf 100644 (file)
@@ -211,6 +211,8 @@ imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-red
 # Reenable it when having a custom gc exposed in service workers.
 [ Debug ] http/wpt/service-workers/fetchEvent.https.html [ Skip ]
 
+[ Debug ] http/tests/workers/worker-messageport-2.html [ Slow ]
+
 # Skip workers tests that are timing out or are SharedWorker related only
 imported/w3c/web-platform-tests/workers/constructors/Worker/same-origin.html [ Skip ]
 imported/w3c/web-platform-tests/workers/data-url-shared.html [ Skip ]
diff --git a/LayoutTests/http/tests/workers/worker-messageport-2-expected.txt b/LayoutTests/http/tests/workers/worker-messageport-2-expected.txt
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
@@ -0,0 +1 @@
+PASS
diff --git a/LayoutTests/http/tests/workers/worker-messageport-2.html b/LayoutTests/http/tests/workers/worker-messageport-2.html
new file mode 100644 (file)
index 0000000..0c081e1
--- /dev/null
@@ -0,0 +1,45 @@
+<html>
+<body>
+<p>Test postMessage and garbage collection.</p>
+<div id=result></div>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+var worker = new Worker('resources/messageport-echo-worker.js');
+
+worker.onmessage = (event) => {
+    if (event.data !== "ready") {
+        document.body.innerHTML = "FAIL";
+        if (window.testRunner)
+            testRunner.notifyDone();
+        return;
+    }
+    worker.terminate();
+
+    function gcRec(n) {
+        if (n < 1)
+            return {};
+        var temp = {i: "ab" + i + (i / 100000)};
+        temp += "foo";
+        gcRec(n-1);
+    }
+    for (var i = 0; i < 100000; i++)
+        gcRec(10);
+
+    setTimeout(() => {
+        document.body.innerHTML = "PASS";
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, 0);
+}
+
+var channel = new MessageChannel();
+channel.port1.onmessage = (event) => {  };
+worker.postMessage("Here is your port", [channel.port2]);
+
+</script>
+</body>
+</html>
index 98eef7a..5e79840 100644 (file)
@@ -1,3 +1,25 @@
+2018-03-15  Youenn Fablet  <youenn@apple.com>
+
+        MessagePort is not always destroyed on the right thread
+        https://bugs.webkit.org/show_bug.cgi?id=183619
+        <rdar://problem/38204711>
+
+        Reviewed by Chris Dumez.
+
+        Add assertion to ensure MessagePort is destroyed in the right thread.
+        Modify methods taking a ref in a lambda to rely on weak pointers and refing the WorkerThread if in a worker context.
+        It is safe to ref the WorkerThread since it is thread safe ref counted and we are passing the ref to the main thread
+        where the WorkerThread is expected to be destroyed.
+
+        Test: http/tests/workers/worker-messageport-2.html
+
+        * dom/MessagePort.cpp:
+        (WebCore::MessagePort::~MessagePort):
+        (WebCore::MessagePort::dispatchMessages):
+        (WebCore::MessagePort::updateActivity):
+        (WebCore::MessagePort::hasPendingActivity const):
+        * dom/MessagePort.h:
+
 2018-03-15  Jer Noble  <jer.noble@apple.com>
 
         Adopt new AVURLAssetUseClientURLLoadingExclusively AVURLAsset creation option.
index 08f7572..23b9ae4 100644 (file)
@@ -34,6 +34,7 @@
 #include "MessagePortChannelProvider.h"
 #include "MessageWithMessagePorts.h"
 #include "WorkerGlobalScope.h"
+#include "WorkerThread.h"
 
 namespace WebCore {
 
@@ -108,6 +109,8 @@ MessagePort::MessagePort(ScriptExecutionContext& scriptExecutionContext, const M
 
 MessagePort::~MessagePort()
 {
+    ASSERT(m_creationThread.ptr() == &Thread::current());
+
     LOG(MessagePorts, "Destroyed MessagePort %s (%p) in process %" PRIu64, m_identifier.logString().utf8().data(), this, Process::identifier().toUInt64());
 
     ASSERT(allMessagePortsLock().isLocked());
@@ -243,8 +246,16 @@ void MessagePort::dispatchMessages()
     if (!isEntangled())
         return;
 
-    auto messagesTakenHandler = [this, protectedThis = makeRef(*this)](Vector<MessageWithMessagePorts>&& messages, Function<void()>&& completionCallback) mutable {
-        auto innerHandler = [this, otherProtectedThis = WTFMove(protectedThis)](Vector<MessageWithMessagePorts>&& messages) {
+    RefPtr<WorkerThread> workerThread;
+    if (is<WorkerGlobalScope>(*m_scriptExecutionContext))
+        workerThread = &downcast<WorkerGlobalScope>(*m_scriptExecutionContext).thread();
+
+    auto messagesTakenHandler = [this, weakThis = makeWeakPtr(this), workerThread = WTFMove(workerThread)](Vector<MessageWithMessagePorts>&& messages, Function<void()>&& completionCallback) mutable {
+        ASSERT(isMainThread());
+        auto innerHandler = [this, weakThis = WTFMove(weakThis)](auto&& messages) {
+            if (!weakThis)
+                return;
+
             LOG(MessagePorts, "MessagePort %s (%p) dispatching %zu messages", m_identifier.logString().utf8().data(), this, messages.size());
 
             if (!m_scriptExecutionContext)
@@ -265,26 +276,36 @@ void MessagePort::dispatchMessages()
             }
         };
 
-        if (!m_scriptExecutionContext)
-            return;
-
-        if (m_scriptExecutionContext->isContextThread()) {
+        if (!workerThread) {
             innerHandler(WTFMove(messages));
             completionCallback();
             return;
         }
-
-        m_scriptExecutionContext->postTask([innerHandler = WTFMove(innerHandler), messages = WTFMove(messages), completionCallback = WTFMove(completionCallback)](ScriptExecutionContext&) mutable {
+        workerThread->runLoop().postTaskForMode([innerHandler = WTFMove(innerHandler), messages = WTFMove(messages), completionCallback = WTFMove(completionCallback)](auto&) mutable {
             innerHandler(WTFMove(messages));
             RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] {
                 completionCallback();
             });
-        });
+        }, WorkerRunLoop::defaultMode());
     };
 
     MessagePortChannelProvider::singleton().takeAllMessagesForPort(m_identifier, WTFMove(messagesTakenHandler));
 }
 
+void MessagePort::updateActivity(MessagePortChannelProvider::HasActivity hasActivity)
+{
+    bool hasHadLocalActivity = m_hasHadLocalActivitySinceLastCheck;
+    m_hasHadLocalActivitySinceLastCheck = false;
+
+    if (hasActivity == MessagePortChannelProvider::HasActivity::No && !hasHadLocalActivity)
+        m_isRemoteEligibleForGC = true;
+
+    if (hasActivity == MessagePortChannelProvider::HasActivity::Yes)
+        m_isRemoteEligibleForGC = false;
+
+    m_isAskingRemoteAboutGC = false;
+}
+
 bool MessagePort::hasPendingActivity() const
 {
     m_mightBeEligibleForGC = true;
@@ -303,32 +324,23 @@ bool MessagePort::hasPendingActivity() const
 
     // If we're not in the middle of asking the remote port about collectability, do so now.
     if (!m_isAskingRemoteAboutGC) {
-        MessagePortChannelProvider::singleton().checkRemotePortForActivity(m_remoteIdentifier, [this, protectedThis = makeRef(*this)](MessagePortChannelProvider::HasActivity hasActivity) mutable {
-            auto innerHandler = [this, otherProtectedThis = WTFMove(protectedThis)](MessagePortChannelProvider::HasActivity hasActivity) {
-                bool hasHadLocalActivity = m_hasHadLocalActivitySinceLastCheck;
-                m_hasHadLocalActivitySinceLastCheck = false;
-
-                if (hasActivity == MessagePortChannelProvider::HasActivity::No && !hasHadLocalActivity)
-                    m_isRemoteEligibleForGC = true;
-
-                if (hasActivity == MessagePortChannelProvider::HasActivity::Yes)
-                    m_isRemoteEligibleForGC = false;
+        RefPtr<WorkerThread> workerThread;
+        if (is<WorkerGlobalScope>(*m_scriptExecutionContext))
+            workerThread = &downcast<WorkerGlobalScope>(*m_scriptExecutionContext).thread();
 
-                m_isAskingRemoteAboutGC = false;
-            };
+        MessagePortChannelProvider::singleton().checkRemotePortForActivity(m_remoteIdentifier, [weakThis = makeWeakPtr(const_cast<MessagePort*>(this)), workerThread = WTFMove(workerThread)](MessagePortChannelProvider::HasActivity hasActivity) mutable {
 
-
-            if (!m_scriptExecutionContext)
-                return;
-
-            if (m_scriptExecutionContext->isContextThread()) {
-                innerHandler(hasActivity);
+            ASSERT(isMainThread());
+            if (!workerThread) {
+                if (weakThis)
+                    weakThis->updateActivity(hasActivity);
                 return;
             }
 
-            m_scriptExecutionContext->postTask([innerHandler = WTFMove(innerHandler), hasActivity](ScriptExecutionContext&) mutable {
-                innerHandler(hasActivity);
-            });
+            workerThread->runLoop().postTaskForMode([weakThis = WTFMove(weakThis), hasActivity](auto&) mutable {
+                if (weakThis)
+                    weakThis->updateActivity(hasActivity);
+            }, WorkerRunLoop::defaultMode());
         });
         m_isAskingRemoteAboutGC = true;
     }
index 3588f94..8514195 100644 (file)
@@ -32,6 +32,8 @@
 #include "MessagePortChannel.h"
 #include "MessagePortIdentifier.h"
 #include "MessageWithMessagePorts.h"
+#include <wtf/Threading.h>
+#include <wtf/WeakPtr.h>
 
 namespace JSC {
 class ExecState;
@@ -48,6 +50,8 @@ public:
     static Ref<MessagePort> create(ScriptExecutionContext&, const MessagePortIdentifier& local, const MessagePortIdentifier& remote);
     virtual ~MessagePort();
 
+    auto& weakPtrFactory() const { return m_weakFactory; }
+
     ExceptionOr<void> postMessage(JSC::ExecState&, JSC::JSValue message, Vector<JSC::Strong<JSC::JSObject>>&&);
 
     void start();
@@ -106,10 +110,14 @@ private:
     // A port starts out its life entangled, and remains entangled until it is closed or is cloned.
     bool isEntangled() const { return !m_closed && m_entangled; }
 
+    void updateActivity(MessagePortChannelProvider::HasActivity);
+
     bool m_started { false };
     bool m_closed { false };
     bool m_entangled { true };
 
+    WeakPtrFactory<MessagePort> m_weakFactory;
+
     // Flags to manage querying the remote port for GC purposes
     mutable bool m_mightBeEligibleForGC { false };
     mutable bool m_hasHadLocalActivitySinceLastCheck { false };
@@ -121,6 +129,10 @@ private:
     MessagePortIdentifier m_remoteIdentifier;
 
     mutable std::atomic<unsigned> m_refCount { 1 };
+
+#if !ASSERT_DISABLED
+    Ref<Thread> m_creationThread { Thread::current() };
+#endif
 };
 
 } // namespace WebCore