Fix retain cycle in ComponentNameResolverManager

Summary: Storing a global_ref to `jThis` in a JNI hybrid object leads to a reference cycle which cannot be cleaned up by Java GC. The Java object will keep the C++ object alive and vice versa. In this class, we didn't seem to need this reference anyway. Changelog: [Internal] Differential Revision: D55240490
facebook · Mar 22, 2024 · 42c73f3 · 42c73f3
1 parent 0a12557
commit 42c73f3
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 33 deletions.
diff --git a/...s/react-native/ReactAndroid/src/main/jni/react/uimanager/ComponentNameResolverManager.cpp b/...s/react-native/ReactAndroid/src/main/jni/react/uimanager/ComponentNameResolverManager.cpp
@@ -5,11 +5,6 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-#include <string>
-
-#include <fbjni/fbjni.h>
-#include <jsi/jsi.h>
-
 #include "ComponentNameResolverManager.h"
 
 #include <react/renderer/componentregistry/native/NativeComponentRegistryBinding.h>
@@ -18,12 +13,13 @@ namespace facebook::react {
 
 using namespace facebook::jni;
 
+constexpr static auto ComponentNameResolverJavaDescriptor =
+    "com/facebook/react/uimanager/ComponentNameResolver";
+
 ComponentNameResolverManager::ComponentNameResolverManager(
-    jni::alias_ref<ComponentNameResolverManager::javaobject> jThis,
     RuntimeExecutor runtimeExecutor,
     jni::alias_ref<jobject> componentNameResolver)
-    : javaPart_(jni::make_global(jThis)),
-      runtimeExecutor_(runtimeExecutor),
+    : runtimeExecutor_(std::move(runtimeExecutor)),
       componentNameResolver_(jni::make_global(componentNameResolver)) {}
 
 jni::local_ref<ComponentNameResolverManager::jhybriddata>
@@ -32,7 +28,7 @@ ComponentNameResolverManager::initHybrid(
     jni::alias_ref<JRuntimeExecutor::javaobject> runtimeExecutor,
     jni::alias_ref<jobject> componentNameResolver) {
   return makeCxxInstance(
-      jThis, runtimeExecutor->cthis()->get(), componentNameResolver);
+      runtimeExecutor->cthis()->get(), componentNameResolver);
 }
 
 void ComponentNameResolverManager::registerNatives() {
@@ -45,28 +41,23 @@ void ComponentNameResolverManager::registerNatives() {
 }
 
 void ComponentNameResolverManager::installJSIBindings() {
-  runtimeExecutor_([thizz = this](jsi::Runtime& runtime) {
-    auto viewManagerProvider = [thizz](const std::string& name) -> bool {
-      if (thizz->componentNames_.size() == 0) {
+  runtimeExecutor_([this](jsi::Runtime& runtime) {
+    auto viewManagerProvider = [this](const std::string& name) -> bool {
+      if (componentNames_.size() == 0) {
         static auto getComponentNames =
-            jni::findClassStatic(ComponentNameResolverManager::
-                                     ComponentNameResolverJavaDescriptor)
+            jni::findClassStatic(ComponentNameResolverJavaDescriptor)
                 ->getMethod<jni::alias_ref<jtypeArray<jstring>>()>(
                     "getComponentNames");
 
-        auto componentNamesJArray =
-            getComponentNames(thizz->componentNameResolver_.get());
+        auto componentNamesJArray = getComponentNames(componentNameResolver_);
         auto len = componentNamesJArray->size();
         for (size_t i = 0; i < len; i++) {
           jni::local_ref<jstring> elem = (*componentNamesJArray)[i];
-          auto componentName = elem->toStdString();
-          thizz->componentNames_.insert(componentName);
+          componentNames_.insert(elem->toStdString());
         }
       }
-
-      return thizz->componentNames_.find(name) != thizz->componentNames_.end();
+      return componentNames_.find(name) != componentNames_.end();
     };
-
     bindHasComponentProvider(runtime, std::move(viewManagerProvider));
   });
 }

diff --git a/...ges/react-native/ReactAndroid/src/main/jni/react/uimanager/ComponentNameResolverManager.h b/...ges/react-native/ReactAndroid/src/main/jni/react/uimanager/ComponentNameResolverManager.h
@@ -7,11 +7,10 @@
 
 #pragma once
 
-#include <ReactCommon/CallInvokerHolder.h>
-#include <ReactCommon/RuntimeExecutor.h>
+#include <unordered_set>
+
 #include <fbjni/fbjni.h>
 #include <react/jni/JRuntimeExecutor.h>
-#include <set>
 
 namespace facebook::react {
 
@@ -21,9 +20,6 @@ class ComponentNameResolverManager
   static auto constexpr kJavaDescriptor =
       "Lcom/facebook/react/uimanager/ComponentNameResolverManager;";
 
-  constexpr static auto ComponentNameResolverJavaDescriptor =
-      "com/facebook/react/uimanager/ComponentNameResolver";
-
   static facebook::jni::local_ref<jhybriddata> initHybrid(
       facebook::jni::alias_ref<jhybridobject> jThis,
       facebook::jni::alias_ref<JRuntimeExecutor::javaobject> runtimeExecutor,
@@ -33,18 +29,14 @@ class ComponentNameResolverManager
 
  private:
   friend HybridBase;
-  facebook::jni::global_ref<ComponentNameResolverManager::javaobject> javaPart_;
-  RuntimeExecutor runtimeExecutor_;
 
+  RuntimeExecutor runtimeExecutor_;
   facebook::jni::global_ref<jobject> componentNameResolver_;
-
-  std::set<std::string> componentNames_;
+  std::unordered_set<std::string> componentNames_;
 
   void installJSIBindings();
 
   explicit ComponentNameResolverManager(
-      facebook::jni::alias_ref<ComponentNameResolverManager::jhybridobject>
-          jThis,
       RuntimeExecutor runtimeExecutor,
       facebook::jni::alias_ref<jobject> componentNameResolver);
 };