Changeset 271876 in webkit

Timestamp:

Jan 25, 2021, 9:09:16 PM (6 years ago)

Author:

Simon Fraser

Message:

Crash when remote inspecting in debug builds
​https://bugs.webkit.org/show_bug.cgi?id=220956
<rdar://73379637>

Reviewed by Devin Rousso.

Convert RemoteConnectionToTarget from using BlockPtr<> to Function<> because BlockPtr<>
was triggering crashes which seem to be related to mixing ARC and non-ARC code.

• inspector/remote/RemoteConnectionToTarget.h:
• inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:

(Inspector::RemoteTargetHandleRunSourceGlobal):
(Inspector::RemoteTargetQueueTaskOnGlobalQueue):
(Inspector::RemoteTargetHandleRunSourceWithInfo):
(Inspector::RemoteConnectionToTarget::dispatchAsyncOnTarget):
(Inspector::RemoteConnectionToTarget::setup):
(Inspector::RemoteConnectionToTarget::close):
(Inspector::RemoteConnectionToTarget::sendMessageToTarget):
(Inspector::RemoteConnectionToTarget::queueTaskOnPrivateRunLoop):
(Inspector::RemoteConnectionToTarget::takeQueue):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• inspector/remote/RemoteConnectionToTarget.h (modified) (3 diffs)
• inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm (modified) (9 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-01-25  Simon Fraser  <simon.fraser@apple.com>
+
+        Crash when remote inspecting in debug builds
+        https://bugs.webkit.org/show_bug.cgi?id=220956
+        <rdar://73379637>
+
+        Reviewed by Devin Rousso.
+
+        Convert RemoteConnectionToTarget from using BlockPtr<> to Function<> because BlockPtr<> 
+        was triggering crashes which seem to be related to mixing ARC and non-ARC code.
+
+        * inspector/remote/RemoteConnectionToTarget.h:
+        * inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:
+        (Inspector::RemoteTargetHandleRunSourceGlobal):
+        (Inspector::RemoteTargetQueueTaskOnGlobalQueue):
+        (Inspector::RemoteTargetHandleRunSourceWithInfo):
+        (Inspector::RemoteConnectionToTarget::dispatchAsyncOnTarget):
+        (Inspector::RemoteConnectionToTarget::setup):
+        (Inspector::RemoteConnectionToTarget::close):
+        (Inspector::RemoteConnectionToTarget::sendMessageToTarget):
+        (Inspector::RemoteConnectionToTarget::queueTaskOnPrivateRunLoop):
+        (Inspector::RemoteConnectionToTarget::takeQueue):
+
 2021-01-25  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.h

@@ -45 +45 @@
 
 #if PLATFORM(COCOA)
-typedef Vector<BlockPtr<void ()>> RemoteTargetQueue;
+typedef Vector<Function<void ()>> RemoteTargetQueue;
 #endif
 
@@ -74 +74 @@
     Lock& queueMutex() { return m_queueMutex; }
     const RemoteTargetQueue& queue() const { return m_queue; }
-    void clearQueue() { m_queue.clear(); }
+    RemoteTargetQueue takeQueue();
 #endif
 
@@ -83 +83 @@
 private:
 #if PLATFORM(COCOA)
-    void dispatchAsyncOnTarget(void (^block)());
+    void dispatchAsyncOnTarget(Function<void ()>&&);
 
     void setupRunLoop();
     void teardownRunLoop();
-    void queueTaskOnPrivateRunLoop(void (^block)());
+    void queueTaskOnPrivateRunLoop(Function<void ()>&&);
 #endif
 

trunk/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm

@@ -56 +56 @@
     {
         LockHolder lock(rwiQueueMutex);
-        queueCopy = *rwiQueue;
-        rwiQueue->clear();
-    }
-
-    for (const auto& block : queueCopy)
-        block();
-}
-
-static void RemoteTargetQueueTaskOnGlobalQueue(void (^task)())
+        std::swap(queueCopy, *rwiQueue);
+    }
+
+    for (const auto& function : queueCopy)
+        function();
+}
+
+static void RemoteTargetQueueTaskOnGlobalQueue(Function<void ()>&& function)
 {
     ASSERT(rwiRunLoopSource);
@@ -71 +70 @@
     {
         LockHolder lock(rwiQueueMutex);
-        rwiQueue->append(task);
+        rwiQueue->append(WTFMove(function));
     }
 
@@ -102 +101 @@
     {
         LockHolder lock(connectionToTarget->queueMutex());
-        queueCopy = connectionToTarget->queue();
-        connectionToTarget->clearQueue();
-    }
-
-    for (const auto& block : queueCopy)
-        block();
+        queueCopy = connectionToTarget->takeQueue();
+    }
+
+    for (const auto& function : queueCopy)
+        function();
 }
 
@@ -139 +137 @@
 }
 
-void RemoteConnectionToTarget::dispatchAsyncOnTarget(void (^block)())
+void RemoteConnectionToTarget::dispatchAsyncOnTarget(Function<void ()>&& callback)
 {
     if (m_runLoop) {
-        queueTaskOnPrivateRunLoop(block);
+        queueTaskOnPrivateRunLoop(WTFMove(callback));
         return;
     }
@@ -148 +146 @@
 #if USE(WEB_THREAD)
     if (WebCoreWebThreadIsEnabled && WebCoreWebThreadIsEnabled()) {
-        WebCoreWebThreadRun(block);
+        WebCoreWebThreadRun(^ { callback(); });
         return;
     }
 #endif
 
-    RemoteTargetQueueTaskOnGlobalQueue(block);
+    RemoteTargetQueueTaskOnGlobalQueue(WTFMove(callback));
 }
 
