Skip to content

Commit

Permalink
Fix retain cycle in ComponentNameResolverManager (facebook#43611)
Browse files Browse the repository at this point in the history
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]

Reviewed By: sammy-SC

Differential Revision: D55240490
  • Loading branch information
javache authored and facebook-github-bot committed Mar 22, 2024
1 parent c835f91 commit 183e4dc
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand All @@ -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() {
Expand All @@ -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));
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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,
Expand All @@ -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);
};
Expand Down

0 comments on commit 183e4dc

Please sign in to comment.