MessagePort is not always destroyed in the right thread

https://bugs.webkit.org/show_bug.cgi?id=183053

Patch by Youenn Fablet <youenn@apple.com> on 2018-02-26
Reviewed by Chris Dumez.

Source/WebCore:

Make existingMessagePortForIdentifier take a lambda so that we hold the lock until there
is no longer a need to keep the MessagePort around.
This is very time sensitive and does not happen a lot when running WPT tests.

Update existing call sites to pass a lambda.

* dom/MessagePort.cpp:
(WebCore::MessagePort::existingMessagePortForIdentifier):
* dom/MessagePort.h:
* dom/messageports/MessagePortChannelProviderImpl.cpp:
(WebCore::MessagePortChannelProviderImpl::postMessageToRemote):
(WebCore::MessagePortChannelProviderImpl::checkProcessLocalPortForActivity):

Source/WebKit:

Update code to pass a lambda to MessagePort::existingMessagePortForIdentifier.

* WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
(WebKit::WebMessagePortChannelProvider::checkProcessLocalPortForActivity):
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::messagesAvailableForPort):

Canonical link: https://commits.webkit.org/198848@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229028 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,23 @@
+2018-02-26  Youenn Fablet  <youenn@apple.com>
+
+        MessagePort is not always destroyed in the right thread
+        https://bugs.webkit.org/show_bug.cgi?id=183053
+
+        Reviewed by Chris Dumez.
+
+        Make existingMessagePortForIdentifier take a lambda so that we hold the lock until there
+        is no longer a need to keep the MessagePort around.
+        This is very time sensitive and does not happen a lot when running WPT tests.
+
+        Update existing call sites to pass a lambda.
+
+        * dom/MessagePort.cpp:
+        (WebCore::MessagePort::existingMessagePortForIdentifier):
+        * dom/MessagePort.h:
+        * dom/messageports/MessagePortChannelProviderImpl.cpp:
+        (WebCore::MessagePortChannelProviderImpl::postMessageToRemote):
+        (WebCore::MessagePortChannelProviderImpl::checkProcessLocalPortForActivity):
+
 2018-02-26  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r226745.

Source/WebCore/dom/MessagePort.cpp

@@ -56,12 +56,8 @@ void MessagePort::ref() const
 
 void MessagePort::deref() const
 {
-    // MessagePort::existingMessagePortForIdentifier() is unique in that it holds a raw pointer to a MessagePort
-    // but might create a RefPtr from it.
-    // If that happens on one thread at the same time that a MessagePort is being deref'ed and destroyed on a
-    // different thread then Bad Things could happen.
-    // This custom deref() function is designed to handle that contention by guaranteeing that nobody can be
-    // creating a RefPtr inside existingMessagePortForIdentifier while the object is mid-deletion.
+    // This custom deref() function ensures that as long as the lock to allMessagePortsLock is taken, no MessagePort will be destroyed.
+    // This allows isExistingMessagePortLocallyReachable and notifyMessageAvailable to easily query the map and manipulate MessagePort instances.
 
     if (!--m_refCount) {
         Locker<Lock> locker(allMessagePortsLock());
@@ -74,11 +70,19 @@ void MessagePort::deref() const
     }
 }
 
-RefPtr<MessagePort> MessagePort::existingMessagePortForIdentifier(const MessagePortIdentifier& identifier)
+bool MessagePort::isExistingMessagePortLocallyReachable(const MessagePortIdentifier& identifier)
 {
     Locker<Lock> locker(allMessagePortsLock());
+    auto* port = allMessagePorts().get(identifier);
+    return port && port->isLocallyReachable();
+}
+
+void MessagePort::notifyMessageAvailable(const MessagePortIdentifier& identifier)
+{
+    Locker<Lock> locker(allMessagePortsLock());
+    if (auto* port = allMessagePorts().get(identifier))
+        port->messageAvailable();
 
-    return allMessagePorts().get(identifier);
 }
 
 Ref<MessagePort> MessagePort::create(ScriptExecutionContext& scriptExecutionContext, const MessagePortIdentifier& local, const MessagePortIdentifier& remote)

Source/WebCore/dom/MessagePort.h

