Fix __DEBUGGER_SESSION_OBSERVER__ for concurrent sessions (#54083)

Summary:
Pull Request resolved: #54083

Changelog: [Internal]

Fixes a bug in `__DEBUGGER_SESSION_OBSERVER__` where the wrong state could be reported if more than one session existed. Now we reuse the accurate domain status tracking mechanism added in D83847356.

Reviewed By: hoxyq

Differential Revision: D84007326

fbshipit-source-id: e7f2c33286aa69c3bd9d5662d35dad579ca3c770

Author: motiz88

packages/react-native/ReactCommon/jsinspector-modern/RuntimeAgent.cpp

@@ -29,9 +29,14 @@ RuntimeAgent::RuntimeAgent(
     }
   }
 
-  if (sessionState_.isRuntimeDomainEnabled &&
-      sessionState_.isLogDomainEnabled) {
-    targetController_.notifyDebuggerSessionCreated();
+  if (sessionState_.isRuntimeDomainEnabled) {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Runtime, true, *this);
+  }
+
+  if (sessionState_.isLogDomainEnabled) {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Log, true, *this);
   }
 
   if (sessionState_.isNetworkDomainEnabled) {
@@ -60,20 +65,20 @@ bool RuntimeAgent::handleRequest(const cdp::PreparsedRequest& req) {
     // We are not responding to this request, just processing a side effect.
     return false;
   }
-  if (req.method == "Runtime.enable" && sessionState_.isLogDomainEnabled) {
-    targetController_.notifyDebuggerSessionCreated();
-  }
-  if (req.method == "Log.enable" && sessionState_.isRuntimeDomainEnabled) {
-    targetController_.notifyDebuggerSessionCreated();
-  }
-  if (req.method == "Runtime.disable" && sessionState_.isLogDomainEnabled) {
-    targetController_.notifyDebuggerSessionDestroyed();
-  }
-  if (req.method == "Log.disable" && sessionState_.isRuntimeDomainEnabled) {
-    targetController_.notifyDebuggerSessionDestroyed();
-  }
-
-  if (req.method == "Network.enable" || req.method == "Network.disable") {
+  if (req.method == "Runtime.enable" || req.method == "Runtime.disable") {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Runtime,
+        sessionState_.isRuntimeDomainEnabled,
+        *this);
+    // Fall through
+  } else if (req.method == "Log.enable" || req.method == "Log.disable") {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Log,
+        sessionState_.isLogDomainEnabled,
+        *this);
+    // Fall through
+  } else if (
+      req.method == "Network.enable" || req.method == "Network.disable") {
     targetController_.notifyDomainStateChanged(
         RuntimeTargetController::Domain::Network,
         sessionState_.isNetworkDomainEnabled,
@@ -120,9 +125,13 @@ RuntimeAgent::ExportedState RuntimeAgent::getExportedState() {
 }
 
 RuntimeAgent::~RuntimeAgent() {
-  if (sessionState_.isRuntimeDomainEnabled &&
-      sessionState_.isLogDomainEnabled) {
-    targetController_.notifyDebuggerSessionDestroyed();
+  if (sessionState_.isRuntimeDomainEnabled) {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Runtime, false, *this);
+  }
+  if (sessionState_.isLogDomainEnabled) {
+    targetController_.notifyDomainStateChanged(
+        RuntimeTargetController::Domain::Log, false, *this);
   }
   if (sessionState_.isNetworkDomainEnabled) {
     targetController_.notifyDomainStateChanged(

packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp

@@ -179,12 +179,38 @@ void RuntimeTarget::notifyDomainStateChanged(
     Domain domain,
     bool enabled,
     const RuntimeAgent& notifyingAgent) {
+  bool runtimeAndLogStatusBefore = false, runtimeAndLogStatusAfter = false;
+  if (domain == Domain::Log || domain == Domain::Runtime) {
+    runtimeAndLogStatusBefore =
+        agentsByEnabledDomain_[Domain::Runtime].contains(&notifyingAgent) &&
+        agentsByEnabledDomain_[Domain::Log].contains(&notifyingAgent);
+  }
+
   if (enabled) {
     agentsByEnabledDomain_[domain].insert(&notifyingAgent);
   } else {
     agentsByEnabledDomain_[domain].erase(&notifyingAgent);
   }
   threadSafeDomainStatus_[domain] = !agentsByEnabledDomain_[domain].empty();
+
+  if (domain == Domain::Log || domain == Domain::Runtime) {
+    runtimeAndLogStatusAfter =
+        agentsByEnabledDomain_[Domain::Runtime].contains(&notifyingAgent) &&
+        agentsByEnabledDomain_[Domain::Log].contains(&notifyingAgent);
+
+    if (runtimeAndLogStatusBefore != runtimeAndLogStatusAfter) {
+      if (runtimeAndLogStatusAfter) {
+        if (++agentsWithRuntimeAndLogDomainsEnabled_ == 1) {
+          emitDebuggerSessionCreated();
+        }
+      } else {
+        assert(agentsWithRuntimeAndLogDomainsEnabled_ > 0);
+        if (--agentsWithRuntimeAndLogDomainsEnabled_ == 0) {
+          emitDebuggerSessionDestroyed();
+        }
+      }
+    }
+  }
 }
 
 bool RuntimeTarget::isDomainEnabled(Domain domain) const {
@@ -199,14 +225,6 @@ void RuntimeTargetController::installBindingHandler(
   target_.installBindingHandler(bindingName);
 }
 
-void RuntimeTargetController::notifyDebuggerSessionCreated() {
-  target_.emitDebuggerSessionCreated();
-}
-
-void RuntimeTargetController::notifyDebuggerSessionDestroyed() {
-  target_.emitDebuggerSessionDestroyed();
-}
-
 void RuntimeTargetController::enableSamplingProfiler() {
   target_.enableSamplingProfiler();
 }

packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h

@@ -124,7 +124,7 @@ class RuntimeTargetDelegate {
  */
 class RuntimeTargetController {
  public:
-  enum class Domain { Network, kMaxValue };
+  enum class Domain { Network, Runtime, Log, kMaxValue };
 
   explicit RuntimeTargetController(RuntimeTarget& target);
 
@@ -135,18 +135,6 @@ class RuntimeTargetController {
    */
   void installBindingHandler(const std::string& bindingName);
 
-  /**
-   * Notifies the target to emit some message that debugger session is
-   * created.
-   */
-  void notifyDebuggerSessionCreated();
-
-  /**
-   * Notifies the target to emit some message that debugger session is
-   * destroyed.
-   */
-  void notifyDebuggerSessionDestroyed();
-
   /**
    * Notifies the target that an agent has received an enable or disable
    * message for the given domain.
@@ -294,6 +282,12 @@ class JSINSPECTOR_EXPORT RuntimeTarget
    */
   EnumArray<Domain, std::atomic<bool>> threadSafeDomainStatus_{};
 
+  /**
+   * The number of agents that currently have both the Log and Runtime domains
+   * enabled.
+   */
+  size_t agentsWithRuntimeAndLogDomainsEnabled_{0};
+
   /**
    * This TracingAgent is owned by the InstanceTracingAgent, both are bound to
    * the lifetime of their corresponding targets and the lifetime of the tracing

packages/react-native/ReactCommon/jsinspector-modern/tests/DebuggerSessionObserverTest.cpp

@@ -148,4 +148,48 @@ TEST_F(
   EXPECT_FALSE(eval("latestStatus").asBool());
 }
 
+TEST_F(DebuggerSessionObserverTest, testTwoConcurrentConnections) {
+  connect();
+
+  auto secondary = connectSecondary();
+
+  EXPECT_CALL(fromPage(), onMessage(_)).Times(AnyNumber());
+  EXPECT_CALL(secondary.fromPage(), onMessage(_)).Times(AnyNumber());
+
+  // No domains enabled to start with
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  toPage_->sendMessage(R"({"id": 1, "method": "Runtime.enable"})");
+  // Primary: Runtime, Secondary: None
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  secondary.toPage().sendMessage(R"({"id": 2, "method": "Log.enable"})");
+  // Primary: Runtime, Secondary: Log
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  secondary.toPage().sendMessage(R"({"id": 3, "method": "Runtime.enable"})");
+  // Primary: Runtime, Secondary: [Runtime, Log]
+  EXPECT_TRUE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  toPage_->sendMessage(R"({"id": 4, "method": "Log.enable"})");
+  // Primary: [Runtime, Log], Secondary: [Runtime, Log]
+  EXPECT_TRUE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  toPage_->sendMessage(R"({"id": 5, "method": "Runtime.disable"})");
+  // Primary: Log, Secondary: [Runtime, Log]
+  EXPECT_TRUE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  secondary.toPage().sendMessage(R"({"id": 6, "method": "Log.disable"})");
+  // Primary: Log, Secondary: Runtime
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  secondary.toPage().sendMessage(R"({"id": 7, "method": "Runtime.disable"})");
+  // Primary: Log, Secondary: None
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+
+  toPage_->sendMessage(R"({"id": 8, "method": "Log.disable"})");
+  // Primary: None, Secondary: None
+  EXPECT_FALSE(eval("__DEBUGGER_SESSION_OBSERVER__.hasActiveSession").asBool());
+}
+
 } // namespace facebook::react::jsinspector_modern

packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.h

@@ -164,6 +164,52 @@ class JsiIntegrationPortableTestBase : public ::testing::Test,
     return engineAdapter_->getRuntime();
   }
 
+  class SecondaryConnection {
+   public:
+    SecondaryConnection(
+        std::unique_ptr<ILocalConnection> toPage,
+        JsiIntegrationPortableTestBase<EngineAdapter, Executor>& test,
+        size_t remoteConnectionIndex)
+        : toPage_(std::move(toPage)),
+          remoteConnectionIndex_(remoteConnectionIndex),
+          test_(test) {}
+
+    ILocalConnection& toPage() {
+      return *toPage_;
+    }
+
+    MockRemoteConnection& fromPage() {
+      return *test_.remoteConnections_[remoteConnectionIndex_];
+    }
+
+   private:
+    std::unique_ptr<ILocalConnection> toPage_;
+    size_t remoteConnectionIndex_;
+    JsiIntegrationPortableTestBase<EngineAdapter, Executor>& test_;
+  };
+
+  SecondaryConnection connectSecondary() {
+    auto toPage = page_->connect(remoteConnections_.make_unique());
+
+    SecondaryConnection secondary{
+        std::move(toPage), *this, remoteConnections_.objectsVended() - 1};
+
+    using namespace ::testing;
+    // Default to ignoring console messages originating inside the backend.
+    EXPECT_CALL(
+        secondary.fromPage(),
+        onMessage(JsonParsed(AllOf(
+            AtJsonPtr("/method", "Runtime.consoleAPICalled"),
+            AtJsonPtr("/params/context", "main#InstanceAgent")))))
+        .Times(AnyNumber());
+
+    // We'll always get an onDisconnect call when we tear
+    // down the test. Expect it in order to satisfy the strict mock.
+    EXPECT_CALL(secondary.fromPage(), onDisconnect());
+
+    return secondary;
+  }
+
   std::shared_ptr<HostTarget> page_;
   InstanceTarget* instance_{};
   RuntimeTarget* runtimeTarget_{};