diff --git a/scripts/test-closed-window.js b/scripts/test-closed-window.js new file mode 100644 index 0000000..9126c64 --- /dev/null +++ b/scripts/test-closed-window.js @@ -0,0 +1,216 @@ +#!/usr/bin/env node +/** + * Test: zombie geckodriver cleanup + * + * Scenario B (SIGKILL): Firefox is completely dead (user clicked [X]). + * Recovery test: can close() clean up and reconnect after Firefox dies? + * + * Scenario C (SIGKILL, non-headless): Same as B with a visible browser window. + */ +import { FirefoxDevTools } from '../dist/index.js'; +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function parsePids(output) { + return output + .trim() + .split('\n') + .map(Number) + .filter((n) => !isNaN(n)); +} + +function pgrep(pattern) { + try { + return parsePids(execFileSync('pgrep', ['-f', pattern], { encoding: 'utf-8' })); + } catch { + return []; + } +} + +function getProcState(pid) { + try { + const stat = readFileSync(`/proc/${pid}/stat`, 'utf-8'); + return stat[stat.lastIndexOf(')') + 2]; + } catch { + return null; + } +} + +function isAlive(pid) { + const state = getProcState(pid); + return state !== null && state !== 'Z'; +} + +function killHard(pid) { + try { + process.kill(pid, 'SIGKILL'); + } catch {} +} + +function getDescendants(parentPid) { + const result = []; + const queue = [parentPid]; + while (queue.length > 0) { + const pid = queue.shift(); + try { + const children = parsePids(execFileSync('pgrep', ['-P', String(pid)], { encoding: 'utf-8' })); + result.push(...children); + queue.push(...children); + } catch {} + } + return result; +} + +function waitForDeath(pid, timeoutMs) { + return new Promise((resolve) => { + const start = Date.now(); + const check = () => { + if (!isAlive(pid)) resolve(true); + else if (Date.now() - start > timeoutMs) resolve(false); + else setTimeout(check, 100); + }; + check(); + }); +} + +function killAll(pids) { + for (const pid of pids) killHard(pid); +} + +async function reconnect(geckosBefore, excludePids) { + const r = await launchFirefox(geckosBefore, excludePids); + await r.devTools.navigate('about:blank'); + console.log(' Navigation works'); + await r.devTools.close(); + return r.geckoPid; +} + +// Log unhandled rejections instead of swallowing them silently +process.on('unhandledRejection', (reason) => { + console.error('[unhandled rejection]', reason); +}); + +// --------------------------------------------------------------------------- +// Launch helper +// --------------------------------------------------------------------------- + +async function launchFirefox(geckosBefore, excludePids = [], headless = true) { + const devTools = new FirefoxDevTools({ + headless, + viewport: { width: 1280, height: 720 }, + }); + await devTools.connect(); + + const geckoPid = pgrep('geckodriver').find( + (p) => !geckosBefore.has(p) && !excludePids.includes(p) + ); + if (!geckoPid) throw new Error('No geckodriver PID found after connect'); + + const firefoxPids = getDescendants(geckoPid); + if (firefoxPids.length === 0) { + killHard(geckoPid); + throw new Error('No Firefox PIDs found under geckodriver'); + } + + return { devTools, geckoPid, firefoxPids }; +} + +// --------------------------------------------------------------------------- +// Test +// --------------------------------------------------------------------------- + +async function main() { + console.log('--- Zombie geckodriver fix test ---\n'); + const geckosBefore = new Set(pgrep('geckodriver')); + const usedPids = []; + + console.log('Scenario B: Firefox killed (SIGKILL)'); + + console.log(' 1. Launching Firefox...'); + const b = await launchFirefox(geckosBefore, usedPids); + console.log(` Geckodriver PID: ${b.geckoPid}, Firefox PIDs: ${b.firefoxPids.join(', ')}`); + + console.log(' 2. Killing Firefox...'); + killAll(b.firefoxPids); + for (const pid of b.firefoxPids) { + if (!(await waitForDeath(pid, 5000))) { + console.error(` [FATAL] Firefox PID ${pid} survived SIGKILL`); + killHard(b.geckoPid); + process.exit(1); + } + } + console.log(' Firefox is dead'); + + console.log( + ` 3. ${isAlive(b.geckoPid) ? 'Zombie geckodriver detected' : 'Geckodriver died with Firefox (no zombie)'}` + ); + + console.log(' 4. Running cleanup (close() handles timeout + force-kill)...'); + await b.devTools.close(); + + console.log(' 5. Verifying geckodriver is dead...'); + if (!(await waitForDeath(b.geckoPid, 5000))) { + console.error(' [FAIL] Geckodriver still alive after cleanup'); + killHard(b.geckoPid); + process.exit(1); + } + console.log(' Geckodriver is dead'); + + console.log(' 6. Reconnecting...'); + usedPids.push(b.geckoPid); + usedPids.push(await reconnect(geckosBefore, usedPids)); + console.log(' Scenario B: PASS\n'); + + console.log('Scenario C: Firefox killed (SIGKILL) — non-headless'); + + console.log(' 1. Launching Firefox (non-headless)...'); + const c = await launchFirefox(geckosBefore, usedPids, false); + console.log(` Geckodriver PID: ${c.geckoPid}, Firefox PIDs: ${c.firefoxPids.join(', ')}`); + + console.log(' 2. Killing Firefox...'); + killAll(c.firefoxPids); + for (const pid of c.firefoxPids) { + if (!(await waitForDeath(pid, 5000))) { + console.error(` [FATAL] Firefox PID ${pid} survived SIGKILL`); + killHard(c.geckoPid); + process.exit(1); + } + } + console.log(' Firefox is dead'); + + console.log( + ` 3. ${isAlive(c.geckoPid) ? 'Zombie geckodriver detected' : 'Geckodriver died with Firefox (no zombie)'}` + ); + + console.log(' 4. Running cleanup (close() handles timeout + force-kill)...'); + await c.devTools.close(); + + console.log(' 5. Verifying geckodriver is dead...'); + if (!(await waitForDeath(c.geckoPid, 5000))) { + console.error(' [FAIL] Geckodriver still alive after cleanup'); + killHard(c.geckoPid); + process.exit(1); + } + console.log(' Geckodriver is dead'); + + console.log(' 6. Reconnecting...'); + usedPids.push(c.geckoPid); + usedPids.push(await reconnect(geckosBefore, usedPids)); + console.log(' Scenario C: PASS\n'); + + // Final cleanup + const leftover = pgrep('geckodriver').filter((p) => !geckosBefore.has(p)); + for (const pid of leftover) { + killAll(getDescendants(pid)); + killHard(pid); + } +} + +main().catch((err) => { + console.error('Fatal error:', err); + process.exit(1); +}); diff --git a/src/firefox/core.ts b/src/firefox/core.ts index eec387d..2e051a4 100644 --- a/src/firefox/core.ts +++ b/src/firefox/core.ts @@ -244,25 +244,6 @@ export class FirefoxCore { } } - /** - * Reset driver state (used when Firefox is detected as closed) - */ - reset(): void { - if (this.driver) { - const d = this.driver as any; - if (d._bidiConnection) { - d._bidiConnection.close(); - d._bidiConnection = undefined; - } - if ('quit' in this.driver) { - void (this.driver as { quit(): Promise }).quit(); - } - } - this.driver = null; - this.currentContextId = null; - logDebug('Driver state reset'); - } - /** * Get current browsing context ID */ @@ -384,27 +365,58 @@ export class FirefoxCore { /** * Close driver and cleanup. - * When connected to an existing Firefox instance, only kills geckodriver - * without closing the browser. + * - Tries graceful quit() with a timeout; on timeout, force-kills via onQuit_(). + * - Restores env vars, closes log fd, clears all state. + * - Never throws — callers can rely on cleanup completing. */ async close(): Promise { - if (this.driver) { - // Selenium's quit() skips closing the BiDi WebSocket when onQuit_ is set - // (it returns early before reaching the _bidiConnection.close() branch). - // We must close it first: geckodriver may not release the Marionette session - // until the BiDi connection is cleanly terminated, which would leave Firefox's - // Marionette locked and prevent reconnection. - const d = this.driver as any; - if (d._bidiConnection) { - d._bidiConnection.close(); - d._bidiConnection = undefined; + if (!this.driver) { + return; + } + + const webdriver = this.driver as any; // Selenium WebDriver + const webdriverQuitTimeout = 5000; + + // Null to prevent re-entrancy + this.driver = null; + this.currentContextId = null; + this.logFilePath = undefined; + this.profileWarning = null; + + // Selenium's quit() skips closing the BiDi WebSocket when onQuit_ is set. + // We must close it first: geckodriver may not release the Marionette session + // until the BiDi connection is cleanly terminated. + if (webdriver._bidiConnection) { + try { + webdriver._bidiConnection.close(); + } catch { + /* already dead */ + } finally { + webdriver._bidiConnection = undefined; } - // In connect-existing mode, geckodriver's DELETE /session releases Marionette - // without terminating Firefox (since geckodriver was started with --connect-existing). - if ('quit' in this.driver) { - await (this.driver as { quit(): Promise }).quit(); + } + + // In connect-existing mode, geckodriver's DELETE /session releases Marionette + // without terminating Firefox (since geckodriver was started with --connect-existing). + if ('quit' in webdriver) { + let timer: NodeJS.Timeout; + try { + // Give webdriver.quit() a certain timeout + await Promise.race([ + (webdriver as { quit(): Promise }).quit(), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('close timeout')), webdriverQuitTimeout); + }), + ]); + } catch { + const webdriverHasOnQuit = typeof webdriver.onQuit_ === 'function'; + logDebug('WebDriver.quit() timed out or failed - force killing geckodriver'); + if (webdriverHasOnQuit) { + void webdriver.onQuit_().catch(() => {}); + } + } finally { + clearTimeout(timer!); } - this.driver = null; } // Close log file descriptor if open diff --git a/src/firefox/index.ts b/src/firefox/index.ts index a195e04..9ec51f2 100644 --- a/src/firefox/index.ts +++ b/src/firefox/index.ts @@ -465,24 +465,21 @@ export class FirefoxClient { return this.core.getOptions(); } - /** - * Reset all internal state (used when Firefox is detected as closed) - */ - reset(): void { - this.core.reset(); - this.consoleEvents = null; - this.networkEvents = null; - this.dom = null; - this.pages = null; - this.snapshot = null; - } - // ============================================================================ // Cleanup // ============================================================================ async close(): Promise { - await this.core.close(); + try { + await this.core.close(); + } catch (error) { + logDebug(`close() failed: ${error instanceof Error ? error.message : String(error)}`); + } + this.consoleEvents = null; + this.networkEvents = null; + this.dom = null; + this.pages = null; + this.snapshot = null; } } diff --git a/src/index.ts b/src/index.ts index 66a3363..ff73ac5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -41,11 +41,13 @@ let nextLaunchOptions: FirefoxLaunchOptions | null = null; let pendingWarning: string | null = null; /** - * Reset Firefox instance (used when disconnection is detected) + * Reset Firefox instance (used when disconnection is detected). + * Tries graceful close with a timeout; if it hangs (zombie geckodriver), + * force-kills the process tree. */ -export function resetFirefox(): void { +export async function resetFirefox(): Promise { if (firefox) { - firefox.reset(); + await firefox.close(); firefox = null; } pendingWarning = null; @@ -81,7 +83,7 @@ export async function getFirefox(): Promise { const isConnected = await firefox.isConnected(); if (!isConnected) { log('Firefox connection lost, reconnecting...'); - resetFirefox(); + await resetFirefox(); } else { return firefox; } @@ -143,7 +145,7 @@ export async function getFirefox(): Promise { // leave geckodriver running with an active Marionette session, causing // "Connection attempt denied because an active session has been found" // on the next connect attempt. - await firefox.close().catch(() => {}); + await firefox.close(); firefox = null; throw error; } @@ -385,13 +387,7 @@ export async function run( // Clean up the Marionette session so Firefox accepts new connections. // Without this, the session stays locked after the MCP client disconnects. const cleanup = async () => { - if (firefox) { - try { - await firefox.close(); - } catch { - // ignore - } - } + await resetFirefox(); await server.close(); process.exit(0); }; diff --git a/src/tools/firefox-management.ts b/src/tools/firefox-management.ts index 1811a51..5b4fb6f 100644 --- a/src/tools/firefox-management.ts +++ b/src/tools/firefox-management.ts @@ -289,13 +289,8 @@ export async function handleRestartFirefox(input: unknown) { // Set options for next launch setNextLaunchOptions(newOptions); - // Close current instance (ignore errors - we're restarting anyway) - try { - await currentFirefox.close(); - } catch { - // Ignore close errors - we'll reset anyway - } - resetFirefox(); + // Close current instance + await resetFirefox(); // Prepare change summary const changes = []; @@ -331,7 +326,7 @@ export async function handleRestartFirefox(input: unknown) { // Firefox not running (or disconnected) - configure for first start if (currentFirefox) { // Had a stale disconnected reference, clean it up - resetFirefox(); + await resetFirefox(); } // Use provided firefoxPath, or fall back to CLI args if available diff --git a/tests/firefox/connect-existing.test.ts b/tests/firefox/connect-existing.test.ts index 664360a..7c5a4bc 100644 --- a/tests/firefox/connect-existing.test.ts +++ b/tests/firefox/connect-existing.test.ts @@ -35,9 +35,10 @@ describe('getFirefox() reconnect behavior', () => { marionettePort: 2828, }); - // Verify reset clears the state + // Verify close() clears the state + (core as any).driver = { quit: vi.fn().mockResolvedValue(undefined) }; core.setCurrentContextId('old-context'); - core.reset(); + await core.close(); expect(core.getCurrentContextId()).toBe(null); expect(() => core.getDriver()).toThrow('Driver not connected'); }); diff --git a/tests/firefox/core.test.ts b/tests/firefox/core.test.ts index a6c067d..cb00a99 100644 --- a/tests/firefox/core.test.ts +++ b/tests/firefox/core.test.ts @@ -62,16 +62,152 @@ describe('FirefoxCore', () => { }); }); - describe('reset', () => { - it('should reset driver and context to null', () => { + describe('close', () => { + it('should call quit() and null driver when quit() succeeds', async () => { + const quit = vi.fn().mockResolvedValue(undefined); + const onQuit = vi.fn().mockResolvedValue(undefined); const core = new FirefoxCore({ headless: true }); - core.setCurrentContextId('test-context'); + (core as any).driver = { quit, onQuit_: onQuit }; + core.setCurrentContextId('ctx-1'); - core.reset(); + await core.close(); + expect(quit).toHaveBeenCalledTimes(1); + expect(onQuit).not.toHaveBeenCalled(); expect(core.getCurrentContextId()).toBe(null); expect(() => core.getDriver()).toThrow('Driver not connected'); }); + + it('should call onQuit_() when quit() times out', async () => { + vi.useFakeTimers(); + try { + const onQuit = vi.fn().mockResolvedValue(undefined); + const core = new FirefoxCore({ headless: true }); + (core as any).driver = { + quit: vi.fn().mockReturnValue(new Promise(() => {})), + onQuit_: onQuit, + }; + core.setCurrentContextId('ctx-1'); + + const closePromise = core.close(); + + await vi.advanceTimersByTimeAsync(5500); + await closePromise; + + expect(onQuit).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it('should call onQuit_() when quit() rejects', async () => { + const onQuit = vi.fn().mockResolvedValue(undefined); + const core = new FirefoxCore({ headless: true }); + (core as any).driver = { + quit: vi.fn().mockRejectedValue(new Error('session not found')), + onQuit_: onQuit, + }; + core.setCurrentContextId('ctx-1'); + + await core.close(); + + expect(onQuit).toHaveBeenCalled(); + }); + + it('should be idempotent with driver — second call is a no-op', async () => { + const quit = vi.fn().mockResolvedValue(undefined); + const onQuit = vi.fn().mockResolvedValue(undefined); + const core = new FirefoxCore({ headless: true }); + (core as any).driver = { quit, onQuit_: onQuit }; + core.setCurrentContextId('ctx-1'); + + await core.close(); + await core.close(); + + expect(quit).toHaveBeenCalledTimes(1); + expect(onQuit).not.toHaveBeenCalled(); + }); + + it('should be idempotent without driver — returns without error', async () => { + const core = new FirefoxCore({ headless: true }); + await core.close(); + await expect(core.close()).resolves.toBeUndefined(); + }); + + it('should close BiDi WebSocket and clear the reference', async () => { + const bidiClose = vi.fn(); + const webdriver = { + quit: vi.fn().mockResolvedValue(undefined), + _bidiConnection: { close: bidiClose }, + }; + const core = new FirefoxCore({ headless: true }); + (core as any).driver = webdriver; + + await core.close(); + + expect(bidiClose).toHaveBeenCalled(); + expect(webdriver._bidiConnection).toBeUndefined(); + }); + + it('should swallow BiDi close errors and continue cleanup', async () => { + const webdriver = { + quit: vi.fn().mockResolvedValue(undefined), + _bidiConnection: { + close: vi.fn().mockImplementation(() => { + throw new Error('ws dead'); + }), + }, + }; + const core = new FirefoxCore({ headless: true }); + (core as any).driver = webdriver; + core.setCurrentContextId('ctx-1'); + + await core.close(); + + expect(webdriver._bidiConnection).toBeUndefined(); + }); + + it('should restore env vars and clear state fields', async () => { + const savedNewKey = process.env.FIREFOX_MCP_TEST_NEWKEY; + const savedExisting = process.env.FIREFOX_MCP_TEST_EXISTING; + try { + const core = new FirefoxCore({ headless: true }); + (core as any).driver = { + quit: vi.fn().mockResolvedValue(undefined), + }; + core.setCurrentContextId('ctx-1'); + (core as any).logFilePath = '/tmp/test.log'; + (core as any).profileWarning = 'test warning'; + (core as any).logFileFd = 42; + (core as any).originalEnv = { + FIREFOX_MCP_TEST_NEWKEY: undefined, + FIREFOX_MCP_TEST_EXISTING: 'oldvalue', + }; + process.env.FIREFOX_MCP_TEST_NEWKEY = 'was-set-by-connect'; + process.env.FIREFOX_MCP_TEST_EXISTING = 'overwritten-by-connect'; + + await core.close(); + + expect(core.getCurrentContextId()).toBe(null); + expect((core as any).logFilePath).toBeUndefined(); + expect((core as any).profileWarning).toBeNull(); + expect((core as any).logFileFd).toBeUndefined(); + expect((core as any).originalEnv).toEqual({}); + expect('FIREFOX_MCP_TEST_NEWKEY' in process.env).toBe(false); + expect(process.env.FIREFOX_MCP_TEST_EXISTING).toBe('oldvalue'); + } finally { + if (savedNewKey === undefined) { + delete process.env.FIREFOX_MCP_TEST_NEWKEY; + } else { + process.env.FIREFOX_MCP_TEST_NEWKEY = savedNewKey; + } + if (savedExisting === undefined) { + delete process.env.FIREFOX_MCP_TEST_EXISTING; + } else { + process.env.FIREFOX_MCP_TEST_EXISTING = savedExisting; + } + } + }); }); });