From bd8e6cd2e4270981cd93c926cef062f4a302d45a Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Fri, 13 Sep 2019 18:35:33 -0700 Subject: [PATCH] Fix JNI local reference table overflow problem 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 --- .../core/platform/android/JavaTurboModule.cpp | 58 ++++++++++++++----- .../core/platform/android/JavaTurboModule.h | 2 +- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp index e6fb5bb0fb3460..21b0287e7794ce 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp @@ -237,6 +237,9 @@ std::vector JavaTurboModule::convertJSIArgsToJNIArgs( auto jargs = std::vector(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); @@ -283,7 +286,10 @@ std::vector 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, "", "(D)V"); jarg->l = @@ -297,7 +303,10 @@ std::vector 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, "", "(Z)V"); jarg->l = @@ -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()); @@ -402,16 +435,13 @@ jsi::Value JavaTurboModule::invokeJavaMethod( std::vector 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 jargs = convertJSIArgsToJNIArgs( env, runtime, methodName, methodArgTypes, args, - count, + argCount, jsInvoker_, valueKind); @@ -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, @@ -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, "", "(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(); diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h index e08894375466eb..a63cc4c57e1bd1 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h @@ -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