Retain MessagePortChannel for transfer when disentangling ports
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 23:38:46 +0000 (23:38 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 23:38:46 +0000 (23:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184502
<rdar://problem/39372771>

Patch by Tadeu Zagallo <tzagallo@apple.com> on 2018-04-17
Reviewed by Geoffrey Garen.

Source/WebCore:

MessagePortChannels should be retained while ports are being transferred, but that was only
happening when sending a port through another port, but not when sending it through a worker.

Test: workers/worker-to-worker.html

* dom/messageports/MessagePortChannel.cpp:
(WebCore::MessagePortChannel::entanglePortWithProcess):
(WebCore::MessagePortChannel::disentanglePort):
(WebCore::MessagePortChannel::postMessageToRemote):
(WebCore::MessagePortChannel::takeAllMessagesForPort):

LayoutTests:

Check that the MessageChannel does not get eagerly deallocated when transferring both of its
ports. Original test case provided with the bug report by Ashley Gullen <ashley@scirra.com>

* workers/worker-to-worker-expected.txt: Added.
* workers/worker-to-worker.html: Added.
* workers/worker-to-worker.js: Added.

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

LayoutTests/ChangeLog
LayoutTests/workers/worker-to-worker-expected.txt [new file with mode: 0644]
LayoutTests/workers/worker-to-worker.html [new file with mode: 0644]
LayoutTests/workers/worker-to-worker.js [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/messageports/MessagePortChannel.cpp

index 5405bcd..5fd683c 100644 (file)
@@ -1,3 +1,18 @@
+2018-04-17  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Retain MessagePortChannel for transfer when disentangling ports
+        https://bugs.webkit.org/show_bug.cgi?id=184502
+        <rdar://problem/39372771>
+
+        Reviewed by Geoffrey Garen.
+
+        Check that the MessageChannel does not get eagerly deallocated when transferring both of its
+        ports. Original test case provided with the bug report by Ashley Gullen <ashley@scirra.com>
+
+        * workers/worker-to-worker-expected.txt: Added.
+        * workers/worker-to-worker.html: Added.
+        * workers/worker-to-worker.js: Added.
+
 2018-04-17  Jonathan Bedard  <jbedard@apple.com>
 
         Unreviewed rollout of r230632. Regression in memory usage.
diff --git a/LayoutTests/workers/worker-to-worker-expected.txt b/LayoutTests/workers/worker-to-worker-expected.txt
new file mode 100644 (file)
index 0000000..894f546
--- /dev/null
@@ -0,0 +1,11 @@
+Worker to worker communication via MessagePorts
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+[Worker 1] Received message: hello
+[Worker 2] Received message: hello
+
diff --git a/LayoutTests/workers/worker-to-worker.html b/LayoutTests/workers/worker-to-worker.html
new file mode 100644 (file)
index 0000000..6fd2558
--- /dev/null
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../resources/js-test.js"></script>
+<script>
+description("Worker to worker communication via MessagePorts");
+
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+
+const worker = (name, port, wait) => new Promise(resolve => {
+  const w = new Worker("worker-to-worker.js");
+  w.postMessage({ name, wait, port }, [port]);
+  w.onmessage = (e) => {
+    debug(e.data);
+    resolve();
+  };
+});
+
+const mc = new MessageChannel();
+Promise.all([
+  // force one worker to wait to make order deterministic
+  worker("Worker 1", mc.port1, 1),
+  worker("Worker 2", mc.port2),
+]).then(() => {
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+});
+
+</script>
+</body>
+</html>
+
diff --git a/LayoutTests/workers/worker-to-worker.js b/LayoutTests/workers/worker-to-worker.js
new file mode 100644 (file)
index 0000000..7534f47
--- /dev/null
@@ -0,0 +1,11 @@
+self.onmessage = e => {
+  const {name} = e.data;
+
+  e.data.port.onmessage = e => {
+    self.postMessage(`[${name}] Received message: ${e.data}`);
+  }
+
+  const sendMsg = () => e.data.port.postMessage('hello');
+  if (e.data.wait) setTimeout(sendMsg, e.data.wait);
+  else sendMsg();
+};
index a508bbf..d18975d 100644 (file)
@@ -1,5 +1,24 @@
 2018-04-17  Tadeu Zagallo  <tzagallo@apple.com>
 
+        Retain MessagePortChannel for transfer when disentangling ports
+        https://bugs.webkit.org/show_bug.cgi?id=184502
+        <rdar://problem/39372771>
+
+        Reviewed by Geoffrey Garen.
+
+        MessagePortChannels should be retained while ports are being transferred, but that was only
+        happening when sending a port through another port, but not when sending it through a worker.
+
+        Test: workers/worker-to-worker.html
+
+        * dom/messageports/MessagePortChannel.cpp:
+        (WebCore::MessagePortChannel::entanglePortWithProcess):
+        (WebCore::MessagePortChannel::disentanglePort):
+        (WebCore::MessagePortChannel::postMessageToRemote):
+        (WebCore::MessagePortChannel::takeAllMessagesForPort):
+
+2018-04-17  Tadeu Zagallo  <tzagallo@apple.com>
+
         References from CSSStyleDeclaration to CSSValues should be weak
         https://bugs.webkit.org/show_bug.cgi?id=180280
         <rdar://problem/35804869>
index d7506f0..83e1961 100644 (file)
@@ -87,6 +87,7 @@ void MessagePortChannel::entanglePortWithProcess(const MessagePortIdentifier& po
     ASSERT(!m_processes[i] || *m_processes[i] == process);
     m_processes[i] = process;
     m_entangledToProcessProtectors[i] = this;
+    m_pendingMessagePortTransfers[i].remove(this);
 }
 
 void MessagePortChannel::disentanglePort(const MessagePortIdentifier& port)
@@ -100,6 +101,7 @@ void MessagePortChannel::disentanglePort(const MessagePortIdentifier& port)
 
     ASSERT(m_processes[i] || m_isClosed[i]);
     m_processes[i] = std::nullopt;
+    m_pendingMessagePortTransfers[i].add(this);
 
     // This set of steps is to guarantee that the lock is unlocked before the
     // last ref to this object is released.
@@ -133,24 +135,6 @@ bool MessagePortChannel::postMessageToRemote(MessageWithMessagePorts&& message,
     ASSERT(remoteTarget == m_ports[0] || remoteTarget == m_ports[1]);
     size_t i = remoteTarget == m_ports[0] ? 0 : 1;
 
-    for (auto& channelPair : message.transferredPorts) {
-        auto* channel = m_registry.existingChannelContainingPort(channelPair.first);
-        // One of the ports in the channel might have been closed, therefore removing record of the channel.
-        // That's okay; such ports can still be transferred. We just don't have to protect the channel.
-        if (!channel)
-            continue;
-
-        ASSERT(channel->includesPort(channelPair.second));
-
-#ifndef NDEBUG
-        if (auto* otherChannel = m_registry.existingChannelContainingPort(channelPair.second))
-            ASSERT(channel == otherChannel);
-#endif
-        // Having a pending message should keep a port alive with a ref.
-        // The ref will be cleared after the batch of pending messages has been delivered.
-        m_pendingMessagePortTransfers[i].add(channel);
-    }
-
     m_pendingMessages[i].append(WTFMove(message));
     LOG(MessagePorts, "MessagePortChannel %s (%p) now has %zu messages pending on port %s", logString().utf8().data(), this, m_pendingMessages[i].size(), remoteTarget.logString().utf8().data());
 
@@ -187,10 +171,7 @@ void MessagePortChannel::takeAllMessagesForPort(const MessagePortIdentifier& por
     LOG(MessagePorts, "There are %zu messages to take for port %s. Taking them now, messages in flight is now %" PRIu64, result.size(), port.logString().utf8().data(), m_messageBatchesInFlight);
 
     auto size = result.size();
-    HashSet<RefPtr<MessagePortChannel>> transferredPortProtectors;
-    transferredPortProtectors.swap(m_pendingMessagePortTransfers[i]);
-
-    callback(WTFMove(result), [size, this, port, protectedThis = WTFMove(m_pendingMessageProtectors[i]), transferredPortProtectors = WTFMove(transferredPortProtectors)] {
+    callback(WTFMove(result), [size, this, port, protectedThis = WTFMove(m_pendingMessageProtectors[i])] {
         UNUSED_PARAM(port);
 #if LOG_DISABLED
         UNUSED_PARAM(size);