Changeset 229554 in webkit

Timestamp:

Mar 12, 2018 2:59:32 PM (6 years ago)

Author:

jmarcell@apple.com

Message:

Cherry-pick r229028. rdar://problem/37992284

Location:

branches/safari-605-branch/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/MessagePort.cpp (modified) (2 diffs)
• WebCore/dom/MessagePort.h (modified) (1 diff)
• WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp (modified) (2 diffs)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp (modified) (1 diff)
• WebKit/WebProcess/WebProcess.cpp (modified) (1 diff)

branches/safari-605-branch/Source/WebCore/ChangeLog

@@ +1 @@
+2018-03-11  Jason Marcell  <jmarcell@apple.com>
+
+        Cherry-pick r229028. rdar://problem/37992284
+
+    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-03-11  Jason Marcell  <jmarcell@apple.com>
 

branches/safari-605-branch/Source/WebCore/dom/MessagePort.cpp

@@ -57 +57 @@
 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) {
@@ -75 +71 @@
 }
 
-RefPtr<MessagePort> MessagePort::existingMessagePortForIdentifier(const MessagePortIdentifier& identifier)
+bool MessagePort::isExistingMessagePortLocallyReachable(const MessagePortIdentifier& identifier)
 {
     Locker<Lock> locker(allMessagePortsLock());
-
-    return allMessagePorts().get(identifier);
+    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();
+
 }
 

branches/safari-605-branch/Source/WebCore/dom/MessagePort.h

@@ -58 +58 @@
     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();

branches/safari-605-branch/Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp

@@ -83 +83 @@
 {
     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);
     });
 }
@@ -120 +117 @@
     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);
 }
 

branches/safari-605-branch/Source/WebKit/ChangeLog

@@ +1 @@
+2018-03-11  Jason Marcell  <jmarcell@apple.com>
+
+        Cherry-pick r229028. rdar://problem/37992284
+
+    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-26  Jason Marcell  <jmarcell@apple.com>
 

branches/safari-605-branch/Source/WebKit/WebProcess/WebCoreSupport/WebMessagePortChannelProvider.cpp

@@ -121 +121 @@
 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);
 }
 

branches/safari-605-branch/Source/WebKit/WebProcess/WebProcess.cpp

@@ -1071 +1071 @@
 void WebProcess::messagesAvailableForPort(const MessagePortIdentifier& identifier)
 {
-    auto port = MessagePort::existingMessagePortForIdentifier(identifier);
-    if (port)
-        port->messageAvailable();
+    MessagePort::notifyMessageAvailable(identifier);
 }