Keep HostTarget registered until ReactHostImpl/ReactInstanceManager i…

…s invalidated (#45146) Summary: Pull Request resolved: #45146 Changelog: [Internal] Currently, on Android, we destroy the Fusebox `HostTarget` when we receive the `onHostDestroy` event, which (counterintuitively) does not mean the ReactHost/InstanceManager ("Java Host") is being destroyed. This can lead to situations where the `HostTarget` is destroyed too soon (e.g. when a single Java Host is reused across multiple Activities). Now that we have the `invalidate()` method on the Java Host classes, we can tie `HostTarget`'s destruction to that instead. Since calling `invalidate()` is explicitly optional, we also need to account for the case where the caller just lets go of the Java Host reference and expects GC to handle cleanup. This includes: * Breaking the retain cycle between the Java Host and its C++ part. We achieve this using `WeakReference` to reference the Java Host. * Making the C++ part of the Host safe to destroy from any thread (and in particular the finalizer thread). We achieve this by scheduling `HostTarget`'s unregistration (in C++) on the executor supplied by the Java Host. Reviewed By: hoxyq Differential Revision: D58284590 fbshipit-source-id: 4ee4780354fb81137b891d5891d6138ac215cbff
facebook · Jun 24, 2024 · 4a8f0ee · 4a8f0ee
1 parent a7adfef
commit 4a8f0ee
Show file tree

Hide file tree

Showing 7 changed files with 151 additions and 109 deletions.
diff --git a/...ages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/...ages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
@@ -113,6 +113,7 @@
 import com.facebook.soloader.SoLoader;
 import com.facebook.systrace.Systrace;
 import com.facebook.systrace.SystraceMessage;
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -781,10 +782,13 @@ public void destroy() {
       }
     }
 
-    // If the host is being destroyed, now that the current context/instance
+    // If the host has been invalidated, now that the current context/instance
     // has been destroyed, we can safely destroy the host's inspector target.
-    if (mLifecycleState == LifecycleState.BEFORE_CREATE) {
-      destroyInspectorHostTarget();
+    if (mInstanceManagerInvalidated) {
+      if (mInspectorTarget != null) {
+        mInspectorTarget.close();
+        mInspectorTarget = null;
+      }
     }
 
     mHasStartedCreatingInitialContext = false;
@@ -840,10 +844,6 @@ private synchronized void moveToBeforeCreateLifecycleState() {
       if (mLifecycleState == LifecycleState.BEFORE_RESUME) {
         currentContext.onHostDestroy(mKeepActivity);
       }
-    } else {
-      // There's no current context that requires the host inspector target to
-      // be kept alive, so we can destroy it immediately.
-      destroyInspectorHostTarget();
     }
     mLifecycleState = LifecycleState.BEFORE_CREATE;
   }
@@ -1539,46 +1539,59 @@ private void processPackage(
     SystraceMessage.endSection(TRACE_TAG_REACT_JAVA_BRIDGE).flush();
   }
 
