From 68a0e352f7b92f3540ebc0b285a902ddd5a1e395 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 13 Mar 2024 04:44:07 -0700 Subject: [PATCH 1/2] [skip ci] Enable CDP flags, enable Bridgeless in RNTester Differential Revision: D54846939 --- .../featureflags/ReactNativeFeatureFlagsDefaults.kt | 6 +++--- .../react/featureflags/ReactNativeFeatureFlagsDefaults.h | 6 +++--- .../scripts/featureflags/ReactNativeFeatureFlags.config.js | 4 ++-- .../src/private/featureflags/ReactNativeFeatureFlags.js | 6 +++--- .../java/com/facebook/react/uiapp/RNTesterApplication.kt | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt index 12fbeaeb870f..b0ee67293157 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<<4fda339ba0682c3d729725e20a016d27>> */ /** @@ -41,9 +41,9 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi override fun inspectorEnableCxxInspectorPackagerConnection(): Boolean = false - override fun inspectorEnableHermesCDPAgent(): Boolean = false + override fun inspectorEnableHermesCDPAgent(): Boolean = true - override fun inspectorEnableModernCDPRegistry(): Boolean = false + override fun inspectorEnableModernCDPRegistry(): Boolean = true override fun skipMountHookNotifications(): Boolean = false diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsDefaults.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsDefaults.h index 9086c75f6b03..5eb286363f98 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsDefaults.h +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsDefaults.h @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<> */ /** @@ -64,11 +64,11 @@ class ReactNativeFeatureFlagsDefaults : public ReactNativeFeatureFlagsProvider { } bool inspectorEnableHermesCDPAgent() override { - return false; + return true; } bool inspectorEnableModernCDPRegistry() override { - return false; + return true; } bool skipMountHookNotifications() override { diff --git a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js index ef84a26cf9d1..8d4db84745cb 100644 --- a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js +++ b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js @@ -73,12 +73,12 @@ const definitions: FeatureFlagDefinitions = { 'Flag determining if the C++ implementation of InspectorPackagerConnection should be used instead of the per-platform one. This flag is global and should not be changed across React Host lifetimes.', }, inspectorEnableHermesCDPAgent: { - defaultValue: false, + defaultValue: true, description: 'Flag determining if the new Hermes CDPAgent API should be enabled in the modern CDP backend. This flag is global and should not be changed across React Host lifetimes.', }, inspectorEnableModernCDPRegistry: { - defaultValue: false, + defaultValue: true, description: 'Flag determining if the modern CDP backend should be enabled. This flag is global and should not be changed across React Host lifetimes.', }, diff --git a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js index f28b03b9bfac..9485be52a231 100644 --- a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js +++ b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<66678139be02293ff532e8e125ccc608>> + * @generated SignedSource<> * @flow strict-local */ @@ -134,11 +134,11 @@ export const inspectorEnableCxxInspectorPackagerConnection: Getter = cr /** * Flag determining if the new Hermes CDPAgent API should be enabled in the modern CDP backend. This flag is global and should not be changed across React Host lifetimes. */ -export const inspectorEnableHermesCDPAgent: Getter = createNativeFlagGetter('inspectorEnableHermesCDPAgent', false); +export const inspectorEnableHermesCDPAgent: Getter = createNativeFlagGetter('inspectorEnableHermesCDPAgent', true); /** * Flag determining if the modern CDP backend should be enabled. This flag is global and should not be changed across React Host lifetimes. */ -export const inspectorEnableModernCDPRegistry: Getter = createNativeFlagGetter('inspectorEnableModernCDPRegistry', false); +export const inspectorEnableModernCDPRegistry: Getter = createNativeFlagGetter('inspectorEnableModernCDPRegistry', true); /** * This is a temporary flag to disable part of the mount hooks pipeline to investigate a crash. */ diff --git a/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/RNTesterApplication.kt b/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/RNTesterApplication.kt index 01756aea9821..1d1f20952410 100644 --- a/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/RNTesterApplication.kt +++ b/packages/rn-tester/android/app/src/main/java/com/facebook/react/uiapp/RNTesterApplication.kt @@ -133,7 +133,7 @@ class RNTesterApplication : Application(), ReactApplication { super.onCreate() SoLoader.init(this, /* native exopackage */ false) if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED) { - load() + load(bridgelessEnabled = false) } } } From a28f40496712c211c7929d7d51657871112738f4 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 13 Mar 2024 04:44:07 -0700 Subject: [PATCH 2/2] Integrate Hermes CDPDebugAPI with RN console polyfill (#43454) Summary: Changelog: [Internal] 1. Introduces the `RuntimeTargetDelegate::addConsoleMessage` method, which is a 1:1 wrapper around the Hermes `CDPDebugAPI` [method of the same name](https://github.com/facebook/hermes/blob/33cf8cfe781aabcad6b3d39821d815fe5317f977/API/hermes/cdp/CDPDebugAPI.h#L43-L47). 2. Installs a global `__inspectorLog` function callable from JS, matching [this existing call](https://github.com/facebook/react-native/blob/4f10f3069fff9090d700d9bcfbf49da1aa85f272/packages/polyfills/console.js#L430-L437) in React Native's `console` polyfill. NOTE: We'll almost immediately replace `__inspectorLog` with a standalone native implementation of the `console` API that doesn't depend on the polyfill, but this is an easy way to validate the approach before tackling the full `console` spec, which requires some more C++ code and boilerplate. Differential Revision: D54494298 --- .../chrome/HermesRuntimeTargetDelegate.cpp | 65 +++++++++++++++++ .../chrome/HermesRuntimeTargetDelegate.h | 3 + .../jsinspector-modern/ConsoleMessage.h | 53 ++++++++++++++ .../FallbackRuntimeTargetDelegate.cpp | 6 ++ .../FallbackRuntimeTargetDelegate.h | 3 + .../jsinspector-modern/RuntimeTarget.cpp | 73 +++++++++++++++++++ .../jsinspector-modern/RuntimeTarget.h | 27 +++++++ .../jsinspector-modern/tests/InspectorMocks.h | 5 ++ .../tests/ReactInstanceIntegrationTest.cpp | 15 ++-- 9 files changed, 244 insertions(+), 6 deletions(-) create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/ConsoleMessage.h diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp index 9c5542b08ebe..c73867a0fdbf 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp @@ -73,6 +73,65 @@ class HermesRuntimeTargetDelegate::Impl final : public RuntimeTargetDelegate { std::move(runtimeExecutor))); } + void addConsoleMessage(jsi::Runtime& /*unused*/, ConsoleMessage message) + override { + using HermesConsoleMessage = facebook::hermes::cdp::ConsoleMessage; + using HermesConsoleAPIType = facebook::hermes::cdp::ConsoleAPIType; + + HermesConsoleAPIType type{}; + switch (message.type) { + case ConsoleAPIType::kLog: + type = HermesConsoleAPIType::kLog; + break; + case ConsoleAPIType::kDebug: + type = HermesConsoleAPIType::kDebug; + break; + case ConsoleAPIType::kInfo: + type = HermesConsoleAPIType::kInfo; + break; + case ConsoleAPIType::kError: + type = HermesConsoleAPIType::kError; + break; + case ConsoleAPIType::kWarning: + type = HermesConsoleAPIType::kWarning; + break; + case ConsoleAPIType::kDir: + type = HermesConsoleAPIType::kDir; + break; + case ConsoleAPIType::kDirXML: + type = HermesConsoleAPIType::kDirXML; + break; + case ConsoleAPIType::kTable: + type = HermesConsoleAPIType::kTable; + break; + case ConsoleAPIType::kTrace: + type = HermesConsoleAPIType::kTrace; + break; + case ConsoleAPIType::kStartGroup: + type = HermesConsoleAPIType::kStartGroup; + break; + case ConsoleAPIType::kStartGroupCollapsed: + type = HermesConsoleAPIType::kStartGroupCollapsed; + break; + case ConsoleAPIType::kEndGroup: + type = HermesConsoleAPIType::kEndGroup; + break; + case ConsoleAPIType::kClear: + type = HermesConsoleAPIType::kClear; + break; + case ConsoleAPIType::kAssert: + type = HermesConsoleAPIType::kAssert; + break; + case ConsoleAPIType::kTimeEnd: + type = HermesConsoleAPIType::kTimeEnd; + break; + default: + throw std::logic_error{"Unknown console message type"}; + } + cdpDebugAPI_->addConsoleMessage( + HermesConsoleMessage{message.timestamp, type, std::move(message.args)}); + } + private: HermesRuntimeTargetDelegate& delegate_; std::shared_ptr runtime_; @@ -118,6 +177,12 @@ HermesRuntimeTargetDelegate::createAgentDelegate( std::move(runtimeExecutor)); } +void HermesRuntimeTargetDelegate::addConsoleMessage( + jsi::Runtime& runtime, + ConsoleMessage message) { + impl_->addConsoleMessage(runtime, std::move(message)); +} + #ifdef HERMES_ENABLE_DEBUGGER CDPDebugAPI& HermesRuntimeTargetDelegate::getCDPDebugAPI() { return impl_->getCDPDebugAPI(); diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.h b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.h index 2196568c0997..7182a82a8c30 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.h +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.h @@ -45,6 +45,9 @@ class HermesRuntimeTargetDelegate : public RuntimeTargetDelegate { executionContextDescription, RuntimeExecutor runtimeExecutor) override; + void addConsoleMessage(jsi::Runtime& runtime, ConsoleMessage message) + override; + private: // We use the private implementation idiom to ensure this class has the same // layout regardless of whether HERMES_ENABLE_DEBUGGER is defined. The net diff --git a/packages/react-native/ReactCommon/jsinspector-modern/ConsoleMessage.h b/packages/react-native/ReactCommon/jsinspector-modern/ConsoleMessage.h new file mode 100644 index 000000000000..4ca5b1c66582 --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/ConsoleMessage.h @@ -0,0 +1,53 @@ +/* + * 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 + +#include + +namespace facebook::react::jsinspector_modern { + +enum class ConsoleAPIType { + kLog, + kDebug, + kInfo, + kError, + kWarning, + kDir, + kDirXML, + kTable, + kTrace, + kStartGroup, + kStartGroupCollapsed, + kEndGroup, + kClear, + kAssert, + kTimeEnd, + kCount +}; + +struct ConsoleMessage { + double timestamp; + ConsoleAPIType type; + std::vector args; + + ConsoleMessage( + double timestamp, + ConsoleAPIType type, + std::vector args) + : timestamp(timestamp), type(type), args(std::move(args)) {} + + ConsoleMessage(const ConsoleMessage& other) = delete; + ConsoleMessage(ConsoleMessage&& other) noexcept = default; + ConsoleMessage& operator=(const ConsoleMessage& other) = delete; + ConsoleMessage& operator=(ConsoleMessage&& other) noexcept = default; + ~ConsoleMessage() = default; +}; + +} // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.cpp b/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.cpp index cd2cb9ab44f7..f7d36d415f93 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.cpp @@ -26,4 +26,10 @@ FallbackRuntimeTargetDelegate::createAgentDelegate( std::move(channel), sessionState, engineDescription_); } +void FallbackRuntimeTargetDelegate::addConsoleMessage( + jsi::Runtime& /*unused*/, + ConsoleMessage /*unused*/) { + // TODO: Best-effort printing (without RemoteObjects) +} + } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.h b/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.h index 923a0d301ee0..fa859f0fceda 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/FallbackRuntimeTargetDelegate.h @@ -31,6 +31,9 @@ class FallbackRuntimeTargetDelegate : public RuntimeTargetDelegate { const ExecutionContextDescription& executionContextDescription, RuntimeExecutor runtimeExecutor) override; + void addConsoleMessage(jsi::Runtime& runtime, ConsoleMessage message) + override; + private: std::string engineDescription_; }; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp index 44397a8ae648..797d79de65fb 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp @@ -21,6 +21,7 @@ std::shared_ptr RuntimeTarget::create( std::shared_ptr runtimeTarget{ new RuntimeTarget(executionContextDescription, delegate, jsExecutor)}; runtimeTarget->setExecutor(selfExecutor); + runtimeTarget->installGlobals(); return runtimeTarget; } @@ -32,6 +33,78 @@ RuntimeTarget::RuntimeTarget( delegate_(delegate), jsExecutor_(jsExecutor) {} +void RuntimeTarget::installGlobals() { + installConsoleHandler(); +} + +void RuntimeTarget::installConsoleHandler() { + jsExecutor_([selfWeak = weak_from_this(), + selfExecutor = executorFromThis()](jsi::Runtime& runtime) { + // TODO(moti): Switch from implementing __inspectorLog to directly + // installing a `console` object. + runtime.global().setProperty( + runtime, + "__inspectorLog", + jsi::Function::createFromHostFunction( + runtime, + jsi::PropNameID::forAscii(runtime, "__inspectorLog"), + 4, + [selfWeak, selfExecutor]( + jsi::Runtime& rt, + const jsi::Value& /*thisVal*/, + const jsi::Value* args, + size_t count) { + if (count < 4) { + throw jsi::JSError( + rt, + "__inspectorLog requires at least 4 arguments: logLevel, str, args, framesToSkip"); + } + std::chrono::time_point timestamp = + std::chrono::system_clock::now(); + std::string level = args[0].asString(rt).utf8(rt); + ConsoleAPIType type = ConsoleAPIType::kLog; + if (level == "debug") { + type = ConsoleAPIType::kDebug; + } else if (level == "log") { + type = ConsoleAPIType::kLog; + } else if (level == "warning") { + type = ConsoleAPIType::kWarning; + } else if (level == "error") { + type = ConsoleAPIType::kError; + } + // NOTE: args[1] is the processed string message - ignore it. + jsi::Array argsArray = args[2].asObject(rt).asArray(rt); + std::vector argsVec; + for (size_t i = 0, length = argsArray.length(rt); i != length; + ++i) { + argsVec.emplace_back(argsArray.getValueAtIndex(rt, i)); + } + // TODO(moti): Handle framesToSkip in some way. Note that the + // runtime doesn't even capture a stack trace at the moment. + ConsoleMessage consoleMessage{ + std::chrono::duration_cast< + std::chrono::duration>( + timestamp.time_since_epoch()) + .count(), + type, + std::move(argsVec)}; + if (auto self = selfWeak.lock()) { + // Q: Why is it safe to use self->delegate_ here? + // A: Because the caller of InspectorTarget::registerRuntime + // is explicitly required to guarantee that the delegate not + // only outlives the target, but also outlives all JS code + // execution that occurs on the JS thread. + self->delegate_.addConsoleMessage( + rt, std::move(consoleMessage)); + // To ensure we never destroy `self` on the JS thread, send + // our shared_ptr back to the inspector thread. + selfExecutor([self = std::move(self)](auto&) { (void)self; }); + } + return jsi::Value::undefined(); + })); + }); +} + std::shared_ptr RuntimeTarget::createAgent( FrontendChannel channel, SessionState& sessionState) { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h index 59091730b200..a5f42b8fa1f9 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h @@ -9,6 +9,7 @@ #include +#include "ConsoleMessage.h" #include "ExecutionContext.h" #include "InspectorInterfaces.h" #include "RuntimeAgent.h" @@ -51,6 +52,21 @@ class RuntimeTargetDelegate { previouslyExportedState, const ExecutionContextDescription& executionContextDescription, RuntimeExecutor runtimeExecutor) = 0; + + /** + * Called when the runtime intercepts a console API call. The target delegate + * should notify the frontend (via its agent delegates) of the message, and + * perform any buffering required for logging the message later (in the + * existing and/or new sessions). + * + * \note The method is called on the JS thread, and receives a valid reference + * to the current \c jsi::Runtime. The callee MAY use its own intrinsic + * Runtime reference, if it has one, without checking it for equivalence with + * the one provided here. + */ + virtual void addConsoleMessage( + jsi::Runtime& runtime, + ConsoleMessage message) = 0; }; /** @@ -156,6 +172,17 @@ class JSINSPECTOR_EXPORT RuntimeTarget */ void installBindingHandler(const std::string& bindingName); + /** + * Installs any global values we want to expose to framework/user JavaScript + * code. + */ + void installGlobals(); + + /** + * Install the console API handler. + */ + void installConsoleHandler(); + // Necessary to allow RuntimeAgent to access RuntimeTarget's internals in a // controlled way (i.e. only RuntimeTargetController gets friend access, while // RuntimeAgent itself doesn't). diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h index 66f8d7bdf6a7..f0b73ae9dcaf 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h @@ -136,6 +136,11 @@ class MockRuntimeTargetDelegate : public RuntimeTargetDelegate { const ExecutionContextDescription&, RuntimeExecutor), (override)); + MOCK_METHOD( + void, + addConsoleMessage, + (jsi::Runtime & runtime, ConsoleMessage message), + (override)); }; class MockRuntimeAgentDelegate : public RuntimeAgentDelegate { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp index 2e0f5c209b12..b4442144f725 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp @@ -131,7 +131,11 @@ void ReactInstanceIntegrationTest::initializeRuntime(std::string_view script) { react::ReactInstance::JSRuntimeFlags flags{ .isProfiling = false, }; - instance->initializeRuntime(flags, [](jsi::Runtime&) {}); + instance->initializeRuntime(flags, [](jsi::Runtime& rt) { + // NOTE: RN's console polyfill (included in prelude.js.h) depends on the + // native logging hook being installed, even if it's a noop. + facebook::react::bindNativeLogger(rt, [](auto, auto) {}); + }); messageQueueThread->tick(); @@ -199,11 +203,10 @@ TEST_P(ReactInstanceIntegrationTestWithFlags, ConsoleLog) { InSequence s; - // Hermes console.* interception is currently explicitly disabled under the - // modern registry, and the runtime does not yet fire these events. When the - // implementation is more complete we should be able to remove this - // condition. - if (!InspectorFlags::getInstance().getEnableModernCDPRegistry()) { + // Hermes console.* interception is explicitly disabled under CDPHandler, + // but Hermes CDPAgent and the legacy RN backend should both work. + if (!InspectorFlags::getInstance().getEnableModernCDPRegistry() || + InspectorFlags::getInstance().getEnableHermesCDPAgent()) { EXPECT_CALL( getRemoteConnection(), onMessage(JsonParsed(AllOf(