MessagePort is unexpectedly GC'ed after activity absence

https://bugs.webkit.org/show_bug.cgi?id=193184 rdar://54095480 Reviewed by Ryosuke Niwa and Geoffrey Garen. Per the HTML specification [1], we should keep the MessagePort alive as long as it is entangled. This makes sense since the port could receive messages as long as it is entangled. However, we were missing this check inside MessagePort::virtualHasPendingActivity(). We do still allow collection when entangled if there is no message event handler, since it wouldn't be observable then. Even if a new message would arrive, there would be no-one on JS side to notify. [1] https://html.spec.whatwg.org/multipage/web-messaging.html#ports-and-garbage-collection * LayoutTests/http/tests/messaging/messageport-gc-after-xhr-expected.txt: Added. * LayoutTests/http/tests/messaging/messageport-gc-after-xhr.html: Added. * LayoutTests/http/tests/messaging/resources/messageport-pong-after-xhr.html: Added. Add test coverage. The test is based on the one provided by Yoshiaki Jitsukawa from Sony. * Source/WebCore/dom/MessagePort.cpp: (WebCore::MessagePort::virtualHasPendingActivity const): Canonical link: https://commits.webkit.org/256342@main
WebKit · Nov 4, 2022 · a4790ba · a4790ba
1 parent 84001ea
commit a4790ba
Show file tree

Hide file tree