-  private @Nullable ReactInstanceManagerInspectorTarget getOrCreateInspectorTarget() {
-    if (mInspectorTarget == null && InspectorFlags.getFuseboxEnabled()) {
+  private static class InspectorTargetDelegateImpl
+      implements ReactInstanceManagerInspectorTarget.TargetDelegate {
+    // This weak reference breaks the cycle between the C++ HostTarget and the
+    // Java ReactInstanceManager, preventing memory leaks in apps that create
+    // multiple ReactInstanceManagers over time.
+    private WeakReference<ReactInstanceManager> mReactInstanceManagerWeak;
 
-      mInspectorTarget =
-          new ReactInstanceManagerInspectorTarget(
-              new ReactInstanceManagerInspectorTarget.TargetDelegate() {
-                @Override
-                public void onReload() {
-                  UiThreadUtil.runOnUiThread(() -> mDevSupportManager.handleReloadJS());
-                }
+    public InspectorTargetDelegateImpl(ReactInstanceManager inspectorTarget) {
+      mReactInstanceManagerWeak = new WeakReference<ReactInstanceManager>(inspectorTarget);
+    }
 
-                @Override
-                public void onSetPausedInDebuggerMessage(@Nullable String message) {
-                  if (message == null) {
-                    mDevSupportManager.hidePausedInDebuggerOverlay();
-                  } else {
-                    mDevSupportManager.showPausedInDebuggerOverlay(
-                        message,
-                        new PausedInDebuggerOverlayCommandListener() {
-                          @Override
-                          public void onResume() {
-                            UiThreadUtil.assertOnUiThread();
-                            if (mInspectorTarget != null) {
-                              mInspectorTarget.sendDebuggerResumeCommand();
-                            }
-                          }
-                        });
-                  }
-                }
-              });
+    @Override
+    public void onReload() {
+      UiThreadUtil.runOnUiThread(
+          () -> {
+            ReactInstanceManager reactInstanceManager = mReactInstanceManagerWeak.get();
+            if (reactInstanceManager != null) {
+              reactInstanceManager.mDevSupportManager.handleReloadJS();
+            }
+          });
     }
 
-    return mInspectorTarget;
+    @Override
+    public void onSetPausedInDebuggerMessage(@Nullable String message) {
+      ReactInstanceManager reactInstanceManager = mReactInstanceManagerWeak.get();
+      if (reactInstanceManager == null) {
+        return;
+      }
+      if (message == null) {
+        reactInstanceManager.mDevSupportManager.hidePausedInDebuggerOverlay();
+      } else {
+        reactInstanceManager.mDevSupportManager.showPausedInDebuggerOverlay(
+            message,
+            new PausedInDebuggerOverlayCommandListener() {
+              @Override
+              public void onResume() {
+                UiThreadUtil.assertOnUiThread();
+                if (reactInstanceManager.mInspectorTarget != null) {
+                  reactInstanceManager.mInspectorTarget.sendDebuggerResumeCommand();
+                }
+              }
+            });
+      }
+    }
   }
 
-  @ThreadConfined(UI)
-  private void destroyInspectorHostTarget() {
-    if (mInspectorTarget != null) {
-      mInspectorTarget.close();
-      mInspectorTarget = null;
+  private @Nullable ReactInstanceManagerInspectorTarget getOrCreateInspectorTarget() {
+    if (mInspectorTarget == null && InspectorFlags.getFuseboxEnabled()) {
+
+      mInspectorTarget =
+          new ReactInstanceManagerInspectorTarget(new InspectorTargetDelegateImpl(this));
     }
+
+    return mInspectorTarget;
   }
 }
diff --git a/...ges/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java b/...ges/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.java
@@ -893,11 +893,6 @@ private Task<Void> getOrCreateStartTask() {
   @ThreadConfined(UI)
   private void moveToHostDestroy(@Nullable ReactContext currentContext) {
     mReactLifecycleStateManager.moveToOnHostDestroy(currentContext);
-    if (currentContext == null) {
-      // There's no current context/instance that requires the host inspector
-      // target to be kept alive, so we can destroy it immediately.
-      destroyInspectorHostTarget();
-    }
     setCurrentActivity(null);
   }
 
@@ -1545,6 +1540,16 @@ private Task<Void> getOrCreateDestroyTask(final String reason, @Nullable Excepti
 
                     unregisterInstanceFromInspector(reactInstance);
 
+                    if (mHostInvalidated) {
+                      // If the host has been invalidated, now that the current context/instance
+                      // has been unregistered, we can safely destroy the host's inspector
+                      // target.
+                      if (mReactHostInspectorTarget != null) {
+                        mReactHostInspectorTarget.close();
+                        mReactHostInspectorTarget = null;
+                      }
+                    }
+
                     // Step 1: Destroy DevSupportManager
                     if (mUseDevSupport) {
                       log(method, "DevSupportManager cleanup");
@@ -1698,20 +1703,13 @@ public void setJsEngineResolutionAlgorithm(
 
   private @Nullable ReactHostInspectorTarget getOrCreateReactHostInspectorTarget() {
     if (mReactHostInspectorTarget == null && InspectorFlags.getFuseboxEnabled()) {
+      // NOTE: ReactHostInspectorTarget only retains a weak reference to `this`.
       mReactHostInspectorTarget = new ReactHostInspectorTarget(this);
     }
 
     return mReactHostInspectorTarget;
   }
 
-  @ThreadConfined(UI)
-  private void destroyInspectorHostTarget() {
-    if (mReactHostInspectorTarget != null) {
-      mReactHostInspectorTarget.close();
-      mReactHostInspectorTarget = null;
-    }
-  }
-
   @ThreadConfined(UI)
   private void unregisterInstanceFromInspector(final @Nullable ReactInstance reactInstance) {
     if (reactInstance != null) {
@@ -1722,12 +1720,6 @@ private void unregisterInstanceFromInspector(final @Nullable ReactInstance react
       }
       reactInstance.unregisterFromInspector();
     }
-    if (mReactLifecycleStateManager.getLifecycleState() == LifecycleState.BEFORE_CREATE) {
-      // If the host is being destroyed, now that the current context/instance
-      // has been unregistered, we can safely destroy the host's inspector
-      // target.
-      destroyInspectorHostTarget();
-    }
   }
 
   @Override

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JWeakRefUtils.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JWeakRefUtils.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#pragma once
+
+#include <fbjni/fbjni.h>
+
+namespace facebook::react {
+
+/**
+ * Helper for constructing a JWeakReference. \see JWeakReference.h in fbjni.
+ */
+template <typename T>
+inline jni::local_ref<jni::JWeakReference<T>> makeJWeakReference(
+    jni::alias_ref<T> ref) {
+  return jni::JWeakReference<T>::newInstance(ref);
+}
+
+} // namespace facebook::react
diff --git a/.../react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp b/.../react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp
@@ -31,24 +31,22 @@ void ReactInstanceManagerInspectorTarget::TargetDelegate::
 
 ReactInstanceManagerInspectorTarget::ReactInstanceManagerInspectorTarget(
     jni::alias_ref<ReactInstanceManagerInspectorTarget::jhybridobject> jobj,
-    jni::alias_ref<JExecutor::javaobject> executor,
-    jni::alias_ref<
-        ReactInstanceManagerInspectorTarget::TargetDelegate::javaobject>
+    jni::alias_ref<JExecutor::javaobject> javaExecutor,
+    jni::alias_ref<ReactInstanceManagerInspectorTarget::TargetDelegate>
         delegate)
-    : delegate_(make_global(delegate)) {
+    : delegate_(make_global(delegate)),
+      inspectorExecutor_([javaExecutor =
+                              // Use a SafeReleaseJniRef because this lambda may
+                              // be copied to arbitrary threads.
+                          SafeReleaseJniRef(make_global(javaExecutor))](
+                             auto callback) mutable {
+        auto jrunnable = JNativeRunnable::newObjectCxxArgs(std::move(callback));
+        javaExecutor->execute(jrunnable);
+      }) {
   auto& inspectorFlags = InspectorFlags::getInstance();
 
   if (inspectorFlags.getFuseboxEnabled()) {
-    inspectorTarget_ = HostTarget::create(
-        *this,
-        [javaExecutor =
-             // Use a SafeReleaseJniRef because this lambda may be copied to
-             // arbitrary threads.
-         SafeReleaseJniRef(make_global(executor))](auto callback) mutable {
-          auto jrunnable =
-              JNativeRunnable::newObjectCxxArgs(std::move(callback));
-          javaExecutor->execute(jrunnable);
-        });
+    inspectorTarget_ = HostTarget::create(*this, inspectorExecutor_);
 
     inspectorPageId_ = getInspectorInstance().addPage(
         "React Native Bridge (Experimental)",
@@ -64,18 +62,24 @@ ReactInstanceManagerInspectorTarget::ReactInstanceManagerInspectorTarget(
 
 ReactInstanceManagerInspectorTarget::~ReactInstanceManagerInspectorTarget() {
   if (inspectorPageId_.has_value()) {
-    getInspectorInstance().removePage(*inspectorPageId_);
+    // Remove the page (terminating all sessions) and destroy the target, both
+    // on the inspector queue.
+    inspectorExecutor_([inspectorPageId = *inspectorPageId_,
+                        inspectorTarget = std::move(inspectorTarget_)]() {
+      getInspectorInstance().removePage(inspectorPageId);
+      (void)inspectorTarget;
+    });
   }
 }
 
 jni::local_ref<ReactInstanceManagerInspectorTarget::jhybriddata>
 ReactInstanceManagerInspectorTarget::initHybrid(
     jni::alias_ref<jhybridobject> jobj,
-    jni::alias_ref<JExecutor::javaobject> executor,
+    jni::alias_ref<JExecutor::javaobject> javaExecutor,
     jni::alias_ref<
         ReactInstanceManagerInspectorTarget::TargetDelegate::javaobject>
         delegate) {
-  return makeCxxInstance(jobj, executor, delegate);
+  return makeCxxInstance(jobj, javaExecutor, delegate);
 }
 
 void ReactInstanceManagerInspectorTarget::sendDebuggerResumeCommand() {

diff --git a/...es/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h b/...es/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h
@@ -39,7 +39,7 @@ class ReactInstanceManagerInspectorTarget
 
   static jni::local_ref<jhybriddata> initHybrid(
       jni::alias_ref<jhybridobject> jobj,
-      jni::alias_ref<JExecutor::javaobject> executor,
+      jni::alias_ref<JExecutor::javaobject> javaExecutor,
       jni::alias_ref<
           ReactInstanceManagerInspectorTarget::TargetDelegate::javaobject>
           delegate);
@@ -61,14 +61,13 @@ class ReactInstanceManagerInspectorTarget
 
   ReactInstanceManagerInspectorTarget(
       jni::alias_ref<ReactInstanceManagerInspectorTarget::jhybridobject> jobj,
-      jni::alias_ref<JExecutor::javaobject> executor,
-      jni::alias_ref<
-          ReactInstanceManagerInspectorTarget::TargetDelegate::javaobject>
+      jni::alias_ref<JExecutor::javaobject> javaExecutor,
+      jni::alias_ref<ReactInstanceManagerInspectorTarget::TargetDelegate>
           delegate);
 
-  jni::global_ref<
-      ReactInstanceManagerInspectorTarget::TargetDelegate::javaobject>
+  jni::global_ref<ReactInstanceManagerInspectorTarget::TargetDelegate>
       delegate_;
+  jsinspector_modern::VoidExecutor inspectorExecutor_;
   std::shared_ptr<jsinspector_modern::HostTarget> inspectorTarget_;
   std::optional<int> inspectorPageId_;
 };

diff --git a/...es/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp b/...es/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp
@@ -8,30 +8,28 @@
 #include "JReactHostInspectorTarget.h"
 #include <fbjni/NativeRunnable.h>
 #include <jsinspector-modern/InspectorFlags.h>
+#include <react/jni/JWeakRefUtils.h>
 #include <react/jni/SafeReleaseJniRef.h>
 
 using namespace facebook::jni;
 using namespace facebook::react::jsinspector_modern;
 
 namespace facebook::react {
 JReactHostInspectorTarget::JReactHostInspectorTarget(
-    alias_ref<JReactHostImpl::javaobject> reactHostImpl,
+    alias_ref<JReactHostImpl> reactHostImpl,
     alias_ref<JExecutor::javaobject> executor)
-    : javaReactHostImpl_(make_global(reactHostImpl)),
-      javaExecutor_(make_global(executor)) {
+    : javaReactHostImpl_(make_global(makeJWeakReference(reactHostImpl))),
+      inspectorExecutor_([javaExecutor =
+                              // Use a SafeReleaseJniRef because this lambda may
+                              // be copied to arbitrary threads.
+                          SafeReleaseJniRef(make_global(executor))](
+                             std::function<void()>&& callback) mutable {
+        auto jrunnable = JNativeRunnable::newObjectCxxArgs(std::move(callback));
+        javaExecutor->execute(jrunnable);
+      }) {
   auto& inspectorFlags = InspectorFlags::getInstance();
   if (inspectorFlags.getFuseboxEnabled()) {
-    inspectorTarget_ = HostTarget::create(
-        *this,
-        [javaExecutor =
-             // Use a SafeReleaseJniRef because this lambda may be copied to
-             // arbitrary threads.
-         SafeReleaseJniRef(javaExecutor_)](
-            std::function<void()>&& callback) mutable {
-          auto jrunnable =
-              JNativeRunnable::newObjectCxxArgs(std::move(callback));
-          javaExecutor->execute(jrunnable);
-        });
+    inspectorTarget_ = HostTarget::create(*this, inspectorExecutor_);
 
     inspectorPageId_ = getInspectorInstance().addPage(
         "React Native Bridgeless (Experimental)",
@@ -51,16 +49,22 @@ JReactHostInspectorTarget::JReactHostInspectorTarget(
 
 JReactHostInspectorTarget::~JReactHostInspectorTarget() {
   if (inspectorPageId_.has_value()) {
-    getInspectorInstance().removePage(*inspectorPageId_);
+    // Remove the page (terminating all sessions) and destroy the target, both
+    // on the inspector queue.
+    inspectorExecutor_([inspectorPageId = *inspectorPageId_,
+                        inspectorTarget = std::move(inspectorTarget_)]() {
+      getInspectorInstance().removePage(inspectorPageId);
+      (void)inspectorTarget;
+    });
   }
 }
 
 local_ref<JReactHostInspectorTarget::jhybriddata>
 JReactHostInspectorTarget::initHybrid(
     alias_ref<JReactHostInspectorTarget::jhybridobject> self,
-    jni::alias_ref<JReactHostImpl::javaobject> reactHostImpl,
-    jni::alias_ref<JExecutor::javaobject> executor) {
-  return makeCxxInstance(reactHostImpl, executor);
+    jni::alias_ref<JReactHostImpl> reactHostImpl,
+    jni::alias_ref<JExecutor::javaobject> javaExecutor) {
+  return makeCxxInstance(reactHostImpl, javaExecutor);
 }
 
 void JReactHostInspectorTarget::sendDebuggerResumeCommand() {
@@ -90,12 +94,16 @@ JReactHostInspectorTarget::getMetadata() {
 }
 
 void JReactHostInspectorTarget::onReload(const PageReloadRequest& request) {
-  javaReactHostImpl_->reload("CDP Page.reload");
+  if (auto javaReactHostImplStrong = javaReactHostImpl_->get()) {
+    javaReactHostImplStrong->reload("CDP Page.reload");
+  }
 }
 
 void JReactHostInspectorTarget::onSetPausedInDebuggerMessage(
     const OverlaySetPausedInDebuggerMessageRequest& request) {
-  javaReactHostImpl_->setPausedInDebuggerMessage(request.message);
+  if (auto javaReactHostImplStrong = javaReactHostImpl_->get()) {
+    javaReactHostImplStrong->setPausedInDebuggerMessage(request.message);
+  }
 }
 
 HostTarget* JReactHostInspectorTarget::getInspectorTarget() {