Close DevTools sessions when the debugged page is removed (#58927)

Summary:
Pull Request resolved: #58927

Notice: react/react-native-devtools-frontend#285 needs to land to display the human readable error properly in the disconnection dialog, but this fix works regardless.

When a React Native host is destroyed while DevTools is attached (e.g. the app recreates its React Native instance), the DevTools frontend stayed open in a broken state: the console stopped receiving output, breakpoints were not respected, and no "disconnected" dialog was shown. Two issues combined:

1. `InspectorPackagerConnection` disconnected the local sessions when a page was removed, but the resulting `disconnect` notification was dropped because the session had already been erased, so the dev server was never told that the session ended.
2. The inspector proxy ignored device `disconnect` events for pages with the `nativePageReloads` capability, including connection rejections (e.g. when a debugger session is handed off to a reconnected device that no longer has the page), so the debugger WebSocket stayed open.

This change:
- Sends a `disconnect` event to the dev server for every session when a page is removed.
- Closes the debugger connection in the inspector proxy with a new `[PAGE_REMOVED]` close reason when the device ends or rejects a session on a `nativePageReloads` page, so the frontend shows its disconnected dialog.
- Adds a `DisconnectEventFromDevice` type with an optional `sessionId`, matching the documented protocol for legacy devices.
- Adds a test ensuring all close reasons fit within the 123-byte WebSocket close frame limit.

Changelog:
[General][Fixed] - Fix React Native DevTools staying open but unresponsive after the debugged React Native instance is destroyed

Differential Revision: D123882926

Author: vzaidman

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

@@ -0,0 +1,25 @@
+/**
+ * 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.
+ *
+ * @flow strict-local
+ * @format
+ */
+
+import {WS_CLOSE_REASON} from '../inspector-proxy/Device';
+
+// WebSocket close frames limit the reason to 123 bytes (RFC 6455, 5.5).
+const MAX_CLOSE_REASON_BYTES = 123;
+
+describe('inspector proxy WebSocket close reasons', () => {
+  test.each(Object.entries(WS_CLOSE_REASON))(
+    '%s fits in a close frame',
+    (_, reason) => {
+      expect(Buffer.byteLength(reason)).toBeLessThanOrEqual(
+        MAX_CLOSE_REASON_BYTES,
+      );
+    },
+  );
+});

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

@@ -12,11 +12,11 @@ import type {
   GetPagesResponse,
   JsonPagesListResponse,
 } from '../inspector-proxy/types';
-import type {DeviceMock} from './InspectorDeviceUtils';
 
+import {WS_CLOSE_REASON} from '../inspector-proxy/Device';
 import {fetchJson} from './FetchUtils';
 import {createDebuggerMock} from './InspectorDebuggerUtils';
-import {createDeviceMock} from './InspectorDeviceUtils';
+import {DeviceMock, createDeviceMock} from './InspectorDeviceUtils';
 import {sendFromDebuggerToTarget} from './InspectorProtocolUtils';
 import {withAbortSignalForEachTest} from './ResourceUtils';
 import {withServerForEachTest} from './ServerUtils';
@@ -301,6 +301,64 @@ describe('inspector-proxy device socket handoff', () => {
     }
   });
 
