From b29a78732d346f86ea2ae2e340b5c6eb9a098218 Mon Sep 17 00:00:00 2001 From: Sota Ogo Date: Wed, 18 Aug 2021 23:36:33 -0700 Subject: [PATCH] Emit soft error for warning Summary: This diff adds a default behavior for the unified logger on Android. Added the call site in the CXXNativeModule. Changelog: [internal] Reviewed By: JoshuaGross Differential Revision: D30377767 fbshipit-source-id: 000014828f2f245dc9492e3617218895d9a33536 --- .../src/main/jni/react/jni/Android.mk | 3 +- ReactAndroid/src/main/jni/react/jni/BUCK | 1 + .../jni/react/jni/CatalystInstanceImpl.cpp | 31 +++++++++++--- ReactCommon/cxxreact/Android.mk | 3 +- ReactCommon/cxxreact/BUCK | 1 + ReactCommon/cxxreact/CxxNativeModule.cpp | 40 ++++++++++--------- ReactCommon/cxxreact/CxxNativeModule.h | 7 +++- ReactCommon/cxxreact/React-cxxreact.podspec | 1 + packages/rn-tester/Podfile.lock | 3 +- 9 files changed, 61 insertions(+), 29 deletions(-) diff --git a/ReactAndroid/src/main/jni/react/jni/Android.mk b/ReactAndroid/src/main/jni/react/jni/Android.mk index b3a1d0bbf07..82445d00ec0 100644 --- a/ReactAndroid/src/main/jni/react/jni/Android.mk +++ b/ReactAndroid/src/main/jni/react/jni/Android.mk @@ -77,7 +77,7 @@ LOCAL_CFLAGS += -fexceptions -frtti -Wno-unused-lambda-capture LOCAL_LDLIBS += -landroid # The dynamic libraries (.so files) that this module depends on. -LOCAL_SHARED_LIBRARIES := libreactnativeutilsjni libfolly_json libfb libfbjni libglog_init libyoga +LOCAL_SHARED_LIBRARIES := libreactnativeutilsjni libfolly_json libfb libfbjni libglog_init libyoga logger # The static libraries (.a files) that this module depends on. LOCAL_STATIC_LIBRARIES := libreactnative libruntimeexecutor libcallinvokerholder @@ -124,6 +124,7 @@ $(call import-module,yogajni) $(call import-module,cxxreact) $(call import-module,jsi) $(call import-module,jsiexecutor) +$(call import-module,logger) $(call import-module,callinvoker) $(call import-module,reactperflogger) $(call import-module,hermes) diff --git a/ReactAndroid/src/main/jni/react/jni/BUCK b/ReactAndroid/src/main/jni/react/jni/BUCK index de79bff729e..a53691bf91e 100644 --- a/ReactAndroid/src/main/jni/react/jni/BUCK +++ b/ReactAndroid/src/main/jni/react/jni/BUCK @@ -67,6 +67,7 @@ rn_xplat_cxx_library( react_native_xplat_target("cxxreact:module"), react_native_xplat_target("jsinspector:jsinspector"), react_native_xplat_target("runtimeexecutor:runtimeexecutor"), + react_native_xplat_target("logger:logger"), react_native_xplat_dep("jsi:jsi"), FBJNI_TARGET, ]) if not IS_OSS_BUILD else [], diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 7cdd623998a..d9e5ac34b41 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -26,6 +26,9 @@ #include #include #include +#include + +#include #include "CxxModuleWrapper.h" #include "JNativeRunnable.h" @@ -93,13 +96,8 @@ 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); + CxxNativeModule::setShouldWarnOnUse(true); } void CatalystInstanceImpl::registerNatives() { @@ -145,6 +143,25 @@ void CatalystInstanceImpl::registerNatives() { JNativeRunnable::registerNatives(); } +void log(ReactNativeLogLevel level, const char *message) { + switch (level) { + case ReactNativeLogLevelInfo: + LOG(INFO) << message; + break; + case ReactNativeLogLevelWarning: + LOG(WARNING) << message; + JReactSoftExceptionLogger::logNoThrowSoftExceptionWithMessage( + "react_native_log#warning", message); + break; + case ReactNativeLogLevelError: + LOG(ERROR) << message; + break; + case ReactNativeLogLevelFatal: + LOG(FATAL) << message; + break; + } +} + void CatalystInstanceImpl::initializeBridge( jni::alias_ref callback, // This executor is actually a factory holder. @@ -155,6 +172,8 @@ void CatalystInstanceImpl::initializeBridge( javaModules, jni::alias_ref::javaobject> cxxModules) { + set_react_native_logfunc(&log); + // TODO mhorowitz: how to assert here? // Assertions.assertCondition(mBridge == null, "initializeBridge should be // called once"); diff --git a/ReactCommon/cxxreact/Android.mk b/ReactCommon/cxxreact/Android.mk index a5024364c60..392c33b730a 100644 --- a/ReactCommon/cxxreact/Android.mk +++ b/ReactCommon/cxxreact/Android.mk @@ -20,7 +20,7 @@ LOCAL_CFLAGS := \ LOCAL_CFLAGS += -fexceptions -frtti -Wno-unused-lambda-capture LOCAL_STATIC_LIBRARIES := boost jsi callinvoker reactperflogger runtimeexecutor -LOCAL_SHARED_LIBRARIES := jsinspector libfolly_json glog +LOCAL_SHARED_LIBRARIES := jsinspector libfolly_json glog logger include $(BUILD_STATIC_LIBRARY) @@ -34,3 +34,4 @@ $(call import-module,jsi) $(call import-module,jsinspector) $(call import-module,hermes/inspector) $(call import-module,hermes/executor) +$(call import-module,logger) diff --git a/ReactCommon/cxxreact/BUCK b/ReactCommon/cxxreact/BUCK index 983a27d3878..095e3d98b2c 100644 --- a/ReactCommon/cxxreact/BUCK +++ b/ReactCommon/cxxreact/BUCK @@ -152,6 +152,7 @@ rn_xplat_cxx_library( react_native_xplat_target("microprofiler:microprofiler"), react_native_xplat_target("runtimeexecutor:runtimeexecutor"), react_native_xplat_target("reactperflogger:reactperflogger"), + react_native_xplat_target("logger:logger"), "//third-party/glog:glog", "//xplat/folly:optional", ], diff --git a/ReactCommon/cxxreact/CxxNativeModule.cpp b/ReactCommon/cxxreact/CxxNativeModule.cpp index 64ab808c280..0bcf13657ee 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.cpp +++ b/ReactCommon/cxxreact/CxxNativeModule.cpp @@ -16,6 +16,8 @@ #include "MessageQueueThread.h" #include "SystraceSection.h" +#include + using facebook::xplat::module::CxxModule; namespace facebook { namespace react { @@ -54,10 +56,24 @@ CxxModule::Callback convertCallback( } // namespace -WarnOnUsageLogger CxxNativeModule::warnOnUsageLogger_ = nullptr; +bool CxxNativeModule::shouldWarnOnUse_ = false; -void CxxNativeModule::setWarnOnUsageLogger(WarnOnUsageLogger logger) { - warnOnUsageLogger_ = logger; +void CxxNativeModule::setShouldWarnOnUse(bool value) { + shouldWarnOnUse_ = value; +} + +void CxxNativeModule::emitWarnIfWarnOnUsage( + const std::string &method_name, + const std::string &module_name) { + if (shouldWarnOnUse_) { + std::string message = folly::to( + "Calling ", + method_name, + " on Cxx NativeModule (name = \"", + module_name, + "\")."); + react_native_log_warn(message.c_str()); + } } std::string CxxNativeModule::getName() { @@ -93,11 +109,7 @@ folly::dynamic CxxNativeModule::getConstants() { return nullptr; } - if (warnOnUsageLogger_) { - warnOnUsageLogger_( - "Calling getConstants() on Cxx NativeModule (name = \"" + getName() + - "\")."); - } + emitWarnIfWarnOnUsage("getConstants()", getName()); folly::dynamic constants = folly::dynamic::object(); for (auto &pair : module_->getConstants()) { @@ -133,11 +145,7 @@ void CxxNativeModule::invoke( "Method ", method.name, " is synchronous but invoked asynchronously")); } - if (warnOnUsageLogger_) { - warnOnUsageLogger_( - "Calling " + method.name + "() on Cxx NativeModule (name = \"" + - getName() + "\")."); - } + emitWarnIfWarnOnUsage(method.name, getName()); if (params.size() < method.callbacks) { throw std::invalid_argument(folly::to( @@ -222,11 +230,7 @@ MethodCallResult CxxNativeModule::callSerializableNativeHook( "Method ", method.name, " is asynchronous but invoked synchronously")); } - if (warnOnUsageLogger_) { - warnOnUsageLogger_( - "Calling " + method.name + "() on Cxx NativeModule (name = \"" + - getName() + "\")."); - } + emitWarnIfWarnOnUsage(method.name, getName()); return method.syncFunc(std::move(args)); } diff --git a/ReactCommon/cxxreact/CxxNativeModule.h b/ReactCommon/cxxreact/CxxNativeModule.h index 6db75996758..a10f0ec5cb0 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.h +++ b/ReactCommon/cxxreact/CxxNativeModule.h @@ -48,7 +48,7 @@ class RN_EXPORT CxxNativeModule : public NativeModule { unsigned int hookId, folly::dynamic &&args) override; - static void setWarnOnUsageLogger(WarnOnUsageLogger logger); + static void setShouldWarnOnUse(bool value); private: void lazyInit(); @@ -59,8 +59,11 @@ class RN_EXPORT CxxNativeModule : public NativeModule { std::shared_ptr messageQueueThread_; std::unique_ptr module_; std::vector methods_; + void emitWarnIfWarnOnUsage( + const std::string &method_name, + const std::string &module_name); - static WarnOnUsageLogger warnOnUsageLogger_; + static bool shouldWarnOnUse_; }; } // namespace react diff --git a/ReactCommon/cxxreact/React-cxxreact.podspec b/ReactCommon/cxxreact/React-cxxreact.podspec index c007973d351..15020a870af 100644 --- a/ReactCommon/cxxreact/React-cxxreact.podspec +++ b/ReactCommon/cxxreact/React-cxxreact.podspec @@ -45,4 +45,5 @@ Pod::Spec.new do |s| s.dependency "React-runtimeexecutor", version s.dependency "React-perflogger", version s.dependency "React-jsi", version + s.dependency "React-logger", version end diff --git a/packages/rn-tester/Podfile.lock b/packages/rn-tester/Podfile.lock index e24a368eff2..52b05d0344b 100644 --- a/packages/rn-tester/Podfile.lock +++ b/packages/rn-tester/Podfile.lock @@ -262,6 +262,7 @@ PODS: - React-callinvoker (= 1000.0.0) - React-jsi (= 1000.0.0) - React-jsinspector (= 1000.0.0) + - React-logger (= 1000.0.0) - React-perflogger (= 1000.0.0) - React-runtimeexecutor (= 1000.0.0) - React-Fabric (1000.0.0): @@ -885,7 +886,7 @@ SPEC CHECKSUMS: React-callinvoker: c5d61e29df57793f0dc10ec2bc01c846f863e51f React-Core: 15d3fbb3cc863fa9990cc14c303a021cc66892a5 React-CoreModules: 5ee1ed4f8b7f8bdbd45ed155a15c601dca9c73dd - React-cxxreact: 20a63475c83c5450442c754305738c6db6070214 + React-cxxreact: 2fe718ab7094db2941ddaf35b2a44b8b57a7cece React-Fabric: 7641eab239c5fc5669ef22ea08cf383f9066fdcb React-graphics: db797c4609216593a1a12d1661716898d303900f React-jsi: 7cc3d3691803478047e7d2c8eb5d4c2f9c6d2922