From 8b121c507170bc7a6cb76f0db68b1f77e7498a12 Mon Sep 17 00:00:00 2001
From: Pieter De Baets <pieterdb@meta.com>
Date: Mon, 1 Jul 2024 09:16:25 -0700
Subject: [PATCH] Use RAII for cleaning up global_refs in JavaTurboModule
 (#45218)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45218

Noticed that when an exception occurred we would not cleanup global_refs, leaking them in the global table.

Restructure this to use RAII and rely on JNIArgs to do the cleanup as necessary.

Changelog: [Android][Internal]

Reviewed By: RSNara

Differential Revision: D59156494

fbshipit-source-id: c89552d72387bad2a120373e78a2c545415a7c82
---
 .../android/ReactCommon/JavaTurboModule.cpp   | 53 ++++++++++---------
 1 file changed, 29 insertions(+), 24 deletions(-)

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 1fcf29ac86f..52039dd6cdb 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
@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include <cxxreact/MoveWrapper.h>
 #include <cxxreact/SystraceSection.h>
 #include <fbjni/fbjni.h>
 #include <glog/logging.h>
@@ -81,9 +82,22 @@ bool rejectTurboModulePromiseOnNativeError() {
 }
 
 struct JNIArgs {
-  JNIArgs(size_t count) : args_(count) {}
-  std::vector<jvalue> args_;
-  std::vector<jobject> globalRefs_;
+  JNIArgs(size_t count) : args(count) {}
+  JNIArgs(const JNIArgs&) = delete;
+  JNIArgs(JNIArgs&& jniArgs) noexcept = default;
+
+  JNIArgs& operator=(const JNIArgs& other) = delete;
+  JNIArgs& operator=(JNIArgs&& other) noexcept = default;
+
+  std::vector<jvalue> args;
+  std::vector<jobject> globalRefs;
+
+  ~JNIArgs() {
+    JNIEnv* env = jni::Environment::current();
+    for (auto globalRef : globalRefs) {
+      env->DeleteGlobalRef(globalRef);
+    }
+  }
 };
 
 jsi::Value createJSRuntimeError(jsi::Runtime& runtime, jsi::Value&& message) {
@@ -316,11 +330,10 @@ JNIArgs convertJSIArgsToJNIArgs(
   }
 
   JNIArgs jniArgs(valueKind == PromiseKind ? count + 1 : count);
-  auto& jargs = jniArgs.args_;
-  auto& globalRefs = jniArgs.globalRefs_;
+  auto& jargs = jniArgs.args;
+  auto& globalRefs = jniArgs.globalRefs;
 
-  auto makeGlobalIfNecessary =
-      [&globalRefs, env, valueKind](jobject obj) -> jobject {
+  auto makeGlobalIfNecessary = [&](jobject obj) {
     if (valueKind == VoidKind || valueKind == PromiseKind) {
       jobject globalObj = env->NewGlobalRef(obj);
       globalRefs.push_back(globalObj);
@@ -634,8 +647,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
     TMPL::syncMethodCallExecutionStart(moduleName, methodName);
   }
 
-  auto& jargs = jniArgs.args_;
-  auto& globalRefs = jniArgs.globalRefs_;
+  auto& jargs = jniArgs.args;
+  auto& globalRefs = jniArgs.globalRefs;
 
   switch (valueKind) {
     case BooleanKind: {
@@ -820,8 +833,7 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
 
       nativeMethodCallInvoker_->invokeAsync(
           methodName,
-          [jargs,
-           globalRefs,
+          [jniArgs = makeMoveWrapper(std::move(jniArgs)),
            methodID,
            instance_ = jni::make_weak(instance_),
            moduleNameStr = name_,
@@ -848,17 +860,14 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
             const char* methodName = methodNameStr.c_str();
 
             TMPL::asyncMethodCallExecutionStart(moduleName, methodName, id);
-            env->CallVoidMethodA(instance.get(), methodID, jargs.data());
+            env->CallVoidMethodA(
+                instance.get(), methodID, jniArgs->args.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);
           });
 
@@ -933,10 +942,9 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
       TMPL::asyncMethodCallDispatch(moduleName, methodName);
       nativeMethodCallInvoker_->invokeAsync(
           methodName,
-          [jargs,
+          [jniArgs = makeMoveWrapper(std::move(jniArgs)),
            rejectCallback = std::move(nativeRejectCallback),
            jsInvocationStack = std::move(jsInvocationStack),
-           globalRefs,
            methodID,
            instance_ = jni::make_weak(instance_),
            moduleNameStr = name_,
@@ -962,24 +970,21 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
             const char* moduleName = moduleNameStr.c_str();
             const char* methodName = methodNameStr.c_str();
             TMPL::asyncMethodCallExecutionStart(moduleName, methodName, id);
-            env->CallVoidMethodA(instance.get(), methodID, jargs.data());
+            env->CallVoidMethodA(
+                instance.get(), methodID, jniArgs->args.data());
             try {
               FACEBOOK_JNI_THROW_PENDING_EXCEPTION();
             } catch (...) {
-              TMPL::asyncMethodCallExecutionFail(moduleName, methodName, id);
               if (rejectTurboModulePromiseOnNativeError() && rejectCallback) {
                 auto exception = std::current_exception();
                 rejectWithException(
                     *rejectCallback, exception, jsInvocationStack);
                 rejectCallback = std::nullopt;
               } else {
+                TMPL::asyncMethodCallExecutionFail(moduleName, methodName, id);
                 throw;
               }
             }
-
-            for (auto globalRef : globalRefs) {
-              env->DeleteGlobalRef(globalRef);
-            }
             TMPL::asyncMethodCallExecutionEnd(moduleName, methodName, id);
           });
       TMPL::asyncMethodCallEnd(moduleName, methodName);