From fe5d02fe1bcf6f35f8e99b95ad310aab3f619c99 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 29 Jan 2026 09:02:01 -0800 Subject: [PATCH 1/3] Remove const qualifier from side-effectful methods of Agents and Targets (#55314) Summary: TSIA - minor refactor for correctness and clarity before making changes to some of these methods up the stack. Changelog: [Internal] Reviewed By: hoxyq Differential Revision: D91140491 --- .../ReactCommon/jsinspector-modern/HostAgent.cpp | 7 +++---- .../ReactCommon/jsinspector-modern/HostAgent.h | 4 ++-- .../ReactCommon/jsinspector-modern/HostTarget.cpp | 5 ++--- .../ReactCommon/jsinspector-modern/HostTarget.h | 2 +- .../ReactCommon/jsinspector-modern/TracingAgent.cpp | 4 ++-- .../ReactCommon/jsinspector-modern/TracingAgent.h | 4 ++-- 6 files changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp index 41d81767df350d..888d27936e90ee 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp @@ -385,8 +385,7 @@ class HostAgent::Impl final { return fuseboxClientType_ == FuseboxClientType::Fusebox; } - void emitExternalTracingProfile( - tracing::HostTracingProfile tracingProfile) const { + void emitExternalTracingProfile(tracing::HostTracingProfile tracingProfile) { assert( hasFuseboxClientConnected() && "Attempted to emit a trace recording to a non-Fusebox client"); @@ -543,11 +542,11 @@ bool HostAgent::hasFuseboxClientConnected() const { } void HostAgent::emitExternalTracingProfile( - tracing::HostTracingProfile tracingProfile) const { + tracing::HostTracingProfile tracingProfile) { impl_->emitExternalTracingProfile(std::move(tracingProfile)); } -void HostAgent::emitSystemStateChanged(bool isSingleHost) const { +void HostAgent::emitSystemStateChanged(bool isSingleHost) { impl_->emitSystemStateChanged(isSingleHost); } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h index 8789cb5d32c165..3046a7788c9686 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h @@ -77,13 +77,13 @@ class HostAgent final { * Emits the HostTracingProfile that was captured externally, not via the * CDP-initiated request. */ - void emitExternalTracingProfile(tracing::HostTracingProfile tracingProfile) const; + void emitExternalTracingProfile(tracing::HostTracingProfile tracingProfile); /** * Emits a system state changed event when the number of ReactHost instances * changes. */ - void emitSystemStateChanged(bool isSingleHost) const; + void emitSystemStateChanged(bool isSingleHost); private: // We use the private implementation idiom to ensure this class has the same diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp index e34ca99bfad2eb..0e3cca677f86ce 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp @@ -111,8 +111,7 @@ class HostTargetSession { return hostAgent_.hasFuseboxClientConnected(); } - void emitHostTracingProfile( - tracing::HostTracingProfile tracingProfile) const { + void emitHostTracingProfile(tracing::HostTracingProfile tracingProfile) { hostAgent_.emitExternalTracingProfile(std::move(tracingProfile)); } @@ -384,7 +383,7 @@ bool HostTarget::hasActiveSessionWithFuseboxClient() const { } void HostTarget::emitTracingProfileForFirstFuseboxClient( - tracing::HostTracingProfile tracingProfile) const { + tracing::HostTracingProfile tracingProfile) { bool emitted = false; sessions_.forEach([&](HostTargetSession& session) { if (emitted) { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h index aed1396b14e6f3..b1640f31a47383 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h @@ -363,7 +363,7 @@ class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis * * @see \c hasActiveFrontendSession */ - void emitTracingProfileForFirstFuseboxClient(tracing::HostTracingProfile tracingProfile) const; + void emitTracingProfileForFirstFuseboxClient(tracing::HostTracingProfile tracingProfile); /** * An endpoint for the Host to report frame timings that will be recorded if and only if there is currently an active diff --git a/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.cpp b/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.cpp index 3fc0957c9010de..4a876eb1b02dad 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.cpp @@ -120,14 +120,14 @@ bool TracingAgent::handleRequest(const cdp::PreparsedRequest& req) { } void TracingAgent::emitExternalHostTracingProfile( - tracing::HostTracingProfile tracingProfile) const { + tracing::HostTracingProfile tracingProfile) { frontendChannel_( cdp::jsonNotification("ReactNativeApplication.traceRequested")); emitHostTracingProfile(std::move(tracingProfile)); } void TracingAgent::emitHostTracingProfile( - tracing::HostTracingProfile tracingProfile) const { + tracing::HostTracingProfile tracingProfile) { auto dataCollectedCallback = [this](folly::dynamic&& eventsChunk) { frontendChannel_( cdp::jsonNotification( diff --git a/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.h b/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.h index d0f474f1234bca..3b2a6325989880 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/TracingAgent.h @@ -44,7 +44,7 @@ class TracingAgent { /** * Emits the HostTracingProfile that was stashed externally by the HostTarget. */ - void emitExternalHostTracingProfile(tracing::HostTracingProfile tracingProfile) const; + void emitExternalHostTracingProfile(tracing::HostTracingProfile tracingProfile); private: /** @@ -60,7 +60,7 @@ class TracingAgent { * Emits captured HostTracingProfile in a series of * Tracing.dataCollected events, followed by a Tracing.tracingComplete event. */ - void emitHostTracingProfile(tracing::HostTracingProfile tracingProfile) const; + void emitHostTracingProfile(tracing::HostTracingProfile tracingProfile); }; } // namespace facebook::react::jsinspector_modern From 40d939a8fcae34f2f34afca48a8f4a16e269a9d3 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 29 Jan 2026 09:02:01 -0800 Subject: [PATCH 2/3] WeakList: add anyOf + const-propagating forEach (#55313) Summary: Changelog: [Internal] TSIA - minor refactor for convenience and correctness. NOTE: When we get C++23, we can deduplicate the identical const/non-const method bodies using the nifty [deducing `this`](https://en.cppreference.com/w/cpp/language/member_functions.html#Explicit_object_member_functions) feature. Reviewed By: hoxyq Differential Revision: D91140490 --- .../jsinspector-modern/HostTarget.cpp | 4 +- .../ReactCommon/jsinspector-modern/WeakList.h | 61 ++++++++++++++++++- .../jsinspector-modern/tests/WeakListTest.cpp | 32 +++++++++- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp index 0e3cca677f86ce..fae9d52605ff6a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp @@ -376,7 +376,7 @@ folly::dynamic createHostMetadataPayload(const HostTargetMetadata& metadata) { bool HostTarget::hasActiveSessionWithFuseboxClient() const { bool hasActiveFuseboxSession = false; - sessions_.forEach([&](HostTargetSession& session) { + sessions_.forEach([&](auto& session) { hasActiveFuseboxSession |= session.hasFuseboxClient(); }); return hasActiveFuseboxSession; @@ -385,7 +385,7 @@ bool HostTarget::hasActiveSessionWithFuseboxClient() const { void HostTarget::emitTracingProfileForFirstFuseboxClient( tracing::HostTracingProfile tracingProfile) { bool emitted = false; - sessions_.forEach([&](HostTargetSession& session) { + sessions_.forEach([&](auto& session) { if (emitted) { /** * HostTracingProfile object is not copiable for performance reasons, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h b/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h index 00338c344ab59c..8b299742449e55 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h @@ -30,7 +30,7 @@ class WeakList { * to destroyed elements) will be removed during iteration. */ template - void forEach(Fn &&fn) const + void forEach(Fn &&fn) { for (auto it = ptrs_.begin(); it != ptrs_.end();) { if (auto ptr = it->lock()) { @@ -42,6 +42,65 @@ class WeakList { } } + /** + * Call the given function for every element in the list, ensuring the element + * is not destroyed for the duration of the call. Elements are visited in the + * order they were inserted. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during iteration. + */ + template + void forEach(Fn &&fn) const + { + for (auto it = ptrs_.cbegin(); it != ptrs_.cend();) { + if (auto ptr = it->lock()) { + fn(*ptr); + ++it; + } else { + it = ptrs_.erase(it); + } + } + } + + /** + * Returns true if the given function returns true for any element in the + * list, ensuring the element is not destroyed for the duration of the call. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during iteration. + */ + template + bool anyOf(Fn &&fn) + { + bool found = false; + forEach([&](auto &element) { + if (!found && fn(element)) { + found = true; + } + }); + return found; + } + + /** + * Returns true if the given function returns true for any element in the + * list, ensuring the element is not destroyed for the duration of the call. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during iteration. + */ + template + bool anyOf(Fn &&fn) const + { + bool found = false; + forEach([&](const auto &element) { + if (!found && fn(element)) { + found = true; + } + }); + return found; + } + /** * Returns the number of (non-null) elements in the list. The count will only * remain accurate as long as the list is not modified and elements are diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp index ee86a8e68a9435..d228c7cc0ff2b3 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp @@ -70,7 +70,7 @@ TEST(WeakListTest, ForEach) { EXPECT_THAT(visited, ElementsAre(1, 3)); } -TEST(WeakListTest, ElementsAreAliveDuringCallback) { +TEST(WeakListTest, ElementsAreAliveDuringForEachCallback) { WeakList list; auto p1 = std::make_shared(1); // A separate weak_ptr to observe the lifetime of `p1`. @@ -88,4 +88,34 @@ TEST(WeakListTest, ElementsAreAliveDuringCallback) { EXPECT_THAT(visited, ElementsAre(1)); } +TEST(WeakListTest, AnyOf) { + WeakList list; + auto p1 = std::make_shared(1); + auto p2 = std::make_shared(2); + auto p3 = std::make_shared(3); + list.insert(p1); + list.insert(p2); + list.insert(p3); + + EXPECT_TRUE(list.anyOf([](const int& value) { return value == 2; })); + EXPECT_FALSE(list.anyOf([](const int& value) { return value == 4; })); +} + +TEST(WeakListTest, ElementsAreAliveDuringAnyOfCallback) { + WeakList list; + auto p1 = std::make_shared(42); + // A separate weak_ptr to observe the lifetime of `p1`. + std::weak_ptr wp1 = p1; + list.insert(p1); + + bool result = list.anyOf([&](const int& value) { + p1.reset(); + EXPECT_FALSE(wp1.expired()); + return value == 42; + }); + + EXPECT_TRUE(result); + EXPECT_TRUE(wp1.expired()); +} + } // namespace facebook::react::jsinspector_modern From 1aab2f904b53eb21ec4a1a45afa02ff73d707071 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 29 Jan 2026 09:02:01 -0800 Subject: [PATCH 3/3] Stash background trace directly in HostTarget, simplify API (#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 --- .../react/runtime/ReactHostInspectorTarget.kt | 4 +- .../runtime/jni/JReactHostInspectorTarget.cpp | 77 +++------- .../runtime/jni/JReactHostInspectorTarget.h | 25 +--- .../jsinspector-modern/HostAgent.cpp | 12 +- .../jsinspector-modern/HostTarget.cpp | 47 +++--- .../jsinspector-modern/HostTarget.h | 47 +++--- .../jsinspector-modern/tests/TracingTest.cpp | 137 ++++++++++++++++++ 7 files changed, 214 insertions(+), 135 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt index f3b52123241344..936bcac115cabd 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostInspectorTarget.kt +++ b/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( diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp index 507c0f54470af5..4db322c8dab69e 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp +++ b/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 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 frameTimingSequence) { - inspectorTarget_->recordFrameTimings({ + inspectorTarget().recordFrameTimings({ frameTimingSequence->getId(), frameTimingSequence->getThreadId(), frameTimingSequence->getBeginDrawingTimestamp(), diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h index 9e7c6fc67aedef..032c96f32ee309 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h @@ -240,7 +240,7 @@ class JReactHostInspectorTarget : public jni::HybridClass executor) override; - std::optional - unstable_getHostTracingProfileThatWillBeEmittedOnInitialization() override; jsinspector_modern::HostTargetTracingDelegate *getTracingDelegate() override; private: @@ -286,6 +284,13 @@ class JReactHostInspectorTarget : public jni::HybridClass jobj, jni::alias_ref reactHostImpl, jni::alias_ref 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 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 inspectorTarget_; std::optional 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 stashedTracingProfile_; /** * Encapsulates the logic around tracing for this HostInspectorTarget. */ diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp index 888d27936e90ee..172ff290ff798e 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp +++ b/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, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp index fae9d52605ff6a..7934de96289ac6 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp +++ b/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 diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h index b1640f31a47383..c4ee8bb1e6d59d 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h +++ b/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 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 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 * Should only be allocated when there is an active tracing session. */ std::unique_ptr 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 stashedTracingProfile_; /** * Protects the state inside traceRecording_. * @@ -429,6 +422,12 @@ class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis */ 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). diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/TracingTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/TracingTest.cpp index b04290604c9a46..f21d61f0658318 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/TracingTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/TracingTest.cpp @@ -185,4 +185,141 @@ TEST_F(TracingTest, BackgroundTracingIsRejectedWhileCDPTracingIsRunning) { page_->stopTracing(); } +TEST_F(TracingTest, EmitsToFirstEligibleSessionWhenMultipleSessionsEnabled) { + auto secondaryFusebox = this->connectSecondary(); + auto secondaryNonFusebox = this->connectSecondary(); + + // Enable ReactNativeApplication domain on primary and secondaryFusebox + // sessions (but NOT on secondaryNonFusebox) + { + InSequence s; + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.metadataUpdated")))); + EXPECT_CALL( + this->fromPage(), onMessage(JsonEq(R"({"id": 1, "result": {}})"))); + } + this->toPage_->sendMessage( + R"({"id": 1, "method": "ReactNativeApplication.enable"})"); + + { + InSequence s; + EXPECT_CALL( + secondaryFusebox.fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.metadataUpdated")))); + EXPECT_CALL( + secondaryFusebox.fromPage(), + onMessage(JsonEq(R"({"id": 1, "result": {}})"))); + } + secondaryFusebox.toPage().sendMessage( + R"({"id": 1, "method": "ReactNativeApplication.enable"})"); + + // Start background tracing + this->page_->startTracing( + tracing::Mode::Background, {tracing::Category::Timeline}); + + // Record some frame timings + auto now = HighResTimeStamp::now(); + this->page_->recordFrameTimings( + tracing::FrameTimingSequence( + 1, // id + 11, // threadId + now, + now + HighResDuration::fromNanoseconds(10), + now + HighResDuration::fromNanoseconds(50))); + + // Primary session should receive the trace (first eligible session). + // Events within a session are ordered. + Sequence primarySeq; + + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.traceRequested")))) + .InSequence(primarySeq); + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed(AtJsonPtr("/method", "Tracing.dataCollected")))) + .Times(AtLeast(1)) + .InSequence(primarySeq); + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed(AtJsonPtr("/method", "Tracing.tracingComplete")))) + .InSequence(primarySeq); + + // secondaryFusebox and secondaryNonFusebox should NOT receive anything + // (only first eligible session receives the trace) + EXPECT_CALL(secondaryFusebox.fromPage(), onMessage(_)).Times(0); + EXPECT_CALL(secondaryNonFusebox.fromPage(), onMessage(_)).Times(0); + + // Stop tracing and emit to first eligible session + EXPECT_TRUE(this->page_->stopAndMaybeEmitBackgroundTrace()); +} + +TEST_F(TracingTest, StashedTraceIsEmittedOnlyToFirstEligibleSession) { + // Start background tracing with no sessions having ReactNativeApplication + // enabled + this->page_->startTracing( + tracing::Mode::Background, {tracing::Category::Timeline}); + + // Record some frame timings + auto now = HighResTimeStamp::now(); + this->page_->recordFrameTimings( + tracing::FrameTimingSequence( + 1, // id + 11, // threadId + now, + now + HighResDuration::fromNanoseconds(10), + now + HighResDuration::fromNanoseconds(50))); + + // Stop tracing - no eligible sessions exist, so the trace is stashed + EXPECT_FALSE(this->page_->stopAndMaybeEmitBackgroundTrace()); + + // Now the primary session enables ReactNativeApplication - it should receive + // the stashed trace. Events within a session are ordered. + Sequence primarySeq; + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.metadataUpdated")))) + .InSequence(primarySeq); + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.traceRequested")))) + .InSequence(primarySeq); + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed(AtJsonPtr("/method", "Tracing.dataCollected")))) + .Times(AtLeast(1)) + .InSequence(primarySeq); + EXPECT_CALL( + this->fromPage(), + onMessage(JsonParsed(AtJsonPtr("/method", "Tracing.tracingComplete")))) + .InSequence(primarySeq); + EXPECT_CALL(this->fromPage(), onMessage(JsonEq(R"({"id": 1, "result": {}})"))) + .InSequence(primarySeq); + this->toPage_->sendMessage( + R"({"id": 1, "method": "ReactNativeApplication.enable"})"); + + // Connect a secondary session and enable ReactNativeApplication - it should + // NOT receive the already-emitted stashed trace + auto secondary = this->connectSecondary(); + Sequence secondarySeq; + EXPECT_CALL( + secondary.fromPage(), + onMessage(JsonParsed( + AtJsonPtr("/method", "ReactNativeApplication.metadataUpdated")))) + .InSequence(secondarySeq); + // No traceRequested, dataCollected, or tracingComplete expected for + // secondary + EXPECT_CALL( + secondary.fromPage(), onMessage(JsonEq(R"({"id": 1, "result": {}})"))) + .InSequence(secondarySeq); + secondary.toPage().sendMessage( + R"({"id": 1, "method": "ReactNativeApplication.enable"})"); +} + } // namespace facebook::react::jsinspector_modern