From f6123aed95124adabd3ace35fab2528beda2ff16 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Fri, 8 Mar 2024 09:25:33 -0800 Subject: [PATCH] Update InspectorFlags to lazily pull upstream values (#43390) Summary: Refactor after D54639775. This avoids the unfortunate side effect where `InspectorFlags::dangerouslyResetFlags()` would immediately reread `ReactNativeFeatureFlags `. This call is now relocated in our test util. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D54684692 --- .../jsinspector-modern/InspectorFlags.cpp | 60 +++++++++---------- .../jsinspector-modern/InspectorFlags.h | 17 ++++-- .../utils/InspectorFlagOverridesGuard.cpp | 2 +- 3 files changed, 40 insertions(+), 39 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp index b473ee70e2a4..22c29f84e276 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp @@ -17,55 +17,51 @@ InspectorFlags& InspectorFlags::getInstance() { return instance; } -InspectorFlags::InspectorFlags() - : enableModernCDPRegistry_( - ReactNativeFeatureFlags::inspectorEnableModernCDPRegistry()), - enableCxxInspectorPackagerConnection_( - ReactNativeFeatureFlags:: - inspectorEnableCxxInspectorPackagerConnection()), - enableHermesCDPAgent_( - ReactNativeFeatureFlags::inspectorEnableHermesCDPAgent()) {} - bool InspectorFlags::getEnableModernCDPRegistry() const { - assertFlagsMatchUpstream(); - return enableModernCDPRegistry_; + return loadFlagsAndAssertUnchanged().enableModernCDPRegistry; } bool InspectorFlags::getEnableCxxInspectorPackagerConnection() const { - assertFlagsMatchUpstream(); - return enableCxxInspectorPackagerConnection_ || + auto& values = loadFlagsAndAssertUnchanged(); + + return values.enableCxxInspectorPackagerConnection || // If we are using the modern CDP registry, then we must also use the C++ // InspectorPackagerConnection implementation. - enableModernCDPRegistry_; + values.enableModernCDPRegistry; } bool InspectorFlags::getEnableHermesCDPAgent() const { - assertFlagsMatchUpstream(); - return enableHermesCDPAgent_; + return loadFlagsAndAssertUnchanged().enableHermesCDPAgent; } void InspectorFlags::dangerouslyResetFlags() { *this = InspectorFlags{}; } -void InspectorFlags::assertFlagsMatchUpstream() const { - if (inconsistentFlagsStateLogged_) { - return; - } +const InspectorFlags::Values& InspectorFlags::loadFlagsAndAssertUnchanged() + const { + InspectorFlags::Values newValues = { + .enableCxxInspectorPackagerConnection = ReactNativeFeatureFlags:: + inspectorEnableCxxInspectorPackagerConnection(), + .enableHermesCDPAgent = + ReactNativeFeatureFlags::inspectorEnableHermesCDPAgent(), + .enableModernCDPRegistry = + ReactNativeFeatureFlags::inspectorEnableModernCDPRegistry(), + }; - if (enableModernCDPRegistry_ != - ReactNativeFeatureFlags::inspectorEnableModernCDPRegistry() || - enableCxxInspectorPackagerConnection_ != - ReactNativeFeatureFlags:: - inspectorEnableCxxInspectorPackagerConnection() || - ReactNativeFeatureFlags::inspectorEnableHermesCDPAgent() != - enableHermesCDPAgent_) { - LOG(ERROR) - << "[InspectorFlags] Error: One or more ReactNativeFeatureFlags values " - << "have changed during the global app lifetime. This may lead to " - << "inconsistent inspector behaviour. Please quit and restart the app."; - inconsistentFlagsStateLogged_ = true; + if (cachedValues_.has_value() && !inconsistentFlagsStateLogged_) { + if (cachedValues_ != newValues) { + LOG(ERROR) + << "[InspectorFlags] Error: One or more ReactNativeFeatureFlags values " + << "have changed during the global app lifetime. This may lead to " + << "inconsistent inspector behaviour. Please quit and restart the app."; + inconsistentFlagsStateLogged_ = true; + } } + + cachedValues_ = newValues; + + return cachedValues_.value(); } } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h index 249ae167dd5e..775ee2b5844b 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h @@ -42,17 +42,22 @@ class InspectorFlags { void dangerouslyResetFlags(); private: - InspectorFlags(); + struct Values { + bool enableCxxInspectorPackagerConnection; + bool enableHermesCDPAgent; + bool enableModernCDPRegistry; + bool operator==(const Values&) const = default; + }; + + InspectorFlags() = default; InspectorFlags(const InspectorFlags&) = delete; InspectorFlags& operator=(const InspectorFlags&) = default; ~InspectorFlags() = default; - bool enableModernCDPRegistry_; - bool enableCxxInspectorPackagerConnection_; - bool enableHermesCDPAgent_; - + mutable std::optional cachedValues_; mutable bool inconsistentFlagsStateLogged_{false}; - void assertFlagsMatchUpstream() const; + + const Values& loadFlagsAndAssertUnchanged() const; }; } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/utils/InspectorFlagOverridesGuard.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/utils/InspectorFlagOverridesGuard.cpp index aa1b87b8f57f..cc5caf11953a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/utils/InspectorFlagOverridesGuard.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/utils/InspectorFlagOverridesGuard.cpp @@ -40,12 +40,12 @@ class ReactNativeFeatureFlagsOverrides InspectorFlagOverridesGuard::InspectorFlagOverridesGuard( const InspectorFlagOverrides& overrides) { + InspectorFlags::getInstance().dangerouslyResetFlags(); ReactNativeFeatureFlags::override( std::make_unique(overrides)); } InspectorFlagOverridesGuard::~InspectorFlagOverridesGuard() { - InspectorFlags::getInstance().dangerouslyResetFlags(); ReactNativeFeatureFlags::dangerouslyReset(); }