fix(browser): retire CDP startup with its debugger owner (#35093)

Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>

src/main/browser/cdp-debugger-channel.ts

@@ -7,6 +7,7 @@ import { sendGuestCdpCommand } from './guest-cdp-command'
 import { isOrcaFrameSessionTraffic } from './guest-focused-frame-route'
 
 export class CdpDebuggerChannel {
+  private nativeDebugger: WebContents['debugger'] | null = null
   private debuggerMessageHandler: ((...args: unknown[]) => void) | null = null
   private debuggerDetachHandler: ((...args: unknown[]) => void) | null = null
   private debuggerLease: ElectronDebuggerLease | null = null
@@ -24,19 +25,17 @@ export class CdpDebuggerChannel {
     if (this.attached) {
       return
     }
+    let lease: ElectronDebuggerLease
     try {
-      this.debuggerLease = acquireElectronDebugger(this.webContents)
+      lease = acquireElectronDebugger(this.webContents)
     } catch {
       throw new Error('Could not attach debugger. DevTools may already be open for this tab.')
     }
+    const dbg = this.webContents.debugger
+    this.nativeDebugger = dbg
+    this.debuggerLease = lease
     this.attached = true
 
-    try {
-      await this.webContents.debugger.sendCommand('Page.enable', {})
-    } catch {
-      /* The page domain may not be ready yet. */
-    }
-
     this.debuggerMessageHandler = (_event: unknown, ...rest: unknown[]) => {
       const [method, params, sessionId] = rest as [
         string,
@@ -57,29 +56,46 @@ export class CdpDebuggerChannel {
       this.responder.send(msg, client)
     }
     this.debuggerDetachHandler = () => {
-      this.attached = false
-      const lease = this.debuggerLease
-      this.debuggerLease = null
-      lease?.release()
+      this.detachDebugger()
       this.onDetached()
     }
-    this.webContents.debugger.on('message', this.debuggerMessageHandler as never)
-    this.webContents.debugger.on('detach', this.debuggerDetachHandler as never)
+    dbg.on('message', this.debuggerMessageHandler)
+    dbg.on('detach', this.debuggerDetachHandler)
+
+    try {
+      await dbg.sendCommand('Page.enable', {})
+    } catch {
+      /* The page domain may not be ready yet. */
+    }
+    if (this.debuggerLease !== lease) {
+      throw new Error('Browser debugger was detached during startup')
+    }
+    if (this.webContents.isDestroyed() || !dbg.isAttached()) {
+      this.detachDebugger()
+      throw new Error('Browser tab is no longer available')
+    }
   }
 
   detachDebugger(): void {
-    if (this.debuggerMessageHandler) {
-      this.webContents.debugger.removeListener('message', this.debuggerMessageHandler as never)
-      this.debuggerMessageHandler = null
-    }
-    if (this.debuggerDetachHandler) {
-      this.webContents.debugger.removeListener('detach', this.debuggerDetachHandler as never)
-      this.debuggerDetachHandler = null
-    }
+    const dbg = this.nativeDebugger
+    const messageHandler = this.debuggerMessageHandler
+    const detachHandler = this.debuggerDetachHandler
     const lease = this.debuggerLease
+    this.nativeDebugger = null
+    this.debuggerMessageHandler = null
+    this.debuggerDetachHandler = null
     this.debuggerLease = null
-    lease?.release()
     this.attached = false
+    try {
+      if (messageHandler) {
+        dbg?.removeListener('message', messageHandler)
+      }
+      if (detachHandler) {
+        dbg?.removeListener('detach', detachHandler)
+      }
+    } finally {
+      lease?.release()
+    }
   }
 
   sendDebuggerCommand(

src/main/browser/cdp-ws-proxy-startup-lifetime.test.ts

@@ -0,0 +1,216 @@
+import { EventEmitter } from 'node:events'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { CdpWsProxy } from './cdp-ws-proxy'
+import { acquireElectronDebugger } from './electron-debugger-lease'
+import { createMockWebContents, mockAsWebContents } from './cdp-ws-proxy-test-harness'
+
+const { createServerMock, webSocketServerMock } = vi.hoisted(() => ({
+  createServerMock: vi.fn(),
+  webSocketServerMock: vi.fn()
+}))
+
+vi.mock('electron', () => ({ webContents: { fromId: vi.fn() } }))
+vi.mock('node:http', () => ({ createServer: createServerMock }))
+vi.mock('ws', () => ({
+  default: { OPEN: 1 },
+  WebSocket: { OPEN: 1 },
+  WebSocketServer: webSocketServerMock
+}))
+
+class ProxyServer extends EventEmitter {
+  listening = false
+
+  listen(_port: number, _host: string, callback: () => void): void {
+    this.listening = true
+    queueMicrotask(callback)
+  }
+
+  address(): { port: number } {
+    return { port: 9222 }
+  }
+
+  close(): void {
+    this.listening = false
+  }
+}
+
+class ProxyWebSocketServer extends EventEmitter {
+  closed = false
+
+  close(): void {
+    this.closed = true
+  }
+}
+
+function pendingEnable(mock: ReturnType<typeof createMockWebContents>) {
+  let finish = (): void => {}
+  let fail = (_error: Error): void => {}
+  const command = new Promise<Record<string, unknown>>((resolve, reject) => {
+    finish = () => resolve({})
+    fail = reject
+  })
+  mock.webContents.debugger.sendCommand.mockReturnValueOnce(command)
+  return { finish, fail }
+}
+
+function invalidateNativeHandle(mock: ReturnType<typeof createMockWebContents>): void {
+  mock.destroy()
+  Object.defineProperty(mock.webContents, 'debugger', {
+    get() {
+      throw new Error('Object has been destroyed')
+    }
+  })
+}
+
+describe('CdpWsProxy startup ownership', () => {
+  let servers: ProxyServer[]
+  let socketServers: ProxyWebSocketServer[]
+  let proxies: CdpWsProxy[]
+
+  beforeEach(() => {
+    servers = []
+    socketServers = []
+    proxies = []
+    createServerMock.mockImplementation(() => {
+      const server = new ProxyServer()
+      servers.push(server)
+      return server
+    })
+    webSocketServerMock.mockImplementation(function () {
+      const server = new ProxyWebSocketServer()
+      socketServers.push(server)
+      return server
+    })
+  })
+
+  afterEach(async () => {
+    await Promise.all(proxies.map((proxy) => proxy.stop()))
+    expect(servers.filter((server) => server.listening)).toHaveLength(0)
+    expect(socketServers.filter((server) => !server.closed)).toHaveLength(0)
+  })
+
+  function makeProxy(mock: ReturnType<typeof createMockWebContents>): CdpWsProxy {
+    const proxy = new CdpWsProxy(mockAsWebContents(mock), () => () => {})
+    proxies.push(proxy)
+    return proxy
+  }
+
+  function expectNoListeners(mock: ReturnType<typeof createMockWebContents>): void {
+    expect(mock.listeners.get('message') ?? []).toHaveLength(0)
+    expect(mock.listeners.get('detach') ?? []).toHaveLength(0)
+  }
+
+  it('tolerates Page.enable rejection on a live shared attachment', async () => {
+    const mock = createMockWebContents()
+    const debuggerHandle = mock.webContents.debugger
+    const lease = acquireElectronDebugger(mockAsWebContents(mock))
+    debuggerHandle.sendCommand.mockRejectedValueOnce(new Error('Page domain is not ready'))
+    const proxy = makeProxy(mock)
+
+    await expect(proxy.start()).resolves.toBe('ws://127.0.0.1:9222')
+    await proxy.stop()
+
+    expectNoListeners(mock)
+    expect(debuggerHandle.isAttached()).toBe(true)
+    expect(debuggerHandle.detach).not.toHaveBeenCalled()
+    lease.release()
+    expect(debuggerHandle.attach).toHaveBeenCalledTimes(1)
+    expect(debuggerHandle.detach).toHaveBeenCalledTimes(1)
+  })
+
+  it.each([false, true])(
+    'refuses server admission after native detach during Page.enable (destroyed: %s)',
+    async (destroyed) => {
+      const mock = createMockWebContents()
+      const debuggerHandle = mock.webContents.debugger
+      const enable = pendingEnable(mock)
+      const proxy = makeProxy(mock)
+      const starting = proxy.start()
+      debuggerHandle.isAttached.mockReturnValue(false)
+      if (destroyed) {
+        invalidateNativeHandle(mock)
+      }
+      mock.emit('detach', {}, 'target closed')
+      enable.fail(new Error('target closed while handling command'))
+
+      await expect(starting).rejects.toThrow('detached during startup')
+      expect(proxy.getPort()).toBe(0)
+      expect(servers).toHaveLength(0)
+      expect(socketServers).toHaveLength(0)
+      expectNoListeners(mock)
+    }
+  )
+
+  it('cleans a destroyed startup even when no native detach event arrives', async () => {
+    const mock = createMockWebContents()
+    const enable = pendingEnable(mock)
+    const proxy = makeProxy(mock)
+    const starting = proxy.start()
+    invalidateNativeHandle(mock)
+    enable.fail(new Error('target closed while handling command'))
+
+    await expect(starting).rejects.toThrow('no longer available')
+    expect(servers).toHaveLength(0)
+    expectNoListeners(mock)
+  })
+
+  it('refuses server admission when stopped after attachment completes', async () => {
+    const mock = createMockWebContents()
+    const proxy = makeProxy(mock)
+    const starting = proxy.start()
+    await Promise.resolve()
+    await proxy.stop()
+
+    await expect(starting).rejects.toThrow('stopped')
+    expect(servers).toHaveLength(0)
+    expectNoListeners(mock)
+  })
+
+  it('does not detach a successor when stopped Page.enable completes late', async () => {
+    const mock = createMockWebContents()
+    const debuggerHandle = mock.webContents.debugger
+    const enable = pendingEnable(mock)
+    const proxy = makeProxy(mock)
+    const starting = proxy.start()
+    await proxy.stop()
+    const successor = makeProxy(mock)
+    await successor.start()
+    enable.finish()
+
+    await expect(starting).rejects.toThrow('detached during startup')
+    expect(servers).toHaveLength(1)
+    expect(debuggerHandle.isAttached()).toBe(true)
+    expect(debuggerHandle.detach).toHaveBeenCalledTimes(1)
+    expect(mock.listeners.get('message')).toHaveLength(1)
+    expect(mock.listeners.get('detach')).toHaveLength(1)
+    await successor.stop()
+    expect(debuggerHandle.detach).toHaveBeenCalledTimes(2)
+    expectNoListeners(mock)
+  })
+
+  it('retires an established proxy without reading a destroyed native handle', async () => {
+    const mock = createMockWebContents()
+    const proxy = makeProxy(mock)
+    await proxy.start()
+    invalidateNativeHandle(mock)
+
+    await expect(proxy.stop()).resolves.toBeUndefined()
+    expectNoListeners(mock)
+    expect(servers[0].listening).toBe(false)
+    expect(socketServers[0].closed).toBe(true)
+  })
+
+  it('closes server resources even if debugger cleanup throws', async () => {
+    const mock = createMockWebContents()
+    const proxy = makeProxy(mock)
+    await proxy.start()
+    mock.webContents.debugger.removeListener.mockImplementationOnce(() => {
+      throw new Error('debugger cleanup failed')
+    })
+
+    await expect(proxy.stop()).rejects.toThrow('debugger cleanup failed')
+    expect(servers[0].listening).toBe(false)
+    expect(socketServers[0].closed).toBe(true)
+    expect(mock.webContents.debugger.detach).toHaveBeenCalledTimes(1)
+  })
+})

src/main/browser/cdp-ws-proxy.ts

@@ -23,6 +23,7 @@ export class CdpWsProxy {
   private client: WebSocket | null = null
   private detachClientListeners: (() => void) | null = null
   private port = 0
+  private stopped = false
   private readonly responder = new CdpClientResponseWriter(() => this.client)
   private readonly sessions = new CdpSyntheticSessionRegistry()
   private readonly discovery: CdpTargetDiscovery
@@ -77,7 +78,14 @@ export class CdpWsProxy {
   }
 
   async start(): Promise<string> {
+    if (this.stopped) {
+      throw new Error('CDP proxy has been stopped')
+    }
     await this.debuggerChannel.attachDebugger()
+    if (this.stopped || this.webContents.isDestroyed()) {
+      await this.stop()
+      throw new Error('CDP proxy has been stopped')
+    }
     return new Promise<string>((resolve, reject) => {
       this.httpServer = createServer((req, res) => {
         if (!isTrustedCdpLoopbackRequest(req, this.port)) {
@@ -145,15 +153,19 @@ export class CdpWsProxy {
   }
 
   async stop(): Promise<void> {
-    this.debuggerChannel.detachDebugger()
-    this.closeClient()
-    if (this.wss) {
-      this.wss.close()
-      this.wss = null
-    }
-    if (this.httpServer) {
-      this.httpServer.close()
-      this.httpServer = null
+    this.stopped = true
+    try {
+      this.debuggerChannel.detachDebugger()
+    } finally {
+      this.closeClient()
+      if (this.wss) {
+        this.wss.close()
+        this.wss = null
+      }
+      if (this.httpServer) {
+        this.httpServer.close()
+        this.httpServer = null
+      }
     }
   }
 

src/main/browser/electron-debugger-lease.test.ts

@@ -41,4 +41,18 @@ describe('electron debugger lease', () => {
     expect(webContents.debugger.attach).not.toHaveBeenCalled()
     expect(webContents.debugger.detach).not.toHaveBeenCalled()
   })
+
+  it('releases a destroyed guest without querying its native debugger', () => {
+    const webContents = createWebContents()
+    // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: this fixture supplies the debugger lease's entire WebContents boundary.
+    const lease = acquireElectronDebugger(webContents as never)
+    webContents.isDestroyed.mockReturnValue(true)
+    webContents.debugger.isAttached.mockImplementation(() => {
+      throw new Error('Object has been destroyed')
+    })
+
+    expect(() => lease.release()).not.toThrow()
+    expect(() => lease.release()).not.toThrow()
+    expect(webContents.debugger.detach).not.toHaveBeenCalled()
+  })
 })

src/main/browser/electron-debugger-lease.ts

@@ -43,7 +43,7 @@ export function acquireElectronDebugger(webContents: WebContents): ElectronDebug
         return
       }
       debuggerLeases.delete(webContents)
-      if (!state.attachedByLease || !dbg.isAttached()) {
+      if (!state.attachedByLease || webContents.isDestroyed() || !dbg.isAttached()) {
         return
       }
       try {