Skip to content

Commit

Permalink
Fix JNI local reference table overflow problem
Browse files Browse the repository at this point in the history
Summary:
This diff improves on D17244061 by moving the `Jni::JniLocalScope` to earlier on in `JavaTurboModule::invokeJavaMethod`. The problem with D17244061 was two-fold:
1. It completely ignored the local references created by just calling `getConstants()`. So, you could spam `getConstants()` and still get an overflow.
2. The capacity was incorrect. Basically, two things happen when you call `PushLocalFrame`. One, we push an actual Jni frame onto a stack somewhere in the dvm: https://fburl.com/f16wfrxi, https://fburl.com/jhpha563. Popping off this stack is  how the local references are cleared when we call `PopLocalFrame`. Two, we do a check to make sure that we can support at least `capacity` more local references: https://fburl.com/jucvew8j. The max is 512. The problem before was that, I was just using the argument count for the capacity. This was inaccurate because we create many `jclass` objects, and several misc. intermediate java objects when we invoke `JavaTurboModule::invokeJavaMethod`. So, I've refined the calculation of capacity to be a bit more accurate. This should make sure that capacity check works, which should help us fail faster if our local reference count approaches 512.

Reviewed By: shergin

Differential Revision: D17337354

fbshipit-source-id: 45574bae4748c52d8f919c1480b9a0936d970628
  • Loading branch information
RSNara authored and facebook-github-bot committed Sep 14, 2019
1 parent 4b59360 commit bd8e6cd
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 14 deletions.
58 changes: 45 additions & 13 deletions ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,9 @@ std::vector<jvalue> JavaTurboModule::convertJSIArgsToJNIArgs(
auto jargs =
std::vector<jvalue>(valueKind == PromiseKind ? count + 1 : count);

jclass booleanClass = nullptr;
jclass doubleClass = nullptr;

for (unsigned int argIndex = 0; argIndex < count; argIndex += 1) {
std::string type = methodArgTypes.at(argIndex);

Expand Down Expand Up @@ -283,7 +286,10 @@ std::vector<jvalue> JavaTurboModule::convertJSIArgsToJNIArgs(
"number", argIndex, methodName, arg, &rt);
}

jclass doubleClass = env->FindClass("java/lang/Double");
if (doubleClass == nullptr) {
doubleClass = env->FindClass("java/lang/Double");
}

jmethodID doubleConstructor =
env->GetMethodID(doubleClass, "<init>", "(D)V");
jarg->l =
Expand All @@ -297,7 +303,10 @@ std::vector<jvalue> JavaTurboModule::convertJSIArgsToJNIArgs(
"boolean", argIndex, methodName, arg, &rt);
}

jclass booleanClass = env->FindClass("java/lang/Boolean");
if (booleanClass == nullptr) {
booleanClass = env->FindClass("java/lang/Boolean");
}

jmethodID booleanConstructor =
env->GetMethodID(booleanClass, "<init>", "(Z)V");
jarg->l =
Expand Down Expand Up @@ -379,10 +388,34 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
const std::string &methodName,
const std::string &methodSignature,
const jsi::Value *args,
size_t count) {
size_t argCount) {
JNIEnv *env = jni::Environment::current();
auto instance = instance_.get();

/**
* To account for jclasses and other misc LocalReferences we create.
*/
unsigned int buffer = 6;
/**
* For promises, we have to create a resolve fn, a reject fn, and a promise
* object. For normal returns, we just create the return object.
*/
unsigned int maxReturnObjects = 3;
unsigned int estimatedLocalRefCount = argCount + maxReturnObjects + buffer;

/**
* This will push a new JNI stack frame for the LocalReferences in this
* function call. When the stack frame for invokeJavaMethod is popped,
* all LocalReferences are deleted.
*
* In total, there can be at most kJniLocalRefMax (= 512) Jni
* LocalReferences alive at a time. estimatedLocalRefCount is provided
* so that PushLocalFrame can throw an out of memory error when the total
* number of alive LocalReferences is estimatedLocalRefCount smaller than
* kJniLocalRefMax.
*/
jni::JniLocalScope scope(env, estimatedLocalRefCount);

jclass cls = env->GetObjectClass(instance);
jmethodID methodID =
env->GetMethodID(cls, methodName.c_str(), methodSignature.c_str());
Expand All @@ -402,16 +435,13 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
std::vector<std::string> methodArgTypes =
getMethodArgTypesFromSignature(methodSignature);

// Ensure all Java arguments created for the method invocation are released
// after the method call.
jni::JniLocalScope scope(env, valueKind == PromiseKind ? count + 1 : count);
std::vector<jvalue> jargs = convertJSIArgsToJNIArgs(
env,
runtime,
methodName,
methodArgTypes,
args,
count,
argCount,
jsInvoker_,
valueKind);

Expand Down Expand Up @@ -523,7 +553,7 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
runtime,
jsi::PropNameID::forAscii(runtime, "fn"),
2,
[this, &jargs, count, instance, methodID, env](
[this, &jargs, argCount, instance, methodID, env](
jsi::Runtime &runtime,
const jsi::Value &thisVal,
const jsi::Value *promiseConstructorArgs,
Expand All @@ -546,15 +576,17 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
rejectJSIFn, runtime, jsInvoker_)
.release();

jclass cls =
jclass jPromiseImpl =
env->FindClass("com/facebook/react/bridge/PromiseImpl");
jmethodID constructor = env->GetMethodID(
cls,
jmethodID jPromiseImplConstructor = env->GetMethodID(
jPromiseImpl,
"<init>",
"(Lcom/facebook/react/bridge/Callback;Lcom/facebook/react/bridge/Callback;)V");
jobject promise = env->NewObject(cls, constructor, resolve, reject);

jargs[count].l = promise;
jobject promise = env->NewObject(
jPromiseImpl, jPromiseImplConstructor, resolve, reject);

jargs[argCount].l = promise;
env->CallVoidMethodA(instance, methodID, jargs.data());

return jsi::Value::undefined();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
const std::string &methodName,
const std::string &methodSignature,
const jsi::Value *args,
size_t count);
size_t argCount);

/**
* This dtor must be called from the JS Thread, since it accesses
Expand Down

0 comments on commit bd8e6cd

Please sign in to comment.