@@ -164 +162 @@
 
     auto targetIdentifier = this->targetIdentifier().valueOr(0);
+
+    dispatchAsyncOnTarget([&, strongThis = makeRef(*this)]() {
+        LockHolder lock(m_targetMutex);
+
+        if (!m_target || !m_target->remoteControlAllowed()) {
+            RemoteInspector::singleton().setupFailed(targetIdentifier);
+            m_target = nullptr;
+        } else if (is<RemoteInspectionTarget>(m_target)) {
+            auto castedTarget = downcast<RemoteInspectionTarget>(m_target);
+            castedTarget->connect(*this, isAutomaticInspection, automaticallyPause);
+            m_connected = true;
+
+            RemoteInspector::singleton().updateTargetListing(targetIdentifier);
+        } else if (is<RemoteAutomationTarget>(m_target)) {
+            auto castedTarget = downcast<RemoteAutomationTarget>(m_target);
+            castedTarget->connect(*this);
+            m_connected = true;
+
+            RemoteInspector::singleton().updateTargetListing(targetIdentifier);
+        }
+    });
+
+    return true;
+}
+
+void RemoteConnectionToTarget::targetClosed()
+{
+    LockHolder lock(m_targetMutex);
+
+    m_target = nullptr;
+}
+
+void RemoteConnectionToTarget::close()
+{
+    auto targetIdentifier = m_target ? m_target->targetIdentifier() : 0;
     
-    ref();
-    dispatchAsyncOnTarget(^{
+    dispatchAsyncOnTarget([&, strongThis = makeRef(*this)]() {
+        LockHolder lock(m_targetMutex);
+        if (m_target) {
+            if (m_connected)
+                m_target->disconnect(*this);
+
+            m_target = nullptr;
+            if (targetIdentifier)
+                RemoteInspector::singleton().updateTargetListing(targetIdentifier);
+        }
+    });
+}
+
+void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
+{
+    dispatchAsyncOnTarget([this, strongMessage = retainPtr(message), strongThis = makeRef(*this)]() {
+        RemoteControllableTarget* target = nullptr;
         {
             LockHolder lock(m_targetMutex);
-
-            if (!m_target || !m_target->remoteControlAllowed()) {
-                RemoteInspector::singleton().setupFailed(targetIdentifier);
-                m_target = nullptr;
-            } else if (is<RemoteInspectionTarget>(m_target)) {
-                auto castedTarget = downcast<RemoteInspectionTarget>(m_target);
-                castedTarget->connect(*this, isAutomaticInspection, automaticallyPause);
-                m_connected = true;
-
-                RemoteInspector::singleton().updateTargetListing(targetIdentifier);
-            } else if (is<RemoteAutomationTarget>(m_target)) {
-                auto castedTarget = downcast<RemoteAutomationTarget>(m_target);
-                castedTarget->connect(*this);
-                m_connected = true;
-
-                RemoteInspector::singleton().updateTargetListing(targetIdentifier);
-            }
+            if (!m_target)
+                return;
+            target = m_target;
         }
-        deref();
-    });
-
-    return true;
-}
-
-void RemoteConnectionToTarget::targetClosed()
-{
-    LockHolder lock(m_targetMutex);
-
-    m_target = nullptr;
-}
-
-void RemoteConnectionToTarget::close()
-{
-    auto targetIdentifier = m_target ? m_target->targetIdentifier() : 0;
-    
-    ref();
-    dispatchAsyncOnTarget(^{
-        {
-            LockHolder lock(m_targetMutex);
-            if (m_target) {
-                if (m_connected)
-                    m_target->disconnect(*this);
-
-                m_target = nullptr;
-                
-                RemoteInspector::singleton().updateTargetListing(targetIdentifier);
-            }
-        }
-        deref();
-    });
-}
-
-void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
-{
-    ref();
-    dispatchAsyncOnTarget(^{
-        {
-            RemoteControllableTarget* target = nullptr;
-            {
-                LockHolder lock(m_targetMutex);
-                if (!m_target)
-                    return;
-                target = m_target;
-            }
-
-            target->dispatchMessageFromRemote(message);
-        }
-        deref();
+
+        target->dispatchMessageFromRemote(strongMessage.get());
     });
 }
@@ -281 +267 @@
 }
 
-void RemoteConnectionToTarget::queueTaskOnPrivateRunLoop(void (^block)())
+void RemoteConnectionToTarget::queueTaskOnPrivateRunLoop(Function<void ()>&& function)
 {
     ASSERT(m_runLoop);
@@ -287 +273 @@
     {
         LockHolder lock(m_queueMutex);
-        m_queue.append(block);
+        m_queue.append(WTFMove(function));
     }
 
@@ -294 +280 @@
 }
 
+RemoteTargetQueue RemoteConnectionToTarget::takeQueue()
+{
+    return std::exchange(m_queue, { });
+}
+
 } // namespace Inspector