From f536f82e12be137114cfc10face6a7dd567820f8 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 13 Aug 2021 13:52:11 -0700 Subject: [PATCH] Warn whenever CxxNativeModules are used Summary: After this diff, when ReactFeatureFlags.warnOnLegacyNativeModuleSystemUse is enabled, the legacy NativeModule infra will log soft exceptions whenever legacy NativeModules are accessed/used. Changelog: [Internal] Reviewed By: p-sun Differential Revision: D30272695 fbshipit-source-id: 7111402c1d8b883a600dcb4559e9ff1d56447070 --- .../react/bridge/CatalystInstanceImpl.java | 7 ++++++ .../jni/react/jni/CatalystInstanceImpl.cpp | 13 ++++++++++ .../main/jni/react/jni/CatalystInstanceImpl.h | 4 ++++ ReactCommon/cxxreact/CxxNativeModule.cpp | 24 +++++++++++++++++++ ReactCommon/cxxreact/CxxNativeModule.h | 6 +++++ 5 files changed, 54 insertions(+) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 2691689cf7d..baffa40aabb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -139,6 +139,11 @@ public class CatalystInstanceImpl implements CatalystInstance { FLog.d(ReactConstants.TAG, "Initializing React Xplat Bridge before initializeBridge"); Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "initializeCxxBridge"); + + if (ReactFeatureFlags.warnOnLegacyNativeModuleSystemUse) { + warnOnLegacyNativeModuleSystemUse(); + } + initializeBridge( new BridgeCallback(this), jsExecutor, @@ -206,6 +211,8 @@ public class CatalystInstanceImpl implements CatalystInstance { private native void jniExtendNativeModules( Collection javaModules, Collection cxxModules); + private native void warnOnLegacyNativeModuleSystemUse(); + private native void initializeBridge( ReactCallback callback, JavaScriptExecutor jsExecutor, diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 2a33fa3d0f8..7cdd623998a 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -29,6 +29,7 @@ #include "CxxModuleWrapper.h" #include "JNativeRunnable.h" +#include "JReactSoftExceptionLogger.h" #include "JavaScriptExecutorHolder.h" #include "JniJSModulesUnbundle.h" #include "NativeArray.h" @@ -92,6 +93,15 @@ CatalystInstanceImpl::initHybrid(jni::alias_ref) { CatalystInstanceImpl::CatalystInstanceImpl() : instance_(std::make_unique()) {} +void logSoftException(std::string message) { + JReactSoftExceptionLogger::logNoThrowSoftExceptionWithMessage( + "ReactNativeLogger#warning", message); +} + +void CatalystInstanceImpl::warnOnLegacyNativeModuleSystemUse() { + CxxNativeModule::setWarnOnUsageLogger(&logSoftException); +} + void CatalystInstanceImpl::registerNatives() { registerHybrid({ makeNativeMethod("initHybrid", CatalystInstanceImpl::initHybrid), @@ -127,6 +137,9 @@ void CatalystInstanceImpl::registerNatives() { CatalystInstanceImpl::handleMemoryPressure), makeNativeMethod( "getRuntimeExecutor", CatalystInstanceImpl::getRuntimeExecutor), + makeNativeMethod( + "warnOnLegacyNativeModuleSystemUse", + CatalystInstanceImpl::warnOnLegacyNativeModuleSystemUse), }); JNativeRunnable::registerNatives(); diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index c40a691232c..cc4006b26b0 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -61,6 +61,10 @@ class CatalystInstanceImpl : public jni::HybridClass { jni::alias_ref::javaobject> cxxModules); + // When called from CatalystInstanceImpl.java, warnings will be logged when + // CxxNativeModules are used. Java NativeModule usages log error in Java. + void warnOnLegacyNativeModuleSystemUse(); + void extendNativeModules( jni::alias_ref::javaobject> javaModules, diff --git a/ReactCommon/cxxreact/CxxNativeModule.cpp b/ReactCommon/cxxreact/CxxNativeModule.cpp index bfbb608553b..64ab808c280 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.cpp +++ b/ReactCommon/cxxreact/CxxNativeModule.cpp @@ -54,6 +54,12 @@ CxxModule::Callback convertCallback( } // namespace +WarnOnUsageLogger CxxNativeModule::warnOnUsageLogger_ = nullptr; + +void CxxNativeModule::setWarnOnUsageLogger(WarnOnUsageLogger logger) { + warnOnUsageLogger_ = logger; +} + std::string CxxNativeModule::getName() { return name_; } @@ -87,6 +93,12 @@ folly::dynamic CxxNativeModule::getConstants() { return nullptr; } + if (warnOnUsageLogger_) { + warnOnUsageLogger_( + "Calling getConstants() on Cxx NativeModule (name = \"" + getName() + + "\")."); + } + folly::dynamic constants = folly::dynamic::object(); for (auto &pair : module_->getConstants()) { constants.insert(std::move(pair.first), std::move(pair.second)); @@ -121,6 +133,12 @@ void CxxNativeModule::invoke( "Method ", method.name, " is synchronous but invoked asynchronously")); } + if (warnOnUsageLogger_) { + warnOnUsageLogger_( + "Calling " + method.name + "() on Cxx NativeModule (name = \"" + + getName() + "\")."); + } + if (params.size() < method.callbacks) { throw std::invalid_argument(folly::to( "Expected ", @@ -204,6 +222,12 @@ MethodCallResult CxxNativeModule::callSerializableNativeHook( "Method ", method.name, " is asynchronous but invoked synchronously")); } + if (warnOnUsageLogger_) { + warnOnUsageLogger_( + "Calling " + method.name + "() on Cxx NativeModule (name = \"" + + getName() + "\")."); + } + return method.syncFunc(std::move(args)); } diff --git a/ReactCommon/cxxreact/CxxNativeModule.h b/ReactCommon/cxxreact/CxxNativeModule.h index 9d730ff7942..6db75996758 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.h +++ b/ReactCommon/cxxreact/CxxNativeModule.h @@ -20,6 +20,8 @@ namespace react { class Instance; class MessageQueueThread; +typedef void (*WarnOnUsageLogger)(std::string message); + std::function makeCallback( std::weak_ptr instance, const folly::dynamic &callbackId); @@ -46,6 +48,8 @@ class RN_EXPORT CxxNativeModule : public NativeModule { unsigned int hookId, folly::dynamic &&args) override; + static void setWarnOnUsageLogger(WarnOnUsageLogger logger); + private: void lazyInit(); @@ -55,6 +59,8 @@ class RN_EXPORT CxxNativeModule : public NativeModule { std::shared_ptr messageQueueThread_; std::unique_ptr module_; std::vector methods_; + + static WarnOnUsageLogger warnOnUsageLogger_; }; } // namespace react