Reject debugger connections to unknown page IDs

packages/dev-middleware/src/__tests__/InspectorProxyCdpTransport-test.js

@@ -268,5 +268,123 @@ describe.each(['HTTP', 'HTTPS'])(
         debugger2?.close();
       }
     });
+
+    test('debugger connection to a nonexistent page is rejected', async () => {
+      let device, debugger_;
+      try {
+        device = await createDeviceMock(
+          `${serverRef.serverBaseWsUrl}/inspector/device?device=device1&name=foo&app=bar`,
+          autoCleanup.signal,
+        );
+        // Set up a page.
+        device.getPages.mockImplementation(() => [
+          {
+            app: 'bar-app',
+            id: 'page1',
+            title: 'bar-title',
+            vm: 'bar-vm',
+          },
+        ]);
+        let pageList: Array<PageDescription> = [];
+        await until(async () => {
+          pageList = (await fetchJson(
+            `${serverRef.serverBaseUrl}/json`,
+            // $FlowIgnore[unclear-type]
+          ): any);
+          expect(pageList).toHaveLength(1);
+        });
+        const [{webSocketDebuggerUrl}] = pageList;
+
+        // Connect the debugger to a nonexistent page.
+        debugger_ = await createDebuggerMock(
+          webSocketDebuggerUrl.replaceAll('page1', 'some-other-id'),
+          autoCleanup.signal,
+        );
+
+        // The debugger gets disconnected automatically.
+        await until(async () => {
+          expect([
+            // CLOSING
+            3,
+            // CLOSED
+            4,
+          ]).toContain(debugger_.socket.readyState);
+        });
+
+        expect(device.connect).not.toHaveBeenCalled();
+        expect(device.disconnect).not.toHaveBeenCalled();
+      } finally {
+        device?.close();
+        debugger_?.close();
+      }
+    });
+
+    test('debugger connection to a nonexistent page does not kill the current debugger connection', async () => {
+      let device, debugger1, debugger2;
+      try {
+        device = await createDeviceMock(
+          `${serverRef.serverBaseWsUrl}/inspector/device?device=device1&name=foo&app=bar`,
+          autoCleanup.signal,
+        );
+        // Set up a page.
+        device.getPages.mockImplementation(() => [
+          {
+            app: 'bar-app',
+            id: 'page1',
+            title: 'bar-title',
+            vm: 'bar-vm',
+          },
+        ]);
+        let pageList: Array<PageDescription> = [];
+        await until(async () => {
+          pageList = (await fetchJson(
+            `${serverRef.serverBaseUrl}/json`,
+            // $FlowIgnore[unclear-type]
+          ): any);
+          expect(pageList).toHaveLength(1);
+        });
+        const [{webSocketDebuggerUrl}] = pageList;
+
+        // Connect the first debugger.
+        debugger1 = await createDebuggerMock(
+          webSocketDebuggerUrl,
+          autoCleanup.signal,
+        );
+
+        // Connect a second debugger to a nonexistent page.
+        debugger2 = await createDebuggerMock(
+          webSocketDebuggerUrl.replaceAll('page1', 'some-other-id'),
+          autoCleanup.signal,
+        );
+
+        // The second debugger gets disconnected automatically.
+        await until(async () => {
+          expect([
+            // CLOSING
+            3,
+            // CLOSED
+            4,
+          ]).toContain(debugger2.socket.readyState);
+        });
+
+        // We can still send messages through the first debugger.
+        await sendFromDebuggerToTarget(debugger1, device, 'page1', {
+          method: 'Runtime.enable',
+          id: 0,
+        });
+
+        expect(device.connect).toHaveBeenCalledWith({
+          event: 'connect',
+          payload: {
+            pageId: 'page1',
+          },
+        });
+        expect(device.disconnect).not.toHaveBeenCalled();
+      } finally {
+        device?.close();
+        debugger1?.close();
+        debugger2?.close();
+      }
+    });
   },
 );

packages/dev-middleware/src/inspector-proxy/Device.js

@@ -308,17 +308,30 @@ export default class Device {
       userAgent: string | null,
     }>,
   ) {
+    const page: ?Page =
+      pageId === REACT_NATIVE_RELOADABLE_PAGE_ID
+        ? this.#createSyntheticPage()
+        : this.#pages.get(pageId);
+
+    if (!page) {
+      debug(
+        `Got new debugger connection for page ${pageId} of ${this.#name}, but no such page exists`,
+      );
+      socket.close();
+      return;
+    }
+
     // Clear any commands we were waiting on.
     this.#deviceEventReporter?.logDisconnection('debugger');
 
+    // Disconnect current debugger if we already have debugger connected.
+    this.#terminateDebuggerConnection();
+
     this.#deviceEventReporter?.logConnection('debugger', {
       pageId,
       frontendUserAgent: metadata.userAgent,
     });
 
-    // Disconnect current debugger if we already have debugger connected.
-    this.#terminateDebuggerConnection();
-
     const debuggerInfo = {
       socket,
       prependedFilePrefix: false,
@@ -327,18 +340,11 @@ export default class Device {
       customHandler: null,
     };
 
-    // TODO(moti): Handle null case explicitly, e.g. refuse to connect to
-    // unknown pages.
-    const page: ?Page =
-      pageId === REACT_NATIVE_RELOADABLE_PAGE_ID
-        ? this.#createSyntheticPage()
-        : this.#pages.get(pageId);
-
     this.#debuggerConnection = debuggerInfo;
 
     debug(`Got new debugger connection for page ${pageId} of ${this.#name}`);
 
-    if (page && this.#debuggerConnection && this.#createCustomMessageHandler) {
+    if (this.#debuggerConnection && this.#createCustomMessageHandler) {
       this.#debuggerConnection.customHandler = this.#createCustomMessageHandler(
         {
           page,
@@ -405,7 +411,7 @@ export default class Device {
         return;
       }
 
-      if (!page || !this.#pageHasCapability(page, 'nativeSourceCodeFetching')) {
+      if (!this.#pageHasCapability(page, 'nativeSourceCodeFetching')) {
         processedReq = this.#interceptClientMessageForSourceFetching(
           debuggerRequest,
           debuggerInfo,

packages/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;
   }
 }

packages/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