diff --git a/src/main/browser/cdp-debugger-channel.ts b/src/main/browser/cdp-debugger-channel.ts index 3124a4f9851..9bb4d1f27c4 100644 --- a/src/main/browser/cdp-debugger-channel.ts +++ b/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( diff --git a/src/main/browser/cdp-ws-proxy-startup-lifetime.test.ts b/src/main/browser/cdp-ws-proxy-startup-lifetime.test.ts new file mode 100644 index 00000000000..07479bfa45b --- /dev/null +++ b/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) { + let finish = (): void => {} + let fail = (_error: Error): void => {} + const command = new Promise>((resolve, reject) => { + finish = () => resolve({}) + fail = reject + }) + mock.webContents.debugger.sendCommand.mockReturnValueOnce(command) + return { finish, fail } +} + +function invalidateNativeHandle(mock: ReturnType): 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): CdpWsProxy { + const proxy = new CdpWsProxy(mockAsWebContents(mock), () => () => {}) + proxies.push(proxy) + return proxy + } + + function expectNoListeners(mock: ReturnType): 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) + }) +}) diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index 6da8270b528..b18c0e88744 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/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 { + 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((resolve, reject) => { this.httpServer = createServer((req, res) => { if (!isTrustedCdpLoopbackRequest(req, this.port)) { @@ -145,15 +153,19 @@ export class CdpWsProxy { } async stop(): Promise { - 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 + } } } diff --git a/src/main/browser/electron-debugger-lease.test.ts b/src/main/browser/electron-debugger-lease.test.ts index ff6fdd22cf8..22d4c399d2e 100644 --- a/src/main/browser/electron-debugger-lease.test.ts +++ b/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() + }) }) diff --git a/src/main/browser/electron-debugger-lease.ts b/src/main/browser/electron-debugger-lease.ts index 11400e95b29..f3c33f5da18 100644 --- a/src/main/browser/electron-debugger-lease.ts +++ b/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 {