Skip to content

Commit fbd8504

Browse files
javachefacebook-github-bot
javache
authored and
facebook-github-bot
committed
Replace RAIICallbackWrapperDestroyer with AsyncCallback (re-land) (#41048)
Summary: Pull Request resolved: #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
1 parent 4134d8d commit fbd8504

File tree

2 files changed

+101
-150
lines changed

2 files changed

+101
-150
lines changed

‎packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ jsi::Value createPromiseAsJSIValue(
3737
jsi::Runtime& rt,
3838
PromiseSetupFunctionType&& func);
3939

40+
// Deprecated. Use AsyncCallback instead.
4041
class RAIICallbackWrapperDestroyer {
4142
public:
4243
RAIICallbackWrapperDestroyer(std::weak_ptr<CallbackWrapper> callbackWrapper)

‎packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp

Lines changed: 100 additions & 150 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <ReactCommon/TurboModulePerfLogger.h>
1818
#include <ReactCommon/TurboModuleUtils.h>
1919
#include <jsi/JSIDynamic.h>
20+
#include <react/bridging/Bridging.h>
2021
#include <react/debug/react_native_assert.h>
2122
#include <react/jni/NativeMap.h>
2223
#include <react/jni/ReadableNativeMap.h>
@@ -63,61 +64,43 @@ struct JNIArgs {
6364
std::vector<jobject> globalRefs_;
6465
};
6566

66-
jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
67-
jsi::Function&& function,
67+
auto createJavaCallback(
6868
jsi::Runtime& rt,
69-
const std::shared_ptr<CallInvoker>& jsInvoker) {
70-
auto weakWrapper =
71-
CallbackWrapper::createWeak(std::move(function), rt, jsInvoker);
72-
73-
// This needs to be a shared_ptr because:
74-
// 1. It cannot be unique_ptr. std::function is copyable but unique_ptr is
75-
// not.
76-
// 2. It cannot be weak_ptr since we need this object to live on.
77-
// 3. It cannot be a value, because that would be deleted as soon as this
78-
// function returns.
79-
auto callbackWrapperOwner =
80-
std::make_shared<RAIICallbackWrapperDestroyer>(weakWrapper);
81-
69+
jsi::Function&& function,
70+
std::shared_ptr<CallInvoker> jsInvoker) {
71+
std::optional<AsyncCallback<>> callback(
72+
{rt, std::move(function), std::move(jsInvoker)});
8273
return JCxxCallbackImpl::newObjectCxxArgs(
83-
[weakWrapper = std::move(weakWrapper),
84-
callbackWrapperOwner = std::move(callbackWrapperOwner),
85-
wrapperWasCalled = false](folly::dynamic responses) mutable {
86-
if (wrapperWasCalled) {
87-
LOG(FATAL) << "callback arg cannot be called more than once";
88-
}
89-
90-
auto strongWrapper = weakWrapper.lock();
91-
if (!strongWrapper) {
74+
[callback = std::move(callback)](folly::dynamic args) mutable {
75+
if (!callback) {
76+
LOG(FATAL) << "Callback arg cannot be called more than once";
9277
return;
9378
}
9479

95-
strongWrapper->jsInvoker().invokeAsync(
96-
[weakWrapper = std::move(weakWrapper),
97-
callbackWrapperOwner = std::move(callbackWrapperOwner),
98-
responses = std::move(responses)]() {
99-
auto strongWrapper2 = weakWrapper.lock();
100-
if (!strongWrapper2) {
101-
return;
102-
}
103-
104-
std::vector<jsi::Value> args;
105-
args.reserve(responses.size());
106-
for (const auto& val : responses) {
107-
args.emplace_back(
108-
jsi::valueFromDynamic(strongWrapper2->runtime(), val));
109-
}
110-
111-
strongWrapper2->callback().call(
112-
strongWrapper2->runtime(),
113-
(const jsi::Value*)args.data(),
114-
args.size());
115-
});
116-
117-
wrapperWasCalled = true;
80+
callback->call([args = std::move(args)](
81+
jsi::Runtime& rt, jsi::Function& jsFunction) {
82+
std::vector<jsi::Value> jsArgs;
83+
jsArgs.reserve(args.size());
84+
for (const auto& val : args) {
85+
jsArgs.emplace_back(jsi::valueFromDynamic(rt, val));
86+
}
87+
jsFunction.call(rt, (const jsi::Value*)jsArgs.data(), jsArgs.size());
88+
});
89+
callback = std::nullopt;
11890
});
11991
}
12092

93+
struct JPromiseImpl : public jni::JavaClass<JPromiseImpl> {
94+
constexpr static auto kJavaDescriptor =
95+
"Lcom/facebook/react/bridge/PromiseImpl;";
96+
97+
static jni::local_ref<javaobject> create(
98+
jni::local_ref<JCallback::javaobject> resolve,
99+
jni::local_ref<JCallback::javaobject> reject) {
100+
return newInstance(resolve, reject);
101+
}
102+
};
103+
121104
// This is used for generating short exception strings.
122105
std::string stringifyJSIValue(const jsi::Value& v, jsi::Runtime* rt = nullptr) {
123106
if (v.isUndefined()) {
@@ -356,8 +339,7 @@ JNIArgs convertJSIArgsToJNIArgs(
356339
}
357340
jsi::Function fn = arg->getObject(rt).getFunction(rt);
358341
jarg->l = makeGlobalIfNecessary(
359-
createJavaCallbackFromJSIFunction(std::move(fn), rt, jsInvoker)
360-
.release());
342+
createJavaCallback(rt, std::move(fn), jsInvoker).release());
361343
} else if (type == "Lcom/facebook/react/bridge/ReadableArray;") {
362344
if (!(arg->isObject() && arg->getObject(rt).isArray(rt))) {
363345
throw JavaTurboModuleArgumentConversionException(
@@ -756,16 +738,14 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
756738
instance_ = jni::make_weak(instance_),
757739
moduleNameStr = name_,
758740
methodNameStr,
759-
id = getUniqueId()]() mutable -> void {
741+
id = getUniqueId()]() mutable {
760742
auto instance = instance_.lockLocal();
761743
if (!instance) {
762744
return;
763745
}
764-
/**
765-
* TODO(ramanpreet): Why do we have to require the environment
766-
* again? Why does JNI crash when we use the env from the upper
767-
* scope?
768-
*/
746+
747+
// Require the env from the current scope, which may be
748+
// different from the original invocation's scope
769749
JNIEnv* env = jni::Environment::current();
770750
const char* moduleName = moduleNameStr.c_str();
771751
const char* methodName = methodNameStr.c_str();
@@ -789,115 +769,85 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
789769
return jsi::Value::undefined();
790770
}
791771
case PromiseKind: {
772+
// We could use AsyncPromise here, but this avoids the overhead of
773+
// the shared_ptr for PromiseHolder
792774
jsi::Function Promise =
793775
runtime.global().getPropertyAsFunction(runtime, "Promise");
794776

795-
jsi::Function promiseConstructorArg = jsi::Function::createFromHostFunction(
777+
// The promise constructor runs its arg immediately, so this is safe
778+
jobject javaPromise;
779+
jsi::Value jsPromise = Promise.callAsConstructor(
796780
runtime,
797-
jsi::PropNameID::forAscii(runtime, "fn"),
798-
2,
799-
[this,
800-
&jargs,
801-
&globalRefs,
802-
argCount,
781+
jsi::Function::createFromHostFunction(
782+
runtime,
783+
jsi::PropNameID::forAscii(runtime, "fn"),
784+
2,
785+
[&](jsi::Runtime& runtime,
786+
const jsi::Value&,
787+
const jsi::Value* args,
788+
size_t argCount) {
789+
if (argCount != 2) {
790+
throw jsi::JSError(runtime, "Incorrect number of arguments");
791+
}
792+
793+
auto resolve = createJavaCallback(
794+
runtime,
795+
args[0].getObject(runtime).getFunction(runtime),
796+
jsInvoker_);
797+
auto reject = createJavaCallback(
798+
runtime,
799+
args[1].getObject(runtime).getFunction(runtime),
800+
jsInvoker_);
801+
javaPromise = JPromiseImpl::create(resolve, reject).release();
802+
803+
return jsi::Value::undefined();
804+
}));
805+
806+
jobject globalPromise = env->NewGlobalRef(javaPromise);
807+
globalRefs.push_back(globalPromise);
808+
env->DeleteLocalRef(javaPromise);
809+
jargs[argCount].l = globalPromise;
810+
811+
const char* moduleName = name_.c_str();
812+
const char* methodName = methodNameStr.c_str();
813+
TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName);
814+
815+
TMPL::asyncMethodCallDispatch(moduleName, methodName);
816+
nativeMethodCallInvoker_->invokeAsync(
817+
methodName,
818+
[jargs,
819+
globalRefs,
803820
methodID,
821+
instance_ = jni::make_weak(instance_),
804822
moduleNameStr = name_,
805823
methodNameStr,
806-
env](
807-
jsi::Runtime& runtime,
808-
const jsi::Value& thisVal,
809-
const jsi::Value* promiseConstructorArgs,
810-
size_t promiseConstructorArgCount) {
811-
if (promiseConstructorArgCount != 2) {
812-
throw std::invalid_argument("Promise fn arg count must be 2");
824+
id = getUniqueId()]() mutable {
825+
auto instance = instance_.lockLocal();
826+
if (!instance) {
827+
return;
813828
}
814829

815-
jsi::Function resolveJSIFn =
816-
promiseConstructorArgs[0].getObject(runtime).getFunction(
817-
runtime);
818-
jsi::Function rejectJSIFn =
819-
promiseConstructorArgs[1].getObject(runtime).getFunction(
820-
runtime);
821-
822-
auto resolve = createJavaCallbackFromJSIFunction(
823-
std::move(resolveJSIFn), runtime, jsInvoker_)
824-
.release();
825-
auto reject = createJavaCallbackFromJSIFunction(
826-
std::move(rejectJSIFn), runtime, jsInvoker_)
827-
.release();
828-
829-
jclass jPromiseImpl =
830-
env->FindClass("com/facebook/react/bridge/PromiseImpl");
831-
jmethodID jPromiseImplConstructor = env->GetMethodID(
832-
jPromiseImpl,
833-
"<init>",
834-
"(Lcom/facebook/react/bridge/Callback;Lcom/facebook/react/bridge/Callback;)V");
835-
836-
jobject promise = env->NewObject(
837-
jPromiseImpl, jPromiseImplConstructor, resolve, reject);
838-
830+
// Require the env from the current scope, which may be
831+
// different from the original invocation's scope
832+
JNIEnv* env = jni::Environment::current();
839833
const char* moduleName = moduleNameStr.c_str();
840834
const char* methodName = methodNameStr.c_str();
835+
TMPL::asyncMethodCallExecutionStart(moduleName, methodName, id);
836+
env->CallVoidMethodA(instance.get(), methodID, jargs.data());
837+
try {
838+
FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
839+
} catch (...) {
840+
TMPL::asyncMethodCallExecutionFail(moduleName, methodName, id);
841+
throw;
842+
}
841843

842-
jobject globalPromise = env->NewGlobalRef(promise);
843-
844-
globalRefs.push_back(globalPromise);
845-
env->DeleteLocalRef(promise);
846-
847-
jargs[argCount].l = globalPromise;
848-
TMPL::asyncMethodCallArgConversionEnd(moduleName, methodName);
849-
TMPL::asyncMethodCallDispatch(moduleName, methodName);
850-
851-
nativeMethodCallInvoker_->invokeAsync(
852-
methodName,
853-
[jargs,
854-
globalRefs,
855-
methodID,
856-
instance_ = jni::make_weak(instance_),
857-
moduleNameStr,
858-
methodNameStr,
859-
id = getUniqueId()]() mutable -> void {
860-
auto instance = instance_.lockLocal();
861-
862-
if (!instance) {
863-
return;
864-
}
865-
/**
866-
* TODO(ramanpreet): Why do we have to require the
867-
* environment again? Why does JNI crash when we use the env
868-
* from the upper scope?
869-
*/
870-
JNIEnv* env = jni::Environment::current();
871-
const char* moduleName = moduleNameStr.c_str();
872-
const char* methodName = methodNameStr.c_str();
873-
874-
TMPL::asyncMethodCallExecutionStart(
875-
moduleName, methodName, id);
876-
env->CallVoidMethodA(instance.get(), methodID, jargs.data());
877-
try {
878-
FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
879-
} catch (...) {
880-
TMPL::asyncMethodCallExecutionFail(
881-
moduleName, methodName, id);
882-
throw;
883-
}
884-
885-
for (auto globalRef : globalRefs) {
886-
env->DeleteGlobalRef(globalRef);
887-
}
888-
TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id);
889-
});
890-
891-
return jsi::Value::undefined();
844+
for (auto globalRef : globalRefs) {
845+
env->DeleteGlobalRef(globalRef);
846+
}
847+
TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id);
892848
});
893-
894-
jsi::Value promise =
895-
Promise.callAsConstructor(runtime, promiseConstructorArg);
896-
checkJNIErrorForMethodCall();
897-
898849
TMPL::asyncMethodCallEnd(moduleName, methodName);
899-
900-
return promise;
850+
return jsPromise;
901851
}
902852
default:
903853
throw std::runtime_error(

0 commit comments

Comments
 (0)