Title: [271876] trunk/Source/_javascript_Core
Revision
271876
Author
simon.fra...@apple.com
Date
2021-01-25 21:09:16 -0800 (Mon, 25 Jan 2021)

Log 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):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (271875 => 271876)


--- trunk/Source/_javascript_Core/ChangeLog	2021-01-26 03:43:54 UTC (rev 271875)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-01-26 05:09:16 UTC (rev 271876)
@@ -1,3 +1,26 @@
+2021-01-25  Simon Fraser  <simon.fra...@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  <shvaikal...@gmail.com>
 
         REGRESSION (r270874): Some React Native apps are reported broken on iOS

Modified: trunk/Source/_javascript_Core/inspector/remote/RemoteConnectionToTarget.h (271875 => 271876)


--- trunk/Source/_javascript_Core/inspector/remote/RemoteConnectionToTarget.h	2021-01-26 03:43:54 UTC (rev 271875)
+++ trunk/Source/_javascript_Core/inspector/remote/RemoteConnectionToTarget.h	2021-01-26 05:09:16 UTC (rev 271876)
@@ -44,7 +44,7 @@
 class RemoteControllableTarget;
 
 #if PLATFORM(COCOA)
-typedef Vector<BlockPtr<void ()>> RemoteTargetQueue;
+typedef Vector<Function<void ()>> RemoteTargetQueue;
 #endif
 
 class RemoteConnectionToTarget final : public ThreadSafeRefCounted<RemoteConnectionToTarget>, public FrontendChannel {
@@ -73,7 +73,7 @@
 
     Lock& queueMutex() { return m_queueMutex; }
     const RemoteTargetQueue& queue() const { return m_queue; }
-    void clearQueue() { m_queue.clear(); }
+    RemoteTargetQueue takeQueue();
 #endif
 
     // FrontendChannel overrides.
@@ -82,11 +82,11 @@
 
 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
 
     // This connection from the RemoteInspector singleton to the InspectionTarget

Modified: trunk/Source/_javascript_Core/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm (271875 => 271876)


--- trunk/Source/_javascript_Core/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm	2021-01-26 03:43:54 UTC (rev 271875)
+++ trunk/Source/_javascript_Core/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm	2021-01-26 05:09:16 UTC (rev 271876)
@@ -55,15 +55,14 @@
     RemoteTargetQueue queueCopy;
     {
         LockHolder lock(rwiQueueMutex);
-        queueCopy = *rwiQueue;
-        rwiQueue->clear();
+        std::swap(queueCopy, *rwiQueue);
     }
 
-    for (const auto& block : queueCopy)
-        block();
+    for (const auto& function : queueCopy)
+        function();
 }
 
-static void RemoteTargetQueueTaskOnGlobalQueue(void (^task)())
+static void RemoteTargetQueueTaskOnGlobalQueue(Function<void ()>&& function)
 {
     ASSERT(rwiRunLoopSource);
     ASSERT(rwiQueue);
@@ -70,7 +69,7 @@
 
     {
         LockHolder lock(rwiQueueMutex);
-        rwiQueue->append(task);
+        rwiQueue->append(WTFMove(function));
     }
 
     CFRunLoopSourceSignal(rwiRunLoopSource);
@@ -101,12 +100,11 @@
     RemoteTargetQueue queueCopy;
     {
         LockHolder lock(connectionToTarget->queueMutex());
-        queueCopy = connectionToTarget->queue();
-        connectionToTarget->clearQueue();
+        queueCopy = connectionToTarget->takeQueue();
     }
 
-    for (const auto& block : queueCopy)
-        block();
+    for (const auto& function : queueCopy)
+        function();
 }
 
 
@@ -138,21 +136,21 @@
     return [[m_destination copy] autorelease];
 }
 