@@ -57,7 +57,9 @@ class MessagePort final : public ActiveDOMObject, public EventTargetWithInlineDa
     // Returns nullptr if the passed-in vector is empty.
     static ExceptionOr<TransferredMessagePortArray> disentanglePorts(Vector<RefPtr<MessagePort>>&&);
     static Vector<RefPtr<MessagePort>> entanglePorts(ScriptExecutionContext&, TransferredMessagePortArray&&);
-    WEBCORE_EXPORT static RefPtr<MessagePort> existingMessagePortForIdentifier(const MessagePortIdentifier&);
+
+    WEBCORE_EXPORT static bool isExistingMessagePortLocallyReachable(const MessagePortIdentifier&);
+    WEBCORE_EXPORT static void notifyMessageAvailable(const MessagePortIdentifier&);
 
     WEBCORE_EXPORT void messageAvailable();
     bool started() const { return m_started; }

Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp

@@ -82,11 +82,8 @@ void MessagePortChannelProviderImpl::messagePortClosed(const MessagePortIdentifi
 void MessagePortChannelProviderImpl::postMessageToRemote(MessageWithMessagePorts&& message, const MessagePortIdentifier& remoteTarget)
 {
     performActionOnMainThread([registry = &m_registry, message = WTFMove(message), remoteTarget]() mutable {
-        bool wasFirstMessageInQueue = registry->didPostMessageToRemote(WTFMove(message), remoteTarget);
-        if (wasFirstMessageInQueue) {
-            if (auto remotePort = MessagePort::existingMessagePortForIdentifier(remoteTarget))
-                remotePort->messageAvailable();
-        }
+        if (registry->didPostMessageToRemote(WTFMove(message), remoteTarget))
+            MessagePort::notifyMessageAvailable(remoteTarget);
     });
 }
 
@@ -119,13 +116,7 @@ void MessagePortChannelProviderImpl::checkProcessLocalPortForActivity(const Mess
 {
     ASSERT(isMainThread());
 
-    auto port = MessagePort::existingMessagePortForIdentifier(identifier);
-    if (!port) {
-        callback(MessagePortChannelProvider::HasActivity::No);
-        return;
-    }
-
-    callback(port->isLocallyReachable() ? HasActivity::Yes : HasActivity::No);
+    callback(MessagePort::isExistingMessagePortLocallyReachable(identifier) ? HasActivity::Yes : HasActivity::No);
 }
 
 

Source/WebKit/ChangeLog

@@ -1,3 +1,17 @@
+2018-02-26  Youenn Fablet  <youenn@apple.com>
+
+        MessagePort is not always destroyed in the right thread
+        https://bugs.webkit.org/show_bug.cgi?id=183053
+
+        Reviewed by Chris Dumez.
+
+        Update code to pass a lambda to MessagePort::existingMessagePortForIdentifier.
+
+        * WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp:
+        (WebKit::WebMessagePortChannelProvider::checkProcessLocalPortForActivity):
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::messagesAvailableForPort):
+
 2018-02-25  Alexey Proskuryakov  <ap@apple.com>
 
         Font smoothing doesn't get disabled if the preference is set before launching WebContent process

Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp

@@ -120,8 +120,7 @@ void WebMessagePortChannelProvider::postMessageToRemote(MessageWithMessagePorts&
 
 void WebMessagePortChannelProvider::checkProcessLocalPortForActivity(const MessagePortIdentifier& identifier, uint64_t callbackIdentifier)
 {
-    auto port = MessagePort::existingMessagePortForIdentifier(identifier);
-    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::DidCheckProcessLocalPortForActivity(callbackIdentifier, port && port->isLocallyReachable()), 0);
+    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::DidCheckProcessLocalPortForActivity { callbackIdentifier, MessagePort::isExistingMessagePortLocallyReachable(identifier) }, 0);
 }
 
 void WebMessagePortChannelProvider::checkProcessLocalPortForActivity(const MessagePortIdentifier&, ProcessIdentifier, CompletionHandler<void(HasActivity)>&&)

Source/WebKit/WebProcess/WebProcess.cpp

@@ -1060,9 +1060,7 @@ void WebProcess::didCheckRemotePortForActivity(uint64_t callbackIdentifier, bool
 
 void WebProcess::messagesAvailableForPort(const MessagePortIdentifier& identifier)
 {
-    auto port = MessagePort::existingMessagePortForIdentifier(identifier);
-    if (port)
-        port->messageAvailable();
+    MessagePort::notifyMessageAvailable(identifier);
 }
 
 #if ENABLE(GAMEPAD)