+  test("debugger handed off to a device that rejects the page with 'nativePageReloads' is disconnected", async () => {
+    let device1, device2, debugger_, webSocketDebuggerUrl;
+    try {
+      ({
+        device: device1,
+        pageList: [{webSocketDebuggerUrl}],
+      } = await connectDevice(
+        '/inspector/device?device=device&name=foo&app=bar',
+        [
+          {
+            ...PAGE_DEFAULTS,
+            capabilities: {
+              nativePageReloads: true,
+            },
+          },
+        ],
+      ));
+
+      const connectedDebugger = await createDebuggerMock(
+        webSocketDebuggerUrl,
+        autoCleanup.signal,
+      );
+      debugger_ = connectedDebugger;
+      await until(() => expect(device1.connect).toBeCalled());
+      const debuggerClosed = new Promise<{code: number, reason: string}>(
+        resolve =>
+          connectedDebugger.socket.once(
+            'close',
+            (code: number, reason: Buffer) =>
+              resolve({code, reason: reason.toString()}),
+          ),
+      );
+
+      // The reconnecting device no longer has page1, so it rejects the
+      // handed-off session.
+      const rejectingDevice = new DeviceMock(
+        serverRef.serverBaseWsUrl +
+          '/inspector/device?device=device&name=foo&app=bar',
+        autoCleanup.signal,
+      );
+      device2 = rejectingDevice;
+      rejectingDevice.connect.mockImplementation(({payload}) =>
+        rejectingDevice.send({event: 'disconnect', payload}),
+      );
+      await rejectingDevice.ready();
+
+      await until(() => expect(rejectingDevice.connect).toBeCalled());
+      expect(await debuggerClosed).toEqual({
+        code: 1000,
+        reason: WS_CLOSE_REASON.PAGE_REMOVED,
+      });
+    } finally {
+      device1?.close();
+      device2?.close();
+      debugger_?.close();
+    }
+  });
+
   test.each([
     ['app', 'name'],
     ['name', 'app'],

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

@@ -8,6 +8,7 @@
  * @format
  */
 
+import {WS_CLOSE_REASON} from '../inspector-proxy/Device';
 import {fetchJson} from './FetchUtils';
 import {createDebuggerMock} from './InspectorDebuggerUtils';
 import {createDeviceMock} from './InspectorDeviceUtils';
@@ -401,6 +402,52 @@ describe('inspector proxy React Native reloads', () => {
     }
   });
 
+  test.each([
+    ['with', true],
+    ['without', false],
+  ])(
+    "device disconnect event %s session ID closes the debugger connection when target has 'nativePageReloads' capability flag",
+    async (_, includeSessionId) => {
+      const {device, debugger_, sessionId} = await createAndConnectTarget(
+        serverRef,
+        autoCleanup.signal,
+        {
+          app: 'bar-app',
+          id: 'page1',
+          title: 'bar-title',
+          vm: 'bar-vm',
+          capabilities: {
+            nativePageReloads: true,
+          },
+        },
+      );
+      const debuggerClosed = new Promise<{code: number, reason: string}>(
+        resolve =>
+          debugger_.socket.once('close', (code: number, reason: Buffer) =>
+            resolve({code, reason: reason.toString()}),
+          ),
+      );
+
+      try {
+        device.send({
+          event: 'disconnect',
+          payload: {
+            pageId: 'page1',
+            ...(includeSessionId ? {sessionId} : {}),
+          },
+        });
+        expect(await debuggerClosed).toEqual({
+          code: 1000,
+          reason: WS_CLOSE_REASON.PAGE_REMOVED,
+        });
+        expect(debugger_.handle).not.toBeCalledWith({method: 'reload'});
+      } finally {
+        device.close();
+        debugger_.close();
+      }
+    },
+  );
+
   test("disabled when target has 'nativePageReloads' capability flag", async () => {
     let device1;
     try {

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

@@ -47,6 +47,8 @@ const WS_CLOSURE_CODE = {
 // https://github.com/facebook/react-native-devtools-frontend/blob/3d17e0fd462dc698db34586697cce2371b25e0d3/front_end/ui/legacy/components/utils/TargetDetachedDialog.ts#L50-L64
 export const WS_CLOSE_REASON = {
   PAGE_NOT_FOUND: '[PAGE_NOT_FOUND] Debugger page not found',
+  PAGE_REMOVED:
+    '[PAGE_REMOVED] The React Native instance being debugged no longer exists. Relaunch DevTools to debug a new instance.',
   CONNECTION_LOST: '[CONNECTION_LOST] Connection lost to corresponding device',
   RECREATING_DEVICE: '[RECREATING_DEVICE] Recreating device connection',
   NEW_DEBUGGER_OPENED:
@@ -606,8 +608,8 @@ export default class Device {
         }
       }
     } else if (message.event === 'disconnect') {
-      // Device sends disconnect events only when page is reloaded or
-      // if debugger socket was disconnected.
+      // Device sends disconnect events when a legacy page is reloaded, or when
+      // it ends or rejects a debugger session (e.g. the page was removed).
       const pageId = message.payload.pageId;
       const sessionId = message.payload.sessionId;
 
@@ -616,6 +618,19 @@ export default class Device {
       const page: ?Page = this.#pages.get(pageId);
 
       if (page != null && this.#pageHasCapability(page, 'nativePageReloads')) {
+        for (const [sid, debuggerConnection] of this.#debuggerConnections) {
+          if (
+            sessionId != null
+              ? sid === sessionId
+              : debuggerConnection.pageId === pageId
+          ) {
+            this.#debuggerConnections.delete(sid);
+            debuggerConnection.socket.close(
+              WS_CLOSURE_CODE.NORMAL,
+              WS_CLOSE_REASON.PAGE_REMOVED,
+            );
+          }
+        }
         return;
       }
 

packages/dev-middleware/src/inspector-proxy/__docs__/README.md

@@ -175,7 +175,12 @@ Debugger            Proxy                        Device
 **Connection Rejection:**
 
 If a device cannot accept a `connect` (e.g., page doesn't exist), it should send
-a `disconnect` back to the proxy for that `pageId`.
+a `disconnect` back to the proxy for that `pageId`. Devices also send a
+`disconnect` for each session when a page is removed.
+
+For targets with `nativePageReloads`, the proxy closes the debugger connection
+with `[PAGE_REMOVED]` when it receives a `disconnect` for its session (or for
+its page, if `sessionId` is omitted).
 
 ### Connection Semantics
 
@@ -215,6 +220,7 @@ The proxy uses specific close reasons that DevTools frontends may recognize:
 | Reason                  | Context                                 |
 | ----------------------- | --------------------------------------- |
 | `[PAGE_NOT_FOUND]`      | Debugger connected to non-existent page |
+| `[PAGE_REMOVED]`        | Device ended or rejected the session    |
 | `[CONNECTION_LOST]`     | Device disconnected                     |
 | `[RECREATING_DEVICE]`   | Device is reconnecting                  |
 | `[NEW_DEBUGGER_OPENED]` | Another debugger took over this page    |

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

@@ -96,6 +96,13 @@ export type DisconnectRequest = Readonly<{
   payload: Readonly<{pageId: string, sessionId: string}>,
 }>;
 
+// Event sent from Device to Inspector Proxy when a debugger session ends or is
+// rejected. Legacy devices omit the sessionId.
+export type DisconnectEventFromDevice = Readonly<{
+  event: 'disconnect',
+  payload: Readonly<{pageId: string, sessionId?: string}>,
+}>;
+
 // Request sent from Inspector Proxy to Device to get a list of pages.
 export type GetPagesRequest = {event: 'getPages'};
 
@@ -107,7 +114,7 @@ export type GetPagesResponse = {
 
 // Union type for all possible messages sent from device to Inspector Proxy.
 export type MessageFromDevice =
-  GetPagesResponse | WrappedEventFromDevice | DisconnectRequest;
+  GetPagesResponse | WrappedEventFromDevice | DisconnectEventFromDevice;
 
 // Union type for all possible messages sent from Inspector Proxy to device.
 export type MessageToDevice =

packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp

@@ -167,11 +167,7 @@ void InspectorPackagerConnection::Impl::handleConnect(
     // be a no op (because the session is not added to
     // `inspectorSessionsByPage_`), so let's always notify the remote client
     // of the disconnection ourselves.
-    folly::dynamic disconnectPayload =
-        folly::dynamic::object("pageId", pageId)("sessionId", proxySessionId);
-    sendToPackager(
-        folly::dynamic::object("event", "disconnect")(
-            "payload", std::move(disconnectPayload)));
+    sendDisconnectToPackager(pageId, proxySessionId);
     return;
   }
   pageIt->second.emplace(
@@ -317,13 +313,18 @@ void InspectorPackagerConnection::Impl::didClose() {
 }
 
 void InspectorPackagerConnection::Impl::onPageRemoved(int pageId) {
-  auto pageIt = inspectorSessionsByPage_.find(std::to_string(pageId));
+  auto pageIdString = std::to_string(pageId);
+  auto pageIt = inspectorSessionsByPage_.find(pageIdString);
 
   while (pageIt != inspectorSessionsByPage_.end() && !pageIt->second.empty()) {
+    auto proxySessionId = pageIt->second.begin()->first;
     pageIt = disconnectSession({
         pageIt,
         pageIt->second.begin(),
     });
+    // RemoteConnection::onDisconnect() is a no op once the session is
+    // removed, so notify the remote client ourselves.
+    sendDisconnectToPackager(pageIdString, proxySessionId);
   }
 }
 
@@ -407,6 +408,16 @@ void InspectorPackagerConnection::Impl::sendToPackager(
   webSocket_->send(folly::toJson(message));
 }
 
+void InspectorPackagerConnection::Impl::sendDisconnectToPackager(
+    const std::string& pageId,
+    const std::string& proxySessionId) {
+  sendToPackager(
+      folly::dynamic::object("event", "disconnect")(
+          "payload",
+          folly::dynamic::object("pageId", pageId)(
+              "sessionId", proxySessionId)));
+}
+
 void InspectorPackagerConnection::Impl::scheduleSendToPackager(
     folly::dynamic message,
     SessionId sourceSessionId,

packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h

@@ -106,6 +106,7 @@ class InspectorPackagerConnection::Impl : public IWebSocketDelegate,
   void closeAllConnections();
   void disposeWebSocket();
   void sendToPackager(const folly::dynamic &message);
+  void sendDisconnectToPackager(const std::string &pageId, const std::string &proxySessionId);
 
   void abort(std::optional<int> posixCode, const std::string &message, const std::string &cause);
 

packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionMultiSessionTest.cpp

@@ -111,6 +111,8 @@ TEST_F(
   mockCallsMustBeInSequence.reset();
   EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
   EXPECT_CALL(*localConnections_[1], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId, "session-1");
+  expectDisconnectSentToPackager(pageId, "session-2");
   getInspectorInstance().removePage(pageId);
 }
 
@@ -193,6 +195,8 @@ TEST_F(
   mockCallsMustBeInSequence.reset();
   EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
   EXPECT_CALL(*localConnections_[1], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId, "session-1");
+  expectDisconnectSentToPackager(pageId, "session-2");
   getInspectorInstance().removePage(pageId);
 }
 
@@ -269,6 +273,7 @@ TEST_F(InspectorPackagerConnectionMultiSessionTest, TestDisconnectBySessionId) {
 
   // Clean up
   EXPECT_CALL(*localConnections_[1], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId, "session-2");
   getInspectorInstance().removePage(pageId);
 }
 
@@ -323,6 +328,7 @@ TEST_F(
   localConnections_[0]->getRemoteConnection().onDisconnect();
 
   EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId, "my-session");
   getInspectorInstance().removePage(pageId);
 }
 
@@ -493,13 +499,17 @@ TEST_F(
           toJson(std::to_string(pageId))));
   ASSERT_TRUE(localConnections_[2]);
 
-  // Remove the page - all sessions should be disconnected.
-  // This is not guaranteed to be in order, so tear down our InSequence guard.
+  // Remove the page - all sessions should be disconnected and reported to the
+  // packager. This is not guaranteed to be in order, so tear down our
+  // InSequence guard.
   mockCallsMustBeInSequence.reset();
 
   EXPECT_CALL(*localConnections_[0], disconnect());
   EXPECT_CALL(*localConnections_[1], disconnect());
   EXPECT_CALL(*localConnections_[2], disconnect());
+  expectDisconnectSentToPackager(pageId, "session-1");
+  expectDisconnectSentToPackager(pageId, "session-2");
+  expectDisconnectSentToPackager(pageId, "session-3");
   getInspectorInstance().removePage(pageId);
 
   EXPECT_FALSE(localConnections_[0]);
@@ -592,6 +602,8 @@ TEST_F(
   mockCallsMustBeInSequence.reset();
   EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
   EXPECT_CALL(*localConnections_[1], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId);
+  expectDisconnectSentToPackager(pageId, "session-1");
   getInspectorInstance().removePage(pageId);
 }
 
@@ -686,6 +698,7 @@ TEST_F(
 
   // Clean up
   EXPECT_CALL(*localConnections_[2], disconnect()).RetiresOnSaturation();
+  expectDisconnectSentToPackager(pageId);
   getInspectorInstance().removePage(pageId);
 }