-void RemoteConnectionToTarget::dispatchAsyncOnTarget(void (^block)())
+void RemoteConnectionToTarget::dispatchAsyncOnTarget(Function<void ()>&& callback)
 {
     if (m_runLoop) {
-        queueTaskOnPrivateRunLoop(block);
+        queueTaskOnPrivateRunLoop(WTFMove(callback));
         return;
     }
 
 #if USE(WEB_THREAD)
     if (WebCoreWebThreadIsEnabled && WebCoreWebThreadIsEnabled()) {
-        WebCoreWebThreadRun(block);
+        WebCoreWebThreadRun(^ { callback(); });
         return;
     }
 #endif
 
-    RemoteTargetQueueTaskOnGlobalQueue(block);
+    RemoteTargetQueueTaskOnGlobalQueue(WTFMove(callback));
 }
 
 bool RemoteConnectionToTarget::setup(bool isAutomaticInspection, bool automaticallyPause)
@@ -163,30 +161,26 @@
         return false;
 
     auto targetIdentifier = this->targetIdentifier().valueOr(0);
-    
-    ref();
-    dispatchAsyncOnTarget(^{
-        {
-            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;
+    dispatchAsyncOnTarget([&, strongThis = makeRef(*this)]() {
+        LockHolder lock(m_targetMutex);
 
-                RemoteInspector::singleton().updateTargetListing(targetIdentifier);
-            } else if (is<RemoteAutomationTarget>(m_target)) {
-                auto castedTarget = downcast<RemoteAutomationTarget>(m_target);
-                castedTarget->connect(*this);
-                m_connected = true;
+        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);
-            }
+            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);
         }
-        deref();
     });
 
     return true;
@@ -203,39 +197,31 @@
 {
     auto targetIdentifier = m_target ? m_target->targetIdentifier() : 0;
     
-    ref();
-    dispatchAsyncOnTarget(^{
-        {
-            LockHolder lock(m_targetMutex);
-            if (m_target) {
-                if (m_connected)
-                    m_target->disconnect(*this);
+    dispatchAsyncOnTarget([&, strongThis = makeRef(*this)]() {
+        LockHolder lock(m_targetMutex);
+        if (m_target) {
+            if (m_connected)
+                m_target->disconnect(*this);
 
-                m_target = nullptr;
-                
+            m_target = nullptr;
+            if (targetIdentifier)
                 RemoteInspector::singleton().updateTargetListing(targetIdentifier);
-            }
         }
-        deref();
     });
 }
 
 void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
 {
-    ref();
-    dispatchAsyncOnTarget(^{
+    dispatchAsyncOnTarget([this, strongMessage = retainPtr(message), strongThis = makeRef(*this)]() {
+        RemoteControllableTarget* target = nullptr;
         {
-            RemoteControllableTarget* target = nullptr;
-            {
-                LockHolder lock(m_targetMutex);
-                if (!m_target)
-                    return;
-                target = m_target;
-            }
+            LockHolder lock(m_targetMutex);
+            if (!m_target)
+                return;
+            target = m_target;
+        }
 
-            target->dispatchMessageFromRemote(message);
-        }
-        deref();
+        target->dispatchMessageFromRemote(strongMessage.get());
     });
 }
 
@@ -280,13 +266,13 @@
     m_runLoopSource = nullptr;
 }
 
-void RemoteConnectionToTarget::queueTaskOnPrivateRunLoop(void (^block)())
+void RemoteConnectionToTarget::queueTaskOnPrivateRunLoop(Function<void ()>&& function)
 {
     ASSERT(m_runLoop);
 
     {
         LockHolder lock(m_queueMutex);
-        m_queue.append(block);
+        m_queue.append(WTFMove(function));
     }
 
     CFRunLoopSourceSignal(m_runLoopSource.get());
@@ -293,6 +279,11 @@
     CFRunLoopWakeUp(m_runLoop.get());
 }
 
+RemoteTargetQueue RemoteConnectionToTarget::takeQueue()
+{
+    return std::exchange(m_queue, { });
+}
+
 } // namespace Inspector
 
 #endif // ENABLE(REMOTE_INSPECTOR)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to