Showing 14 changed files with 224 additions and 4 deletions.
diff --git a/LayoutTests/fast/events/message-port-gc-after-closing-expected.txt b/LayoutTests/fast/events/message-port-gc-after-closing-expected.txt
@@ -0,0 +1,11 @@
+Checks that MessagePort objects get GC'd after closing them
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Closing ports...
+PASS At least one port got GC'd after closing
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/events/message-port-gc-after-closing.html b/LayoutTests/fast/events/message-port-gc-after-closing.html
@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+description("Checks that MessagePort objects get GC'd after closing them");
+jsTestIsAsync = true;
+
+const messagePortsToCreate = 50;
+let portIdentifiers = [];
+let ports = [];
+let worker = new Worker("resources/message-port-gc-worker.js");
+for (let i = 0; i < messagePortsToCreate; ++i) {
+    let { port1, port2 } = new MessageChannel();
+    worker.postMessage("port", [port2]);
+    port1.onmessage = (event) => {
+        testFailed("Received unexpected message on MessagePort");
+    };
+    portIdentifiers.push(internals.messagePortIdentifier(port1));
+    ports.push(port1);
+}
+
+function checkSomePortsGotGCd()
+{
+    gc();
+    for (let portIdentifier of portIdentifiers) {
+        if (!internals.isMessagePortAlive(portIdentifier)) {
+            testPassed("At least one port got GC'd after closing");
+            finishJSTest();
+            clearInterval(intervalHandle);
+            return;
+        }
+    }
+}
+
+gc();
+setTimeout(() => {
+    gc();
+    for (let portIdentifier of portIdentifiers) {
+        if (!internals.isMessagePortAlive(portIdentifier)) {
+            testFailed("MessagePort was GC'd too eagerly");
+            finishJSTest();
+            return;
+        }
+    }
+    debug("Closing ports...");
+    for (let port of ports)
+        port.close();
+
+    ports = [];    
+
+    gc();
+    intervalHandle = setInterval(checkSomePortsGotGCd, 100);
+}, 0);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/events/message-port-gc-after-removing-event-listener-expected.txt b/LayoutTests/fast/events/message-port-gc-after-removing-event-listener-expected.txt
@@ -0,0 +1,11 @@
+Checks that MessagePort objects get GC'd once they no longer have an event listener
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Removing event listeners on ports...
+PASS At least one port got GC'd after removing their event listener
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/events/message-port-gc-after-removing-event-listener.html b/LayoutTests/fast/events/message-port-gc-after-removing-event-listener.html
@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+description("Checks that MessagePort objects get GC'd once they no longer have an event listener");
+jsTestIsAsync = true;
+
+const messagePortsToCreate = 50;
+let portIdentifiers = [];
+let ports = [];
+let worker = new Worker("resources/message-port-gc-worker.js");
+for (let i = 0; i < messagePortsToCreate; ++i) {
+    let { port1, port2 } = new MessageChannel();
+    worker.postMessage("port", [port2]);
+    port1.onmessage = (event) => {
+        testFailed("Received unexpected message on MessagePort");
+    };
+    portIdentifiers.push(internals.messagePortIdentifier(port1));
+    ports.push(port1);
+}
+
+function checkSomePortsGotGCd()
+{
+    gc();
+    for (let portIdentifier of portIdentifiers) {
+        if (!internals.isMessagePortAlive(portIdentifier)) {
+            testPassed("At least one port got GC'd after removing their event listener");
+            finishJSTest();
+            clearInterval(intervalHandle);
+            return;
+        }
+    }
+}
+
+gc();
+setTimeout(() => {
+    gc();
+    for (let portIdentifier of portIdentifiers) {
+        if (!internals.isMessagePortAlive(portIdentifier)) {
+            testFailed("MessagePort was GC'd too eagerly");
+            finishJSTest();
+            return;
+        }
+    }
+    debug("Removing event listeners on ports...");
+    for (let port of ports)
+        port.onmessage = null;
+
+    ports = [];    
+
+    gc();
+    intervalHandle = setInterval(checkSomePortsGotGCd, 100);
+}, 0);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/events/resources/message-port-gc-worker.js b/LayoutTests/fast/events/resources/message-port-gc-worker.js
@@ -0,0 +1,5 @@
+let ports = [];
+
+onmessage = (event) => {
+    ports.push(event.ports[0]);
+}
diff --git a/LayoutTests/http/tests/messaging/messageport-gc-after-xhr-expected.txt b/LayoutTests/http/tests/messaging/messageport-gc-after-xhr-expected.txt
@@ -0,0 +1,11 @@
+Tests that MessagePort doesn't get GC'd while still entangled.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Posting message to the iframe.
+PASS Received the message from the iframe. The MessagePort was not GC'd
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/messaging/messageport-gc-after-xhr.html b/LayoutTests/http/tests/messaging/messageport-gc-after-xhr.html
@@ -0,0 +1,27 @@
+<html>
+<body>
+<script src="/js-test-resources/js-test.js"></script>
+<iframe id="testFrame" src="resources/messageport-pong-after-xhr.html" style="display:none"></iframe>
+<script>
+description("Tests that MessagePort doesn't get GC'd while still entangled.");
+jsTestIsAsync = true;
+
+window.onload = () => {
+    let { port1, port2 } = new MessageChannel();
+    debug("Posting message to the iframe.");
+    document.getElementById('testFrame').contentWindow.postMessage("ping", "*", [port2]);
+    port1.onmessage = (e) => {
+        testPassed("Received the message from the iframe. The MessagePort was not GC'd");
+        finishJSTest();
+    };
+
+    setInterval(gc, 100);
+
+    setTimeout(() => {
+        testFailed("Did not receive the message from the iframe. The MessagePort was probably GC'd.");
+        finishJSTest();
+    }, 5000);
+};
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/messaging/resources/messageport-pong-after-xhr.html b/LayoutTests/http/tests/messaging/resources/messageport-pong-after-xhr.html
@@ -0,0 +1,11 @@
+<script>
+onmessage = (e) => {
+    let port = e.ports[0];
+    let xhr = new XMLHttpRequest();
+    xhr.open("GET", "/incremental/resources/delayed-css.py?delay=300");
+    xhr.onload = () => {
+        port.postMessage("pong");
+    };
+    xhr.send();
+}
+</script>
diff --git a/Source/WebCore/dom/MessagePort.cpp b/Source/WebCore/dom/MessagePort.cpp
@@ -92,6 +92,12 @@ bool MessagePort::isExistingMessagePortLocallyReachable(const MessagePortIdentif
     return port && port->isLocallyReachable();
 }
 
