From fbd8504cc0df8d65fd1782eafcab7b9d464d539b Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 19 Oct 2023 05:20:55 -0700 Subject: [PATCH] Replace RAIICallbackWrapperDestroyer with AsyncCallback (re-land) (#41048) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41048 Reapplies D49792717 AsyncCallback and SyncCallbacks are better primitives for jsi::Function handling. The code is simpler and requires less manual argument passing. See in D49684248 how the API was extended to support more use-cases. The underlying issue causing memory corruption has been addressed in D50286876. Changelog: [Deprecated] AsyncCallback replaces RAIICallbackWrapperDestroyer as a safer way to manage jsi::Function memory ownership. Reviewed By: rshest Differential Revision: D50319914 fbshipit-source-id: e038813cad85c47be1f004bc2ea1fdaf0eee9094 --- .../core/ReactCommon/TurboModuleUtils.h | 1 + .../android/ReactCommon/JavaTurboModule.cpp | 250 +++++++----------- 2 files changed, 101 insertions(+), 150 deletions(-) diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h index d2558250876642..786c4cdb3cec02 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h @@ -37,6 +37,7 @@ jsi::Value createPromiseAsJSIValue( jsi::Runtime& rt, PromiseSetupFunctionType&& func); +// Deprecated. Use AsyncCallback instead. class RAIICallbackWrapperDestroyer { public: RAIICallbackWrapperDestroyer(std::weak_ptr callbackWrapper) 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 3319b7431dc4bd..724d91e3ccb3d1 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 @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -63,61 +64,43 @@ struct JNIArgs { std::vector globalRefs_; }; -jni::local_ref createJavaCallbackFromJSIFunction( - jsi::Function&& function, +auto createJavaCallback( jsi::Runtime& rt, - const std::shared_ptr& jsInvoker) { - auto weakWrapper = - CallbackWrapper::createWeak(std::move(function), rt, jsInvoker); - - // This needs to be a shared_ptr because: - // 1. It cannot be unique_ptr. std::function is copyable but unique_ptr is - // not. - // 2. It cannot be weak_ptr since we need this object to live on. - // 3. It cannot be a value, because that would be deleted as soon as this - // function returns. - auto callbackWrapperOwner = - std::make_shared(weakWrapper); - + jsi::Function&& function, + std::shared_ptr jsInvoker) { + std::optional> callback( + {rt, std::move(function), std::move(jsInvoker)}); return JCxxCallbackImpl::newObjectCxxArgs( - [weakWrapper = std::move(weakWrapper), - callbackWrapperOwner = std::move(callbackWrapperOwner), - wrapperWasCalled = false](folly::dynamic responses) mutable { - if (wrapperWasCalled) { - LOG(FATAL) << "callback arg cannot be called more than once"; - } - - auto strongWrapper = weakWrapper.lock(); - if (!strongWrapper) { + [callback = std::move(callback)](folly::dynamic args) mutable { + if (!callback) { + LOG(FATAL) << "Callback arg cannot be called more than once"; return; } - strongWrapper->jsInvoker().invokeAsync( - [weakWrapper = std::move(weakWrapper), - callbackWrapperOwner = std::move(callbackWrapperOwner), - responses = std::move(responses)]() { - auto strongWrapper2 = weakWrapper.lock(); - if (!strongWrapper2) { - return; - } - - std::vector args; - args.reserve(responses.size()); - for (const auto& val : responses) { - args.emplace_back( - jsi::valueFromDynamic(strongWrapper2->runtime(), val)); - } - - strongWrapper2->callback().call( - strongWrapper2->runtime(), - (const jsi::Value*)args.data(), - args.size()); - }); - - wrapperWasCalled = true; + callback->call([args = std::move(args)]( + jsi::Runtime& rt, jsi::Function& jsFunction) { + std::vector jsArgs; + jsArgs.reserve(args.size()); + for (const auto& val : args) { + jsArgs.emplace_back(jsi::valueFromDynamic(rt, val)); + } + jsFunction.call(rt, (const jsi::Value*)jsArgs.data(), jsArgs.size()); + }); + callback = std::nullopt; }); } +struct JPromiseImpl : public jni::JavaClass { + constexpr static auto kJavaDescriptor = + "Lcom/facebook/react/bridge/PromiseImpl;"; + + static jni::local_ref create( + jni::local_ref resolve, + jni::local_ref reject) { + return newInstance(resolve, reject); + } +}; + // This is used for generating short exception strings. std::string stringifyJSIValue(const jsi::Value& v, jsi::Runtime* rt = nullptr) { if (v.isUndefined()) { @@ -356,8 +339,7 @@ JNIArgs convertJSIArgsToJNIArgs( } jsi::Function fn = arg->getObject(rt).getFunction(rt); jarg->l = makeGlobalIfNecessary( - createJavaCallbackFromJSIFunction(std::move(fn), rt, jsInvoker) - .release()); + createJavaCallback(rt, std::move(fn), jsInvoker).release()); } else if (type == "Lcom/facebook/react/bridge/ReadableArray;") { if (!(arg->isObject() && arg->getObject(rt).isArray(rt))) { throw JavaTurboModuleArgumentConversionException( @@ -756,16 +738,14 @@ jsi::Value JavaTurboModule::invokeJavaMethod( instance_ = jni::make_weak(instance_), moduleNameStr = name_, methodNameStr, - id = getUniqueId()]() mutable -> void { + id = getUniqueId()]() mutable { auto instance = instance_.lockLocal(); if (!instance) { return; } - /** - * TODO(ramanpreet): Why do we have to require the environment - * again? Why does JNI crash when we use the env from the upper - * scope? - */ + + // Require the env from the current scope, which may be + // different from the original invocation's scope JNIEnv* env = jni::Environment::current(); const char* moduleName = moduleNameStr.c_str(); const char* methodName = methodNameStr.c_str(); @@ -789,115 +769,85 @@ jsi::Value JavaTurboModule::invokeJavaMethod( return jsi::Value::undefined(); } case PromiseKind: { + // We could use AsyncPromise here, but this avoids the overhead of + // the shared_ptr for PromiseHolder jsi::Function Promise = runtime.global().getPropertyAsFunction(runtime, "Promise"); - jsi::Function promiseConstructorArg = jsi::Function::createFromHostFunction( + // The promise constructor runs its arg immediately, so this is safe + jobject javaPromise; + jsi::Value jsPromise = Promise.callAsConstructor( runtime, - jsi::PropNameID::forAscii(runtime, "fn"), - 2, - [this, - &jargs, - &globalRefs, - argCount, + jsi::Function::createFromHostFunction( + runtime, + jsi::PropNameID::forAscii(runtime, "fn"), + 2, + [&](jsi::Runtime& runtime, + const jsi::Value&, + const jsi::Value* args, + size_t argCount) { + if (argCount != 2) { + throw jsi::JSError(runtime, "Incorrect number of arguments"); + } + + auto resolve = createJavaCallback( + runtime, + args[0].getObject(runtime).getFunction(runtime), + jsInvoker_); + auto reject = createJavaCallback( + runtime, + args[1].getObject(runtime).getFunction(runtime), + jsInvoker_); + javaPromise = JPromiseImpl::create(resolve, reject).release(); + + return jsi::Value::undefined(); + })); + + jobject globalPromise = env->NewGlobalRef(javaPromise); + globalRefs.push_back(globalPromise); + env->DeleteLocalRef(javaPromise); + jargs[argCount].l = globalPromise; + + const char* moduleName = name_.c_str(); + const char* methodName = methodNameStr.c_str(); + TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName); + + TMPL::asyncMethodCallDispatch(moduleName, methodName); + nativeMethodCallInvoker_->invokeAsync( + methodName, + [jargs, + globalRefs, methodID, + instance_ = jni::make_weak(instance_), moduleNameStr = name_, methodNameStr, - env]( - jsi::Runtime& runtime, - const jsi::Value& thisVal, - const jsi::Value* promiseConstructorArgs, - size_t promiseConstructorArgCount) { - if (promiseConstructorArgCount != 2) { - throw std::invalid_argument("Promise fn arg count must be 2"); + id = getUniqueId()]() mutable { + auto instance = instance_.lockLocal(); + if (!instance) { + return; } - jsi::Function resolveJSIFn = - promiseConstructorArgs[0].getObject(runtime).getFunction( - runtime); - jsi::Function rejectJSIFn = - promiseConstructorArgs[1].getObject(runtime).getFunction( - runtime); - - auto resolve = createJavaCallbackFromJSIFunction( - std::move(resolveJSIFn), runtime, jsInvoker_) - .release(); - auto reject = createJavaCallbackFromJSIFunction( - std::move(rejectJSIFn), runtime, jsInvoker_) - .release(); - - jclass jPromiseImpl = - env->FindClass("com/facebook/react/bridge/PromiseImpl"); - jmethodID jPromiseImplConstructor = env->GetMethodID( - jPromiseImpl, - "", - "(Lcom/facebook/react/bridge/Callback;Lcom/facebook/react/bridge/Callback;)V"); - - jobject promise = env->NewObject( - jPromiseImpl, jPromiseImplConstructor, resolve, reject); - + // Require the env from the current scope, which may be + // different from the original invocation's scope + JNIEnv* env = jni::Environment::current(); const char* moduleName = moduleNameStr.c_str(); const char* methodName = methodNameStr.c_str(); + TMPL::asyncMethodCallExecutionStart(moduleName, methodName, id); + env->CallVoidMethodA(instance.get(), methodID, jargs.data()); + try { + FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); + } catch (...) { + TMPL::asyncMethodCallExecutionFail(moduleName, methodName, id); + throw; + } - jobject globalPromise = env->NewGlobalRef(promise); - - globalRefs.push_back(globalPromise); - env->DeleteLocalRef(promise); - - jargs[argCount].l = globalPromise; - TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName); - TMPL::asyncMethodCallDispatch(moduleName, methodName); - - nativeMethodCallInvoker_->invokeAsync( - methodName, - [jargs, - globalRefs, - methodID, - instance_ = jni::make_weak(instance_), - moduleNameStr, - methodNameStr, - id = getUniqueId()]() mutable -> void { - auto instance = instance_.lockLocal(); - - if (!instance) { - return; - } - /** - * TODO(ramanpreet): Why do we have to require the - * environment again? Why does JNI crash when we use the env - * from the upper scope? - */ - JNIEnv* env = jni::Environment::current(); - const char* moduleName = moduleNameStr.c_str(); - const char* methodName = methodNameStr.c_str(); - - TMPL::asyncMethodCallExecutionStart( - moduleName, methodName, id); - env->CallVoidMethodA(instance.get(), methodID, jargs.data()); - try { - FACEBOOK_JNI_THROW_PENDING_EXCEPTION(); - } catch (...) { - TMPL::asyncMethodCallExecutionFail( - moduleName, methodName, id); - throw; - } - - for (auto globalRef : globalRefs) { - env->DeleteGlobalRef(globalRef); - } - TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id); - }); - - return jsi::Value::undefined(); + for (auto globalRef : globalRefs) { + env->DeleteGlobalRef(globalRef); + } + TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id); }); - - jsi::Value promise = - Promise.callAsConstructor(runtime, promiseConstructorArg); - checkJNIErrorForMethodCall(); - TMPL::asyncMethodCallEnd(moduleName, methodName); - - return promise; + return jsPromise; } default: throw std::runtime_error(