From 828ad04cefd2ccb33bc22c3008051692e7775b86 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Fri, 16 Feb 2024 11:16:41 -0800 Subject: [PATCH] Assign auto-incrementing execution context IDs (#43064) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43064 Changelog: [Internal] Aligns React Native's CDP backend with V8's behaviour of assigning a sequential ID (here unique within a given PageTarget) to each execution context. Reviewed By: huntie Differential Revision: D53776531 fbshipit-source-id: 950599c323f416e8180e42281d94ae9c00f15fb0 --- .../ExecutionContextManager.cpp | 19 ++++++++++++++ .../ExecutionContextManager.h | 25 +++++++++++++++++++ .../jsinspector-modern/InstanceTarget.cpp | 14 +++++++---- .../jsinspector-modern/InstanceTarget.h | 9 ++++++- .../jsinspector-modern/PageTarget.cpp | 8 +++--- .../jsinspector-modern/PageTarget.h | 5 ++++ .../tests/JsiIntegrationTest.cpp | 9 +++---- 7 files changed, 74 insertions(+), 15 deletions(-) create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.cpp create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.h diff --git a/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.cpp b/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.cpp new file mode 100644 index 00000000000..1334473d867 --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.cpp @@ -0,0 +1,19 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include "ExecutionContextManager.h" + +#include + +namespace facebook::react::jsinspector_modern { + +int32_t ExecutionContextManager::allocateExecutionContextId() { + assert(nextExecutionContextId_ != INT32_MAX); + return nextExecutionContextId_++; +} + +} // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.h b/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.h new file mode 100644 index 00000000000..1e1c4210b52 --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/ExecutionContextManager.h @@ -0,0 +1,25 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include + +namespace facebook::react::jsinspector_modern { + +/** + * Generates unique execution context IDs. + */ +class ExecutionContextManager { + public: + int32_t allocateExecutionContextId(); + + private: + int32_t nextExecutionContextId_{1}; +}; + +} // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp index e98389292ca..e128c689ed6 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp @@ -13,15 +13,20 @@ namespace facebook::react::jsinspector_modern { std::shared_ptr InstanceTarget::create( + std::shared_ptr executionContextManager, InstanceTargetDelegate& delegate, VoidExecutor executor) { - std::shared_ptr instanceTarget{new InstanceTarget(delegate)}; + std::shared_ptr instanceTarget{ + new InstanceTarget(executionContextManager, delegate)}; instanceTarget->setExecutor(executor); return instanceTarget; } -InstanceTarget::InstanceTarget(InstanceTargetDelegate& delegate) - : delegate_(delegate) { +InstanceTarget::InstanceTarget( + std::shared_ptr executionContextManager, + InstanceTargetDelegate& delegate) + : delegate_(delegate), + executionContextManager_(std::move(executionContextManager)) { (void)delegate_; } @@ -51,8 +56,7 @@ RuntimeTarget& InstanceTarget::registerRuntime( assert(!currentRuntime_ && "Only one Runtime allowed"); currentRuntime_ = RuntimeTarget::create( ExecutionContextDescription{ - // TODO: IDs should be unique within the current Page. - .id = 1, + .id = executionContextManager_->allocateExecutionContextId(), .origin = "", .name = "main", .uniqueId = std::nullopt}, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h index a96197e4574..101433c8b27 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h @@ -7,6 +7,7 @@ #pragma once +#include "ExecutionContextManager.h" #include "RuntimeTarget.h" #include "ScopedExecutor.h" #include "SessionState.h" @@ -46,6 +47,7 @@ class InstanceTarget : public EnableExecutorFromThis { public: /** * Constructs a new InstanceTarget. + * \param executionContextManager Assigns unique execution context IDs. * \param delegate The object that will receive events from this target. * The caller is responsible for ensuring that the delegate outlives this * object. @@ -54,6 +56,7 @@ class InstanceTarget : public EnableExecutorFromThis { * executor will not be called after the InstanceTarget is destroyed. */ static std::shared_ptr create( + std::shared_ptr executionContextManager, InstanceTargetDelegate& delegate, VoidExecutor executor); @@ -76,15 +79,19 @@ class InstanceTarget : public EnableExecutorFromThis { /** * Constructs a new InstanceTarget. The caller must call setExecutor * immediately afterwards. + * \param executionContextManager Assigns unique execution context IDs. * \param delegate The object that will receive events from this target. * The caller is responsible for ensuring that the delegate outlives this * object. */ - InstanceTarget(InstanceTargetDelegate& delegate); + InstanceTarget( + std::shared_ptr executionContextManager, + InstanceTargetDelegate& delegate); InstanceTargetDelegate& delegate_; std::shared_ptr currentRuntime_{nullptr}; WeakList agents_; + std::shared_ptr executionContextManager_; }; } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp index 78167b16658..86db8b8f400 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp @@ -107,7 +107,9 @@ std::shared_ptr PageTarget::create( return pageTarget; } -PageTarget::PageTarget(PageTargetDelegate& delegate) : delegate_(delegate) {} +PageTarget::PageTarget(PageTargetDelegate& delegate) + : delegate_(delegate), + executionContextManager_{std::make_shared()} {} std::unique_ptr PageTarget::connect( std::unique_ptr connectionToFrontend, @@ -132,8 +134,8 @@ PageTargetDelegate::~PageTargetDelegate() {} InstanceTarget& PageTarget::registerInstance(InstanceTargetDelegate& delegate) { assert(!currentInstance_ && "Only one instance allowed"); - currentInstance_ = - InstanceTarget::create(delegate, makeVoidExecutor(executorFromThis())); + currentInstance_ = InstanceTarget::create( + executionContextManager_, delegate, makeVoidExecutor(executorFromThis())); sessions_.forEach( [currentInstance = &*currentInstance_](PageTargetSession& session) { session.setCurrentInstance(currentInstance); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h index 90c6513ba70..d48a6a7d39e 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h @@ -7,6 +7,7 @@ #pragma once +#include "ExecutionContextManager.h" #include "ScopedExecutor.h" #include "WeakList.h" @@ -166,6 +167,10 @@ class JSINSPECTOR_EXPORT PageTarget PageTargetDelegate& delegate_; WeakList sessions_; PageTargetController controller_{*this}; + // executionContextManager_ is a shared_ptr to guarantee its validity while + // the InstanceTarget is alive (just in case the InstanceTarget ends up + // briefly outliving the PageTarget, which it generally shouldn't). + std::shared_ptr executionContextManager_; std::shared_ptr currentInstance_{nullptr}; inline PageTargetDelegate& getDelegate() { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp index 825f37ad152..ebe9a6b3910 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp @@ -211,12 +211,11 @@ TYPED_TEST(JsiIntegrationPortableTest, ExecutionContextNotifications) { })"))) .RetiresOnSaturation(); - // TODO: Each new execution context should receive a new ID. EXPECT_CALL(this->fromPage(), onMessage(JsonEq(R"({ "method": "Runtime.executionContextCreated", "params": { "context": { - "id": 1, + "id": 2, "origin": "", "name": "main" } @@ -226,11 +225,10 @@ TYPED_TEST(JsiIntegrationPortableTest, ExecutionContextNotifications) { // Simulate a reload triggered by the app (not by the debugger). this->reload(); - // TODO: Each new execution context should receive a new ID. EXPECT_CALL(this->fromPage(), onMessage(JsonEq(R"({ "method": "Runtime.executionContextDestroyed", "params": { - "executionContextId": 1 + "executionContextId": 2 } })"))) .RetiresOnSaturation(); @@ -239,12 +237,11 @@ TYPED_TEST(JsiIntegrationPortableTest, ExecutionContextNotifications) { "method": "Runtime.executionContextsCleared" })"))) .RetiresOnSaturation(); - // TODO: Each new execution context should receive a new ID. EXPECT_CALL(this->fromPage(), onMessage(JsonEq(R"({ "method": "Runtime.executionContextCreated", "params": { "context": { - "id": 1, + "id": 3, "origin": "", "name": "main" }