From e55ea2daf11bb527f500323e3f1bb71f10cbe1c5 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Tue, 18 Jun 2024 09:39:35 -0700 Subject: [PATCH] Fix debugger handoff logic to prevent "zombie" state (#45035) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45035 Changelog: [General][Fixed] Avoid a zombie state when opening a second debugger frontend concurrently. The problem here was that we were sending proxy-protocol messages to the device in the wrong order (`disconnect` *after* `connect`): {F1701266597} The root cause was that we were depending on the outgoing debugger socket's async `close` event to trigger sending the `disconnect` message to the device. This would happen after we'd already (synchronously) sent the `connect` message. With this diff, we send the `disconnect` message synchronously with calling `close()` on the debugger socket, which fixes the ordering problem at the source. To avoid sending duplicate `disconnect` messages (e.g. one before calling `close()` and one from the `close` event handler), we store some extra state on `Device` (`#connectedPageIds`). Reviewed By: robhogan, huntie Differential Revision: D58730634 fbshipit-source-id: 0f54af2e4f8071a8f6d97cc9e3d8a4ea89a46f43 --- .../InspectorProxyCdpTransport-test.js | 11 +-- .../src/inspector-proxy/Device.js | 92 ++++++++++--------- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyCdpTransport-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyCdpTransport-test.js index fe91a2f9394..5c3536be64a 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyCdpTransport-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyCdpTransport-test.js @@ -247,19 +247,16 @@ describe.each(['HTTP', 'HTTPS'])( event: 'create-debugger-mock', name: 'debugger2', }, - // FIXME: We currently send `connect` (for debugger2) before - // `disconnect` (for debugger1), which is wrong - it leaves the - // device thinking the connection is gone while the proxy keeps the - // debugger connection open. The user of debugger2 sees the frontend - // in a "zombie" state (not disconnected, but unresponsive). { - event: 'connect', + // NOTE: For debugger1 + event: 'disconnect', payload: { pageId: 'page1', }, }, { - event: 'disconnect', + // NOTE: For debugger2 + event: 'connect', payload: { pageId: 'page1', }, diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index fb1367dc26c..8542adcfd04 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -58,7 +58,7 @@ const REWRITE_HOSTS_TO_LOCALHOST: Array = [ // more details. const FILE_PREFIX = 'file://'; -type DebuggerInfo = { +type DebuggerConnection = { // Debugger web socket connection socket: WS, // If we replaced address (like '10.0.2.2') to localhost we need to store original @@ -67,10 +67,6 @@ type DebuggerInfo = { prependedFilePrefix: boolean, pageId: string, userAgent: string | null, -}; - -type DebuggerConnection = { - ...DebuggerInfo, customHandler: ?CustomMessageHandler, }; @@ -128,6 +124,8 @@ export default class Device { // The device message middleware factory function allowing implementers to handle unsupported CDP messages. #createCustomMessageHandler: ?CreateCustomMessageHandlerFn; + #connectedPageIds: Set = new Set(); + constructor( id: string, name: string, @@ -215,15 +213,23 @@ export default class Device { if (socket === this.#deviceSocket) { this.#deviceEventReporter?.logDisconnection('device'); // Device disconnected - close debugger connection. - if (this.#debuggerConnection) { - this.#debuggerConnection.socket.close(); - this.#debuggerConnection = null; - } + this.#terminateDebuggerConnection(); clearInterval(this.#pagesPollingIntervalId); } }); } + #terminateDebuggerConnection() { + const debuggerConnection = this.#debuggerConnection; + if (debuggerConnection) { + this.#sendDisconnectEventToDevice( + this.#mapToDevicePageId(debuggerConnection.pageId), + ); + debuggerConnection.socket.close(); + this.#debuggerConnection = null; + } + } + /** * Used to recreate the device connection if there is a device ID collision. * 1. Checks if the same device is attempting to reconnect for the same app. @@ -246,12 +252,14 @@ export default class Device { id === this.#id, 'dangerouslyRecreateDevice() can only be used for the same device ID', ); - if (this.#app !== app || this.#name !== name) { - this.#deviceSocket.close(); - this.#debuggerConnection?.socket.close(); - } const oldDebugger = this.#debuggerConnection; + + if (this.#app !== app || this.#name !== name) { + this.#deviceSocket.close(); + this.#terminateDebuggerConnection(); + } + this.#debuggerConnection = null; if (oldDebugger) { @@ -309,10 +317,7 @@ export default class Device { }); // Disconnect current debugger if we already have debugger connected. - if (this.#debuggerConnection) { - this.#debuggerConnection.socket.close(); - this.#debuggerConnection = null; - } + this.#terminateDebuggerConnection(); const debuggerInfo = { socket, @@ -377,12 +382,7 @@ export default class Device { } } - this.#sendMessageToDevice({ - event: 'connect', - payload: { - pageId: this.#mapToDevicePageId(pageId), - }, - }); + this.#sendConnectEventToDevice(this.#mapToDevicePageId(pageId)); // $FlowFixMe[incompatible-call] socket.on('message', (message: string) => { @@ -426,13 +426,9 @@ export default class Device { socket.on('close', () => { debug(`Debugger for page ${pageId} and ${this.#name} disconnected.`); this.#deviceEventReporter?.logDisconnection('debugger'); - this.#sendMessageToDevice({ - event: 'disconnect', - payload: { - pageId: this.#mapToDevicePageId(pageId), - }, - }); - this.#debuggerConnection = null; + if (this.#debuggerConnection?.socket === socket) { + this.#terminateDebuggerConnection(); + } }); // $FlowFixMe[method-unbinding] @@ -444,6 +440,28 @@ export default class Device { }; } + #sendConnectEventToDevice(devicePageId: string) { + if (this.#connectedPageIds.has(devicePageId)) { + return; + } + this.#connectedPageIds.add(devicePageId); + this.#sendMessageToDevice({ + event: 'connect', + payload: {pageId: devicePageId}, + }); + } + + #sendDisconnectEventToDevice(devicePageId: string) { + if (!this.#connectedPageIds.has(devicePageId)) { + return; + } + this.#connectedPageIds.delete(devicePageId); + this.#sendMessageToDevice({ + event: 'disconnect', + payload: {pageId: devicePageId}, + }); + } + /** * Returns `true` if a page supports the given target capability flag. */ @@ -621,20 +639,10 @@ export default class Device { // page. if (oldPageId != null) { - this.#sendMessageToDevice({ - event: 'disconnect', - payload: { - pageId: oldPageId, - }, - }); + this.#sendDisconnectEventToDevice(oldPageId); } - this.#sendMessageToDevice({ - event: 'connect', - payload: { - pageId: page.id, - }, - }); + this.#sendConnectEventToDevice(page.id); const toSend = [ {method: 'Runtime.enable', id: 1e9},