From c0e84ece4f079bbf5b167eb4f66da5bf15f37377 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Fri, 15 Mar 2024 03:43:28 -0700 Subject: [PATCH] Add regression test for Hermes CDP + lazy compilation bug (#43461) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43461 Adds a regression test for the duplicate `scriptParsed` bug in Hermes (T182003727). In the process, we enable Hermes lazy compilation in all our CDPAgent JSI integration tests. Changelog: [Internal] Reviewed By: huntie Differential Revision: D54852326 fbshipit-source-id: 52e23458d3e9e21902c3659fc944ef85c68731ac --- .../tests/JsiIntegrationTest.cpp | 37 ++++++++++++++----- ...ionTestHermesWithCDPAgentEngineAdapter.cpp | 33 ++++++++++++++++- ...ationTestHermesWithCDPAgentEngineAdapter.h | 24 ++++++++++-- 3 files changed, 81 insertions(+), 13 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp index a4a2e773aa4..748a67c9645 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.cpp @@ -592,15 +592,6 @@ TYPED_TEST(JsiIntegrationHermesModernTest, ResolveBreakpointAfterReload) { this->reload(); - this->expectMessageFromPage(JsonEq(R"({ - "id": 3, - "result": {} - })")); - this->toPage_->sendMessage(R"({ - "id": 3, - "method": "Debugger.enable" - })"); - auto scriptInfo = this->expectMessageFromPage(JsonParsed(AllOf( AtJsonPtr("/method", "Debugger.scriptParsed"), AtJsonPtr("/params/url", "breakpointTest.js")))); @@ -680,6 +671,34 @@ TYPED_TEST(JsiIntegrationHermesModernTest, CDPAgentReentrancyRegressionTest) { }); } +TYPED_TEST(JsiIntegrationHermesModernTest, ScriptParsedExactlyOnce) { + // Regression test for T182003727 (multiple scriptParsed events for a single + // script under Hermes lazy compilation). + + this->connect(); + + InSequence s; + + this->eval(R"( + // NOTE: Triggers lazy compilation in Hermes when running with + // CompilationMode::ForceLazyCompilation. + (function foo(){var x = 2;})() + //# sourceURL=script.js + )"); + + this->expectMessageFromPage(JsonParsed(AllOf( + AtJsonPtr("/method", "Debugger.scriptParsed"), + AtJsonPtr("/params/url", "script.js")))); + this->expectMessageFromPage(JsonEq(R"({ + "id": 1, + "result": {} + })")); + this->toPage_->sendMessage(R"({ + "id": 1, + "method": "Debugger.enable" + })"); +} + #pragma endregion // ModernHermesVariants } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.cpp index 593826f5cce..ed0830b7a71 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.cpp @@ -12,7 +12,13 @@ namespace facebook::react::jsinspector_modern { JsiIntegrationTestHermesWithCDPAgentEngineAdapter:: JsiIntegrationTestHermesWithCDPAgentEngineAdapter( folly::Executor& jsExecutor) - : JsiIntegrationTestHermesEngineAdapter(jsExecutor) {} + : runtime_{hermes::makeHermesRuntime( + ::hermes::vm::RuntimeConfig::Builder() + .withCompilationMode( + ::hermes::vm::CompilationMode::ForceLazyCompilation) + .build())}, + jsExecutor_{jsExecutor}, + runtimeTargetDelegate_{runtime_} {} /* static */ InspectorFlagOverrides JsiIntegrationTestHermesWithCDPAgentEngineAdapter:: @@ -23,4 +29,29 @@ JsiIntegrationTestHermesWithCDPAgentEngineAdapter:: }; } +RuntimeTargetDelegate& +JsiIntegrationTestHermesWithCDPAgentEngineAdapter::getRuntimeTargetDelegate() { + return runtimeTargetDelegate_; +} + +jsi::Runtime& JsiIntegrationTestHermesWithCDPAgentEngineAdapter::getRuntime() + const noexcept { + return *runtime_; +} + +RuntimeExecutor +JsiIntegrationTestHermesWithCDPAgentEngineAdapter::getRuntimeExecutor() + const noexcept { + auto& jsExecutor = jsExecutor_; + return [runtimeWeak = std::weak_ptr(runtime_), &jsExecutor](auto fn) { + jsExecutor.add([runtimeWeak, fn]() { + auto runtime = runtimeWeak.lock(); + if (!runtime) { + return; + } + fn(*runtime); + }); + }; +} + } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.h b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.h index e61f6b82706..3d3524d31a0 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestHermesWithCDPAgentEngineAdapter.h @@ -8,7 +8,15 @@ #pragma once #include "../utils/InspectorFlagOverridesGuard.h" -#include "JsiIntegrationTestHermesEngineAdapter.h" + +#include + +#include +#include +#include +#include + +#include namespace facebook::react::jsinspector_modern { @@ -16,13 +24,23 @@ namespace facebook::react::jsinspector_modern { * An engine adapter for JsiIntegrationTest that uses Hermes (and Hermes's * new CDPAgent API). */ -class JsiIntegrationTestHermesWithCDPAgentEngineAdapter - : public JsiIntegrationTestHermesEngineAdapter { +class JsiIntegrationTestHermesWithCDPAgentEngineAdapter { public: explicit JsiIntegrationTestHermesWithCDPAgentEngineAdapter( folly::Executor& jsExecutor); static InspectorFlagOverrides getInspectorFlagOverrides() noexcept; + + RuntimeTargetDelegate& getRuntimeTargetDelegate(); + + jsi::Runtime& getRuntime() const noexcept; + + RuntimeExecutor getRuntimeExecutor() const noexcept; + + private: + std::shared_ptr runtime_; + folly::Executor& jsExecutor_; + HermesRuntimeTargetDelegate runtimeTargetDelegate_; }; } // namespace facebook::react::jsinspector_modern