From 9b50ad462accc670077992dec7646b2e8d77737e Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Fri, 27 Oct 2023 10:20:40 -0700 Subject: [PATCH] feat(tm-android): Reject Promise if Turbo Module method throws an Error (#37484) Summary: ### [iOS change here](https://github.com/facebook/react-native/pull/40764) This PR builds upon the previous work done in https://github.com/facebook/react-native/pull/36925, which introduced native stack traces to the JSError for synchronous functions. The current modifications concentrate on functions that return Promises. Prior to this PR, errors within Promise-returning functions would be thrown at the platform layer crashing the app without a link to the JS stack. After the implementation of this PR, errors thrown within Promise-returning functions are now captured and transformed into rejected Promises. These rejected Promises contain a JS Error object that contains both the JS stack trace and the cause, along with the platform stack trace. Additionally, this PR ensures that rejections from native functions are now linked to the JS stack trace, providing a more comprehensive view of the rejection flow. ## Changelog: [GENERAL][ADDED] - Turbo Modules Promise-returning functions reject with JS and platform stack traces information Pull Request resolved: https://github.com/facebook/react-native/pull/37484 Test Plan: | Android | |--------| | ![function_promise_android](https://github.com/krystofwoldrich/react-native/assets/31292499/1d1a3adf-986a-47b4-b98b-9e766176b7ae) | Example of intentionally rejected promise on Android: ``` { "name": "Error", "message": "Exception in HostFunction: intentional promise rejection", "stack": "[native code]\ntryCallTwo@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:25844:9\ndoResolve@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:25975:25\nPromise@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:25863:14\n[native code]\nrejectPromise@http://10.0.2.2:8081/js/examples/TurboModule/SampleTurboModuleExample.bundle?platform=android&lazy=true&app=com.facebook.react.uiapp&modulesOnly=true&dev=true&minify=false&runModule=true&shallow=true:42:70\nonPress@http://10.0.2.2:8081/js/examples/TurboModule/SampleTurboModuleExample.bundle?platform=android&lazy=true&app=com.facebook.react.uiapp&modulesOnly=true&dev=true&minify=false&runModule=true&shallow=true:242:71\n_performTransitionSideEffects@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:51896:22\n_receiveSignal@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:51852:45\nonResponderRelease@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:51715:34\ninvokeGuardedCallbackProd@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:2962:21\ninvokeGuardedCallback@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:3048:42\ninvokeGuardedCallbackAndCatchFirstError@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:3051:36\nexecuteDispatch@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:3115:48\nexecuteDispatchesInOrder@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:3132:26\nexecuteDispatchesAndRelease@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4856:35\nforEach@[native code]\nforEachAccumulated@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:3574:22\nrunEventsInBatch@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4874:27\nrunExtractedPluginEventsInBatch@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4896:25\nhttp://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4914:42\nbatchedUpdates$1@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:14750:20\nbatchedUpdates@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4845:36\ndispatchEvent@http://10.0.2.2:8081/js/RNTesterApp.android.bundle?platform=android&dev=true&lazy=true&minify=false&app=com.facebook.react.uiapp&modulesOnly=false&runModule=true:4907:23", "cause": { "nativeStackAndroid": [ { "lineNumber": 173, "file": "SampleTurboModule.java", "methodName": "getValueWithPromise", "class": "com.facebook.fbreact.specs.SampleTurboModule" }, { "lineNumber": -2, "file": "NativeRunnable.java", "methodName": "run", "class": "com.facebook.jni.NativeRunnable" }, { "lineNumber": 942, "file": "Handler.java", "methodName": "handleCallback", "class": "android.os.Handler" }, { "lineNumber": 99, "file": "Handler.java", "methodName": "dispatchMessage", "class": "android.os.Handler" }, { "lineNumber": 27, "file": "MessageQueueThreadHandler.java", "methodName": "dispatchMessage", "class": "com.facebook.react.bridge.queue.MessageQueueThreadHandler" }, { "lineNumber": 201, "file": "Looper.java", "methodName": "loopOnce", "class": "android.os.Looper" }, { "lineNumber": 288, "file": "Looper.java", "methodName": "loop", "class": "android.os.Looper" }, { "lineNumber": 228, "file": "MessageQueueThreadImpl.java", "methodName": "run", "class": "com.facebook.react.bridge.queue.MessageQueueThreadImpl$4" }, { "lineNumber": 1012, "file": "Thread.java", "methodName": "run", "class": "java.lang.Thread" } ], "userInfo": null, "message": "intentional promise rejection", "code": "code 1" } } ``` How I logged out the Errors: ```js console.log('Error in JS:', JSON.stringify({ name: e.name, message: e.message, stack: e.stack, ...e, }, null, 2)); ``` Reviewed By: RSNara Differential Revision: D50613349 Pulled By: javache fbshipit-source-id: b49c469118c8d8d27c43164f110dfe57ddd592d9 --- .../react/config/ReactFeatureFlags.java | 13 ++++ .../android/ReactCommon/JavaTurboModule.cpp | 73 ++++++++++++++++++- .../TurboModule/SampleTurboModuleExample.js | 33 +++++---- 3 files changed, 103 insertions(+), 16 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 035d071bf19b00..366da86b585b6f 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -8,6 +8,7 @@ package com.facebook.react.config; import com.facebook.proguard.annotations.DoNotStripAny; +import com.facebook.react.common.build.ReactBuildConfig; /** * Hi there, traveller! This configuration class is not meant to be used by end-users of RN. It @@ -161,4 +162,16 @@ public class ReactFeatureFlags { * priorities from any thread. */ public static boolean useModernRuntimeScheduler = false; + + /** + * Enables storing js caller stack when creating promise in native module. This is useful in case + * of Promise rejection and tracing the cause. + */ + public static boolean traceTurboModulePromiseRejections = ReactBuildConfig.DEBUG; + + /** + * Enables auto rejecting promises from Turbo Modules method calls. If native error occurs Promise + * in JS will be rejected (The JS error will include native stack) + */ + public static boolean rejectTurboModulePromiseOnNativeError = true; } diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp index 2153e5e8e7c18e..a28b38fa1de194 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp @@ -58,6 +58,28 @@ JavaTurboModule::~JavaTurboModule() { namespace { +constexpr auto kReactFeatureFlagsJavaDescriptor = + "com/facebook/react/config/ReactFeatureFlags"; + +bool getFeatureFlagBoolValue(const char* name) { + static const auto reactFeatureFlagsClass = + facebook::jni::findClassStatic(kReactFeatureFlagsJavaDescriptor); + const auto field = reactFeatureFlagsClass->getStaticField(name); + return reactFeatureFlagsClass->getStaticFieldValue(field); +} + +bool traceTurboModulePromiseRejections() { + static bool traceRejections = + getFeatureFlagBoolValue("traceTurboModulePromiseRejections"); + return traceRejections; +} + +bool rejectTurboModulePromiseOnNativeError() { + static bool rejectOnError = + getFeatureFlagBoolValue("rejectTurboModulePromiseOnNativeError"); + return rejectOnError; +} + struct JNIArgs { JNIArgs(size_t count) : args_(count) {} std::vector args_; @@ -396,7 +418,7 @@ jsi::Value createJSRuntimeError( */ jsi::JSError convertThrowableToJSError( jsi::Runtime& runtime, - jni::local_ref throwable) { + jni::alias_ref throwable) { auto stackTrace = throwable->getStackTrace(); jsi::Array stackElements(runtime, stackTrace->size()); @@ -424,6 +446,22 @@ jsi::JSError convertThrowableToJSError( return {runtime, std::move(error)}; } +void rejectWithException( + AsyncCallback<>& reject, + std::exception_ptr exception, + std::optional& jsInvocationStack) { + auto throwable = jni::getJavaExceptionForCppException(exception); + reject.call([jsInvocationStack, throwable = jni::make_global(throwable)]( + jsi::Runtime& rt, jsi::Function& jsFunction) { + auto jsError = convertThrowableToJSError(rt, throwable); + if (jsInvocationStack.has_value()) { + jsError.value().asObject(rt).setProperty( + rt, "stack", jsInvocationStack.value()); + } + jsFunction.call(rt, jsError.value()); + }); +} + } // namespace jsi::Value JavaTurboModule::invokeJavaMethod( @@ -783,6 +821,10 @@ jsi::Value JavaTurboModule::invokeJavaMethod( jsi::Function Promise = runtime.global().getPropertyAsFunction(runtime, "Promise"); + // The callback is used for auto rejecting if error is caught from method + // invocation + std::optional> nativeRejectCallback; + // The promise constructor runs its arg immediately, so this is safe jobject javaPromise; jsi::Value jsPromise = Promise.callAsConstructor( @@ -799,6 +841,13 @@ jsi::Value JavaTurboModule::invokeJavaMethod( throw jsi::JSError(runtime, "Incorrect number of arguments"); } + if (rejectTurboModulePromiseOnNativeError()) { + nativeRejectCallback = AsyncCallback( + runtime, + args[1].getObject(runtime).getFunction(runtime), + jsInvoker_); + } + auto resolve = createJavaCallback( runtime, args[0].getObject(runtime).getFunction(runtime), @@ -817,14 +866,25 @@ jsi::Value JavaTurboModule::invokeJavaMethod( env->DeleteLocalRef(javaPromise); jargs[argCount].l = globalPromise; + // JS Stack at the time when the promise is created. + std::optional jsInvocationStack; + if (traceTurboModulePromiseRejections()) { + jsInvocationStack = createJSRuntimeError(runtime, "") + .asObject(runtime) + .getProperty(runtime, "stack") + .toString(runtime) + .utf8(runtime); + } + const char* moduleName = name_.c_str(); const char* methodName = methodNameStr.c_str(); TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName); - TMPL::asyncMethodCallDispatch(moduleName, methodName); nativeMethodCallInvoker_->invokeAsync( methodName, [jargs, + rejectCallback = std::move(nativeRejectCallback), + jsInvocationStack = std::move(jsInvocationStack), globalRefs, methodID, instance_ = jni::make_weak(instance_), @@ -856,7 +916,14 @@ jsi::Value JavaTurboModule::invokeJavaMethod( FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); } catch (...) { TMPL::asyncMethodCallExecutionFail(moduleName, methodName, id); - throw; + if (rejectTurboModulePromiseOnNativeError() && rejectCallback) { + auto exception = std::current_exception(); + rejectWithException( + *rejectCallback, exception, jsInvocationStack); + rejectCallback = std::nullopt; + } else { + throw; + } } for (auto globalRef : globalRefs) { diff --git a/packages/rn-tester/js/examples/TurboModule/SampleTurboModuleExample.js b/packages/rn-tester/js/examples/TurboModule/SampleTurboModuleExample.js index 57a2be7f582425..283c557bb2aab3 100644 --- a/packages/rn-tester/js/examples/TurboModule/SampleTurboModuleExample.js +++ b/packages/rn-tester/js/examples/TurboModule/SampleTurboModuleExample.js @@ -54,7 +54,10 @@ class SampleTurboModuleExample extends React.Component<{||}, State> { rejectPromise: () => NativeSampleTurboModule.getValueWithPromise(true) .then(() => {}) - .catch(e => this._setResult('rejectPromise', e.message)), + .catch(e => { + console.error(e); + this._setResult('rejectPromise', e.message); + }), getConstants: () => NativeSampleTurboModule.getConstants(), voidFunc: () => NativeSampleTurboModule.voidFunc(), getBool: () => NativeSampleTurboModule.getBool(true), @@ -81,6 +84,7 @@ class SampleTurboModuleExample extends React.Component<{||}, State> { try { NativeSampleTurboModule.voidFuncThrows?.(); } catch (e) { + console.error(e); return e.message; } }, @@ -88,21 +92,23 @@ class SampleTurboModuleExample extends React.Component<{||}, State> { try { NativeSampleTurboModule.getObjectThrows?.({a: 1, b: 'foo', c: null}); } catch (e) { + console.error(e); return e.message; } }, promiseThrows: () => { - try { - // $FlowFixMe[unused-promise] - NativeSampleTurboModule.promiseThrows?.(); - } catch (e) { - return e.message; - } + NativeSampleTurboModule.promiseThrows?.() + .then(() => {}) + .catch(e => { + console.error(e); + this._setResult('promiseThrows', e.message); + }); }, voidFuncAssert: () => { try { NativeSampleTurboModule.voidFuncAssert?.(); } catch (e) { + console.error(e); return e.message; } }, @@ -110,16 +116,17 @@ class SampleTurboModuleExample extends React.Component<{||}, State> { try { NativeSampleTurboModule.getObjectAssert?.({a: 1, b: 'foo', c: null}); } catch (e) { + console.error(e); return e.message; } }, promiseAssert: () => { - try { - // $FlowFixMe[unused-promise] - NativeSampleTurboModule.promiseAssert?.(); - } catch (e) { - return e.message; - } + NativeSampleTurboModule.promiseAssert?.() + .then(() => {}) + .catch(e => { + console.error(e); + this._setResult('promiseAssert', e.message); + }); }, };