Web Inspector: Closing JSContext inspector while execution is paused hangs owner app

rdar://142537911
https://bugs.webkit.org/show_bug.cgi?id=291711

Reviewed by Devin Rousso.

When closing Web Inspector and disconnecting the frontend from the
debuggable, we have the logic to resume code execution if it was paused.
Resuming execution by default notifies the frontend via RemoteConnectionToTarget::sendMessageToFrontend,
which tries to grab the m_targetMutex, but that was already grabbed
in RemoteConnectionToTarget::close (in the callback with dispatchAsyncOnTarget).
This resulted in a deadlock.

   - The reason why this deadlocking only happens for JSContexts is
     because the other kinds of debuggables call into a different thread
     at some point to complete the disconnecting process. PageDebuggable
     and WebPageDebuggable call callOnMainThreadAndWait in the
     debuggables' disconnect method; for service workers, postDebuggerTask
     is used in ServiceWorkerInspectorProxy::disconnectFromWorker.
     JSContext does everything on the same thread that calls disconnect.

If the frontend were going to be closed anyway, RemoteConnectionToTarget
should be able to avoid sending the frontend any messages, and that's
the logic behind this patch.

* Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:
(Inspector::RemoteConnectionToTarget::setup):
(Inspector::RemoteConnectionToTarget::sendMessageToTarget):
(Inspector::RemoteConnectionToTarget::close):
(Inspector::RemoteConnectionToTarget::targetClosed):
(Inspector::RemoteConnectionToTarget::sendMessageToFrontend):
* Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.h:
* Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:
(Inspector::RemoteConnectionToTarget::setup):
(Inspector::RemoteConnectionToTarget::targetClosed):
(Inspector::RemoteConnectionToTarget::close):
(Inspector::RemoteConnectionToTarget::sendMessageToTarget):
(Inspector::RemoteConnectionToTarget::sendMessageToFrontend):

Canonical link: https://commits.webkit.org/293950@main

Author: the-chenergy

Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp

@@ -67,14 +67,17 @@ bool RemoteConnectionToTarget::setup(bool isAutomaticInspection, bool automatica
     else if (auto* automationTarget = dynamicDowncast<RemoteAutomationTarget>(*target))
         automationTarget->connect(*this);
 
-    m_connected = true;
+    m_connectionState = ConnectionState::Connected;
     RemoteInspector::singleton().updateTargetListing(targetIdentifier);
     RemoteInspector::singleton().setupCompleted(targetIdentifier);
     return true;
 }
 
 void RemoteConnectionToTarget::sendMessageToTarget(String&& message)
 {
+    if (m_connectionState == ConnectionState::Closed)
+        return;
+
     RefPtr<RemoteControllableTarget> target;
     {
         Locker locker { m_targetMutex };
@@ -93,7 +96,7 @@ void RemoteConnectionToTarget::close()
         if (RefPtr target = m_target.get()) {
             targetIdentifier = target->targetIdentifier();
 
-            if (m_connected)
+            if (m_connectionState.exchange(ConnectionState::Closed) == ConnectionState::Connected)
                 target->disconnect(*this);
 
             m_target = nullptr;
@@ -107,6 +110,7 @@ void RemoteConnectionToTarget::close()
 void RemoteConnectionToTarget::targetClosed()
 {
     Locker locker { m_targetMutex };
+    m_connectionState = ConnectionState::Closed;
     m_target = nullptr;
 }
 
@@ -118,6 +122,9 @@ std::optional<TargetID> RemoteConnectionToTarget::targetIdentifier() const
 
 void RemoteConnectionToTarget::sendMessageToFrontend(const String& message)
 {
+    if (m_connectionState == ConnectionState::Closed)
+        return;
+
     std::optional<TargetID> targetIdentifier;
     {
         Locker locker { m_targetMutex };

Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.h

@@ -105,7 +105,13 @@ class RemoteConnectionToTarget final : public ThreadSafeRefCounted<RemoteConnect
 #endif
 
     ThreadSafeWeakPtr<RemoteControllableTarget> m_target WTF_GUARDED_BY_LOCK(m_targetMutex);
-    bool m_connected { false };
+
+    enum class ConnectionState {
+        Pending,
+        Connected,
+        Closed,
+    };
+    std::atomic<ConnectionState> m_connectionState { ConnectionState::Pending };
 
 #if PLATFORM(COCOA)
     RetainPtr<NSString> m_connectionIdentifier;

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

@@ -185,7 +185,7 @@ static void RemoteTargetHandleRunSourceWithInfo(void* info)
         else if (auto* automationTarget = dynamicDowncast<RemoteAutomationTarget>(target.get()))
             automationTarget->connect(*this);
 
-        m_connected = true;
+        m_connectionState = ConnectionState::Connected;
         RemoteInspector::singleton().updateTargetListing(targetIdentifier);
         RemoteInspector::singleton().setupCompleted(targetIdentifier);
     });
@@ -196,7 +196,7 @@ static void RemoteTargetHandleRunSourceWithInfo(void* info)
 void RemoteConnectionToTarget::targetClosed()
 {
     Locker locker { m_targetMutex };
-
+    m_connectionState = ConnectionState::Closed;
     m_target = nullptr;
 }
 
@@ -212,7 +212,7 @@ static void RemoteTargetHandleRunSourceWithInfo(void* info)
     dispatchAsyncOnTarget([this, targetIdentifier, protectedThis = Ref { *this }]() {
         Locker locker { m_targetMutex };
         if (RefPtr target = m_target.get()) {
-            if (m_connected)
+            if (m_connectionState.exchange(ConnectionState::Closed) == ConnectionState::Connected)
                 target->disconnect(*this);
 
             m_target = nullptr;
@@ -225,6 +225,9 @@ static void RemoteTargetHandleRunSourceWithInfo(void* info)
 
 void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
 {
+    if (m_connectionState == ConnectionState::Closed)
+        return;
+
     dispatchAsyncOnTarget([this, strongMessage = retainPtr(message), protectedThis = Ref { *this }]() {
         RefPtr<RemoteControllableTarget> target;
         {
@@ -238,6 +241,9 @@ static void RemoteTargetHandleRunSourceWithInfo(void* info)
 
 void RemoteConnectionToTarget::sendMessageToFrontend(const String& message)
 {
+    if (m_connectionState == ConnectionState::Closed)
+        return;
+
     std::optional<TargetID> targetIdentifier;
     {
         Locker locker { m_targetMutex };