+bool MessagePort::isMessagePortAliveForTesting(const MessagePortIdentifier& identifier)
+{
+    Locker locker { allMessagePortsLock };
+    return allMessagePorts().contains(identifier);
+}
+
 void MessagePort::notifyMessageAvailable(const MessagePortIdentifier& identifier)
 {
     ASSERT(isMainThread());
@@ -350,14 +356,17 @@ bool MessagePort::virtualHasPendingActivity() const
     if (!context || m_isDetached)
         return false;
 
-    // If this object has been idle since the remote port declared itself elgibile for GC, we can GC.
-    if (!m_hasHadLocalActivitySinceLastCheck && m_isRemoteEligibleForGC)
-        return false;
-
     // If this MessagePort has no message event handler then the existence of remote activity cannot keep it alive.
     if (!m_hasMessageEventListener)
         return false;
 
+    if (m_entangled)
+        return true;
+
+    // If this object has been idle since the remote port declared itself eligible for GC, we can GC.
+    if (!m_hasHadLocalActivitySinceLastCheck && m_isRemoteEligibleForGC)
+        return false;
+
     // If we're not in the middle of asking the remote port about collectability, do so now.
     if (!m_isAskingRemoteAboutGC) {
         RefPtr<WorkerOrWorkletThread> workerOrWorkletThread;

diff --git a/Source/WebCore/dom/MessagePort.h b/Source/WebCore/dom/MessagePort.h
@@ -64,6 +64,7 @@ class MessagePort final : public ActiveDOMObject, public EventTarget {
     static ExceptionOr<Vector<TransferredMessagePort>> disentanglePorts(Vector<RefPtr<MessagePort>>&&);
     static Vector<RefPtr<MessagePort>> entanglePorts(ScriptExecutionContext&, Vector<TransferredMessagePort>&&);
 
+    WEBCORE_EXPORT static bool isMessagePortAliveForTesting(const MessagePortIdentifier&);
     WEBCORE_EXPORT static bool isExistingMessagePortLocallyReachable(const MessagePortIdentifier&);
     WEBCORE_EXPORT static void notifyMessageAvailable(const MessagePortIdentifier&);
 

diff --git a/Source/WebCore/dom/MessagePort.idl b/Source/WebCore/dom/MessagePort.idl
@@ -27,6 +27,7 @@
 
 [
     ActiveDOMObject,
+    ExportMacro=WEBCORE_EXPORT,
     Exposed=(Window,Worker,AudioWorklet),
     GenerateIsReachable=Impl,
     JSCustomMarkFunction,

diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
@@ -148,6 +148,7 @@
 #include "MediaUsageInfo.h"
 #include "MemoryCache.h"
 #include "MemoryInfo.h"
+#include "MessagePort.h"
 #include "MockAudioDestinationCocoa.h"
 #include "MockLibWebRTCPeerConnection.h"
 #include "MockPageOverlay.h"
@@ -2861,6 +2862,17 @@ bool Internals::isDocumentAlive(const String& documentIdentifier) const
     return uuid ? Document::allDocumentsMap().contains({ *uuid, Process::identifier() }) : false;
 }
 
+uint64_t Internals::messagePortIdentifier(const MessagePort& port) const
+{
+    return port.identifier().portIdentifier.toUInt64();
+}
+
+bool Internals::isMessagePortAlive(uint64_t messagePortIdentifier) const
+{
+    MessagePortIdentifier portIdentifier { Process::identifier(), makeObjectIdentifier<MessagePortIdentifier::PortIdentifierType>(messagePortIdentifier) };
+    return MessagePort::isMessagePortAliveForTesting(portIdentifier);
+}
+
 uint64_t Internals::storageAreaMapCount() const
 {
     auto* page = contextDocument() ? contextDocument()->page() : nullptr;

diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
@@ -105,6 +105,7 @@ class MallocStatistics;
 class MediaStream;
 class MediaStreamTrack;
 class MemoryInfo;
+class MessagePort;
 class MockCDMFactory;
 class MockContentFilterSettings;
 class MockPageOverlay;
@@ -532,6 +533,9 @@ class Internals final : public RefCounted<Internals>, private ContextDestruction
     String documentIdentifier(const Document&) const;
     bool isDocumentAlive(const String& documentIdentifier) const;
 
+    uint64_t messagePortIdentifier(const MessagePort&) const;
+    bool isMessagePortAlive(uint64_t messagePortIdentifier) const;
+
     uint64_t storageAreaMapCount() const;
 
     uint64_t elementIdentifier(Element&) const;

diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
@@ -977,6 +977,9 @@ typedef (FetchRequest or FetchResponse) FetchObject;
     DOMString documentIdentifier(Document document);
     boolean isDocumentAlive(DOMString documentIdentifier);
 
+    unsigned long long messagePortIdentifier(MessagePort port);
+    boolean isMessagePortAlive(unsigned long long messagePortIdentifier);
+
     readonly attribute unsigned long long storageAreaMapCount;
 
     unsigned long long elementIdentifier(Element element);