Stash background trace directly in HostTarget, simplify API (facebook#55347)

Summary:

Changelog: [Internal]

Moves state and logic related to stashing a background trace from `JReactHostInspectorTarget` (JNI wrapper boilerplate, Android-specific) to `HostTarget` (cross-platform C++).

Managing the full lifecycle of a background trace inside `HostTarget` lets us remove a fair amount of API surface:
* `HostTargetDelegate::unstable_getHostTracingProfileThatWillBeEmittedOnInitialization()`
* `HostTarget::hasActiveSessionWithFuseboxClient()`
* `TracingAgent::emitExternalHostTracingProfile()`

We also refactor some of the surrounding code and add tests (previously missing) for the behaviour of stashed background traces when there is more than one session.

Reviewed By: huntie, hoxyq

Differential Revision: D91589884

Author: motiz88

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt

@@ -41,7 +41,7 @@ internal class ReactHostInspectorTarget(reactHostImpl: ReactHostImpl) :
 
   external fun stopAndMaybeEmitBackgroundTrace(): Boolean
 
-  external fun stopAndDiscardBackgroundTrace()
+  external fun stopTracing()
 
   external override fun getTracingState(): TracingState
 
@@ -65,7 +65,7 @@ internal class ReactHostInspectorTarget(reactHostImpl: ReactHostImpl) :
   }
 
   override fun stopBackgroundTrace() {
-    stopAndDiscardBackgroundTrace()
+    stopTracing()
   }
 
   fun handleNativePerfIssueAdded(

packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp

@@ -73,13 +73,16 @@ JReactHostInspectorTarget::initHybrid(
 }
 
 void JReactHostInspectorTarget::sendDebuggerResumeCommand() {
+  inspectorTarget().sendCommand(HostCommand::DebuggerResume);
+}
+
+jsinspector_modern::HostTarget& JReactHostInspectorTarget::inspectorTarget() {
   if (inspectorTarget_) {
-    inspectorTarget_->sendCommand(HostCommand::DebuggerResume);
-  } else {
-    jni::throwNewJavaException(
-        "java/lang/IllegalStateException",
-        "Cannot send command while the Fusebox backend is not enabled");
+    return *inspectorTarget_;
   }
+  jni::throwNewJavaException(
+      "java/lang/IllegalStateException",
+      "Inspector method called while the Fusebox backend is not enabled.");
 }
 
 jsinspector_modern::HostTargetMetadata
@@ -147,58 +150,22 @@ HostTarget* JReactHostInspectorTarget::getInspectorTarget() {
 }
 
 bool JReactHostInspectorTarget::startBackgroundTrace() {
-  if (inspectorTarget_) {
-    return inspectorTarget_->startTracing(
-        tracing::Mode::Background,
-        {
-            tracing::Category::HiddenTimeline,
-            tracing::Category::RuntimeExecution,
-            tracing::Category::Timeline,
-            tracing::Category::UserTiming,
-        });
-  } else {
-    jni::throwNewJavaException(
-        "java/lang/IllegalStateException",
-        "Cannot start Tracing session while the Fusebox backend is not enabled.");
-  }
+  return inspectorTarget().startTracing(
+      tracing::Mode::Background,
+      {
+          tracing::Category::HiddenTimeline,
+          tracing::Category::RuntimeExecution,
+          tracing::Category::Timeline,
+          tracing::Category::UserTiming,
+      });
 }
 
-tracing::HostTracingProfile JReactHostInspectorTarget::stopTracing() {
-  if (inspectorTarget_) {
-    return inspectorTarget_->stopTracing();
-  } else {
-    jni::throwNewJavaException(
-        "java/lang/IllegalStateException",
-        "Cannot stop Tracing session while the Fusebox backend is not enabled.");
-  }
+void JReactHostInspectorTarget::stopTracing() {
+  inspectorTarget().stopTracing();
 }
 
 jboolean JReactHostInspectorTarget::stopAndMaybeEmitBackgroundTrace() {
-  auto capturedTrace = inspectorTarget_->stopTracing();
-  if (inspectorTarget_->hasActiveSessionWithFuseboxClient()) {
-    inspectorTarget_->emitTracingProfileForFirstFuseboxClient(
-        std::move(capturedTrace));
-    return jboolean(true);
-  }
-
-  stashTracingProfile(std::move(capturedTrace));
-  return jboolean(false);
-}
-
-void JReactHostInspectorTarget::stopAndDiscardBackgroundTrace() {
-  inspectorTarget_->stopTracing();
-}
-
-void JReactHostInspectorTarget::stashTracingProfile(
-    tracing::HostTracingProfile&& hostTracingProfile) {
-  stashedTracingProfile_ = std::move(hostTracingProfile);
-}
-
-std::optional<tracing::HostTracingProfile> JReactHostInspectorTarget::
-    unstable_getHostTracingProfileThatWillBeEmittedOnInitialization() {
-  auto tracingProfile = std::move(stashedTracingProfile_);
-  stashedTracingProfile_.reset();
-  return tracingProfile;
+  return jboolean(inspectorTarget().stopAndMaybeEmitBackgroundTrace());
 }
 
 void JReactHostInspectorTarget::registerNatives() {
@@ -213,9 +180,7 @@ void JReactHostInspectorTarget::registerNatives() {
       makeNativeMethod(
           "stopAndMaybeEmitBackgroundTrace",
           JReactHostInspectorTarget::stopAndMaybeEmitBackgroundTrace),
-      makeNativeMethod(
-          "stopAndDiscardBackgroundTrace",
-          JReactHostInspectorTarget::stopAndDiscardBackgroundTrace),
+      makeNativeMethod("stopTracing", JReactHostInspectorTarget::stopTracing),
       makeNativeMethod(
           "getTracingState", JReactHostInspectorTarget::getTracingState),
       makeNativeMethod(
@@ -256,7 +221,7 @@ HostTargetTracingDelegate* JReactHostInspectorTarget::getTracingDelegate() {
 
 void JReactHostInspectorTarget::recordFrameTimings(
     jni::alias_ref<JFrameTimingSequence::javaobject> frameTimingSequence) {
-  inspectorTarget_->recordFrameTimings({
+  inspectorTarget().recordFrameTimings({
       frameTimingSequence->getId(),
       frameTimingSequence->getThreadId(),
       frameTimingSequence->getBeginDrawingTimestamp(),

packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h

@@ -240,7 +240,7 @@ class JReactHostInspectorTarget : public jni::HybridClass<JReactHostInspectorTar
   /**
    * Stops previously started trace recording and discards the captured trace.
    */
-  void stopAndDiscardBackgroundTrace();
+  void stopTracing();
 
   jsinspector_modern::HostTarget *getInspectorTarget();
 
@@ -277,15 +277,20 @@ class JReactHostInspectorTarget : public jni::HybridClass<JReactHostInspectorTar
   void loadNetworkResource(
       const jsinspector_modern::LoadNetworkResourceRequest &params,
       jsinspector_modern::ScopedExecutor<jsinspector_modern::NetworkRequestListener> executor) override;
-  std::optional<jsinspector_modern::tracing::HostTracingProfile>
-  unstable_getHostTracingProfileThatWillBeEmittedOnInitialization() override;
   jsinspector_modern::HostTargetTracingDelegate *getTracingDelegate() override;
 
  private:
   JReactHostInspectorTarget(
       jni::alias_ref<JReactHostInspectorTarget::javaobject> jobj,
       jni::alias_ref<JReactHostImpl> reactHostImpl,
       jni::alias_ref<JExecutor::javaobject> javaExecutor);
+
+  /**
+   * Returns a reference to the HostTarget, throwing a Java IllegalStateException
+   * if the Fusebox backend is not enabled (i.e., inspectorTarget_ is null).
+   */
+  jsinspector_modern::HostTarget &inspectorTarget();
+
   jni::global_ref<JReactHostInspectorTarget::javaobject> jobj_;
   // This weak reference breaks the cycle between the C++ HostTarget and the
   // Java ReactHostImpl, preventing memory leaks in apps that create multiple
@@ -296,20 +301,6 @@ class JReactHostInspectorTarget : public jni::HybridClass<JReactHostInspectorTar
   std::shared_ptr<jsinspector_modern::HostTarget> inspectorTarget_;
   std::optional<int> inspectorPageId_;
 
-  /**
-   * Stops previously started trace recording and returns the captured HostTracingProfile.
-   */
-  jsinspector_modern::tracing::HostTracingProfile stopTracing();
-  /**
-   * Stashes previously recorded HostTracingProfile that will be emitted when
-   * CDP session is created. Once emitted, the value will be cleared from this
-   * instance.
-   */
-  void stashTracingProfile(jsinspector_modern::tracing::HostTracingProfile &&hostTracingProfile);
-  /**
-   * Previously recorded HostTracingProfile that will be emitted when CDP session is created.
-   */
-  std::optional<jsinspector_modern::tracing::HostTracingProfile> stashedTracingProfile_;
   /**
    * Encapsulates the logic around tracing for this HostInspectorTarget.
    */

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

@@ -236,13 +236,11 @@ class HostAgent::Impl final {
         emitSystemStateChanged(isSingleHost);
       }
 
-      auto stashedTraceRecording =
-          targetController_.getDelegate()
-              .unstable_getHostTracingProfileThatWillBeEmittedOnInitialization();
-      if (stashedTraceRecording.has_value()) {
-        tracingAgent_.emitExternalHostTracingProfile(
-            std::move(stashedTraceRecording.value()));
-      }
+      auto emitted = targetController_.maybeEmitStashedBackgroundTrace();
+      assert(
+          emitted &&
+          "Expected to find at least one session eligible to receive a background trace after ReactNativeApplication.enable");
+      (void)emitted;
 
       return {
           .isFinishedHandlingRequest = true,

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

@@ -102,17 +102,8 @@ class HostTargetSession {
     }
   }
 
-  /**
-   * Returns whether the ReactNativeApplication CDP domain is enabled.
-   *
-   * Chrome DevTools Frontend enables this domain as a client.
-   */
-  bool hasFuseboxClient() const {
-    return hostAgent_.hasFuseboxClientConnected();
-  }
-
-  void emitHostTracingProfile(tracing::HostTracingProfile tracingProfile) {
-    hostAgent_.emitExternalTracingProfile(std::move(tracingProfile));
+  HostAgent& agent() {
+    return hostAgent_;
   }
 
  private:
@@ -323,6 +314,10 @@ bool HostTargetController::decrementPauseOverlayCounter() {
   return --pauseOverlayCounter_ != 0;
 }
 
+bool HostTargetController::maybeEmitStashedBackgroundTrace() {
+  return target_.maybeEmitStashedBackgroundTrace();
+}
+
 namespace {
 
 struct StaticHostTargetMetadata {
@@ -374,32 +369,26 @@ folly::dynamic createHostMetadataPayload(const HostTargetMetadata& metadata) {
   return result;
 }
 
-bool HostTarget::hasActiveSessionWithFuseboxClient() const {
-  bool hasActiveFuseboxSession = false;
-  sessions_.forEach([&](auto& session) {
-    hasActiveFuseboxSession |= session.hasFuseboxClient();
-  });
-  return hasActiveFuseboxSession;
-}
-
-void HostTarget::emitTracingProfileForFirstFuseboxClient(
-    tracing::HostTracingProfile tracingProfile) {
+bool HostTarget::maybeEmitStashedBackgroundTrace() {
   bool emitted = false;
   sessions_.forEach([&](auto& session) {
     if (emitted) {
-      /**
-       * HostTracingProfile object is not copiable for performance reasons,
-       * because it could contain large Runtime sampling profile object.
-       *
-       * This approach would not work with multi-client debugger setup.
-       */
       return;
     }
-    if (session.hasFuseboxClient()) {
-      session.emitHostTracingProfile(std::move(tracingProfile));
+    if (session.agent().hasFuseboxClientConnected()) {
+      auto stashedTrace = std::exchange(stashedTracingProfile_, std::nullopt);
+      if (stashedTrace) {
+        session.agent().emitExternalTracingProfile(std::move(*stashedTrace));
+      }
       emitted = true;
     }
   });
+  return emitted;
+}
+
+bool HostTarget::stopAndMaybeEmitBackgroundTrace() {
+  stashedTracingProfile_ = stopTracing();
+  return maybeEmitStashedBackgroundTrace();
 }
 
 } // namespace facebook::react::jsinspector_modern

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

@@ -181,19 +181,6 @@ class HostTargetDelegate : public LoadNetworkResourceDelegate {
         "LoadNetworkResourceDelegate.loadNetworkResource is not implemented by this host target delegate.");
   }
 
-  /**
-   * [Experimental] Will be called at the CDP session initialization to get the
-   * trace recording that may have been stashed by the Host from the previous
-   * background session.
-   *
-   * \return the HostTracingProfile if there is one that needs to be
-   * displayed, otherwise std::nullopt.
-   */
-  virtual std::optional<tracing::HostTracingProfile> unstable_getHostTracingProfileThatWillBeEmittedOnInitialization()
-  {
-    return std::nullopt;
-  }
-
   /**
    * An optional delegate that will be used by HostTarget to notify about tracing-related events.
    */
@@ -256,6 +243,12 @@ class HostTargetController final {
    */
   tracing::HostTracingProfile stopTracing();
 
+  /**
+   * If there is a stashed background trace, emit it to the first eligible session.
+   * \return true if an eligible session is found (even if there was no stashed background trace).
+   */
+  bool maybeEmitStashedBackgroundTrace();
+
  private:
   HostTarget &target_;
   size_t pauseOverlayCounter_{0};
@@ -352,18 +345,13 @@ class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis<HostTarget>
   tracing::HostTracingProfile stopTracing();
 
   /**
-   * Returns whether there is an active session with the Fusebox client, i.e.
-   * with Chrome DevTools Frontend fork for React Native.
+   * Stops previously started trace recording and:
+   *  - If there is an active CDP session with ReactNativeApplication domain
+   *    enabled, emits the trace and returns true.
+   *  - Otherwise, stashes the captured trace, that will be emitted when a CDP
+   *    session enables ReactNativeApplication. Returns false.
    */
-  bool hasActiveSessionWithFuseboxClient() const;
-
-  /**
-   * Emits the HostTracingProfile for the first active session with the Fusebox
-   * client.
-   *
-   * @see \c hasActiveFrontendSession
-   */
-  void emitTracingProfileForFirstFuseboxClient(tracing::HostTracingProfile tracingProfile);
+  bool stopAndMaybeEmitBackgroundTrace();
 
   /**
    * An endpoint for the Host to report frame timings that will be recorded if and only if there is currently an active
@@ -403,6 +391,11 @@ class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis<HostTarget>
    * Should only be allocated when there is an active tracing session.
    */
   std::unique_ptr<HostTargetTraceRecording> traceRecording_{nullptr};
+  /**
+   * Previously recorded HostTracingProfile that will be emitted when CDP session is created
+   * and enables ReactNativeApplication. Once emitted, the value will be cleared.
+   */
+  std::optional<tracing::HostTracingProfile> stashedTracingProfile_;
   /**
    * Protects the state inside traceRecording_.
    *
@@ -429,6 +422,12 @@ class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis<HostTarget>
    */
   void installPerfIssuesBinding();
 
+  /**
+   * If there is a stashed background trace, emit it to the first eligible session.
+   * \return true if an eligible session is found (even if there was no stashed background trace).
+   */
+  bool maybeEmitStashedBackgroundTrace();
+
   // Necessary to allow HostAgent to access HostTarget's internals in a
   // controlled way (i.e. only HostTargetController gets friend access, while
   // HostAgent itself doesn't).