Fix debugger handoff logic to prevent "zombie" state (facebook#45035)

Summary:
Pull Request resolved: facebook#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

Author: motiz88

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',
             },

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

@@ -58,7 +58,7 @@ const REWRITE_HOSTS_TO_LOCALHOST: Array<string> = [
 // 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<string> = 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',
     );
+
+    const oldDebugger = this.#debuggerConnection;
+
     if (this.#app !== app || this.#name !== name) {
       this.#deviceSocket.close();
-      this.#debuggerConnection?.socket.close();
+      this.#terminateDebuggerConnection();
     }
 
-    const oldDebugger = this.#debuggerConnection;
     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},