diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 00ba661c1f..0cb5787cf3 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -142,6 +142,8 @@ export class Chromium extends AbstractBrowserType { pipe: true, tempDir: temporaryUserDataDir || undefined, attemptToGracefullyClose: async () => { + if ((options as any).__testHookGracefullyClose) + await (options as any).__testHookGracefullyClose(); debugAssert(browserServer._isInitialized()); // We try to gracefully close to prevent crash reporting and core dumps. // Note that it's fine to reuse the pipe transport, since diff --git a/src/server/firefox.ts b/src/server/firefox.ts index 8ce46cf0e8..09215db5b8 100644 --- a/src/server/firefox.ts +++ b/src/server/firefox.ts @@ -137,6 +137,8 @@ export class Firefox extends AbstractBrowserType { pipe: false, tempDir: temporaryProfileDir || undefined, attemptToGracefullyClose: async () => { + if ((options as any).__testHookGracefullyClose) + await (options as any).__testHookGracefullyClose(); debugAssert(browserServer._isInitialized()); // We try to gracefully close to prevent crash reporting and core dumps. const transport = await WebSocketTransport.connect(browserWSEndpoint!, async transport => transport); diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index 1dc5d8450d..9c046212ae 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -111,18 +111,21 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { - spawnedProcess.once('exit', (exitCode, signal) => { - logger._log(browserLog, ``); - processClosed = true; - helper.removeEventListeners(listeners); - options.onkill(exitCode, signal); - // Cleanup as processes exit. - Promise.all([ - options.omitDownloads ? Promise.resolve() : removeFolderAsync(downloadsPath), - options.tempDir ? removeFolderAsync(options.tempDir) : Promise.resolve() - ]).catch((err: Error) => console.error(err)).then(fulfill); - }); + let fulfillClose = () => {}; + const waitForClose = new Promise(f => fulfillClose = f); + let fulfillCleanup = () => {}; + const waitForCleanup = new Promise(f => fulfillCleanup = f); + spawnedProcess.once('exit', (exitCode, signal) => { + logger._log(browserLog, ``); + processClosed = true; + helper.removeEventListeners(listeners); + options.onkill(exitCode, signal); + fulfillClose(); + // Cleanup as processes exit. + Promise.all([ + options.omitDownloads ? Promise.resolve() : removeFolderAsync(downloadsPath), + options.tempDir ? removeFolderAsync(options.tempDir) : Promise.resolve() + ]).catch((err: Error) => console.error(err)).then(fulfillCleanup); }); const listeners = [ helper.addEventListener(process, 'exit', killProcess) ]; @@ -145,12 +148,13 @@ export async function launchProcess(options: LaunchProcessOptions): Promise`); killProcess(); + await waitForClose; // Ensure the process is dead and we called options.onkill. return; } gracefullyClosing = true; logger._log(browserLog, ``); await options.attemptToGracefullyClose().catch(() => killProcess()); - await waitForProcessToClose; + await waitForCleanup; // Ensure the process is dead and we have cleaned up. logger._log(browserLog, ``); } @@ -169,10 +173,12 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { pipe: true, tempDir: temporaryUserDataDir || undefined, attemptToGracefullyClose: async () => { + if ((options as any).__testHookGracefullyClose) + await (options as any).__testHookGracefullyClose(); assert(transport); // We try to gracefully close to prevent crash reporting and core dumps. // Note that it's fine to reuse the pipe transport, since diff --git a/test/fixtures.spec.js b/test/fixtures.spec.js index c18e5a233d..870c6d2eb8 100644 --- a/test/fixtures.spec.js +++ b/test/fixtures.spec.js @@ -19,57 +19,90 @@ const path = require('path'); const {spawn, execSync} = require('child_process'); const {FFOX, CHROMIUM, WEBKIT, WIN, LINUX} = require('./utils').testOptions(browserType); -async function testSignal(state, action, exitOnClose) { - const options = Object.assign({}, state.defaultBrowserOptions, { - handleSIGINT: true, - handleSIGTERM: true, - handleSIGHUP: true, - executablePath: state.browserType.executablePath(), - logger: undefined, - }); - const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), path.join(state.playwrightPath, 'index-for-dev'), state.browserType.name(), JSON.stringify(options), exitOnClose ? 'true' : '']); - let wsEndPointCallback; - const wsEndPointPromise = new Promise(x => wsEndPointCallback = x); - let output = ''; - let browserExitCode = 'none'; - let browserSignal = 'none'; - let browserPid; - res.stdout.on('data', data => { - output += data.toString(); - // Uncomment to debug these tests. - // console.log(data.toString()); - let match = output.match(/browserWS:(.+):browserWS/); - if (match) - wsEndPointCallback(match[1]); - match = output.match(/browserClose:([^:]+):([^:]+):browserClose/); - if (match) { - browserExitCode = match[1]; - browserSignal = match[2]; - } - match = output.match(/browserPid:([^:]+):browserPid/); - if (match) - browserPid = +match[1]; - }); - res.on('error', (...args) => console.log("ERROR", ...args)); - const browser = await state.browserType.connect({ wsEndpoint: await wsEndPointPromise }); - const promises = [ - new Promise(resolve => browser.once('disconnected', resolve)), - new Promise(resolve => res.on('exit', resolve)), - ]; - action(res, browserPid); - const [, exitCode] = await Promise.all(promises); - return { exitCode, browserSignal, browserExitCode, output }; +class Wrapper { + constructor(state, extraOptions) { + this._output = new Map(); + this._outputCallback = new Map(); + + this._browserType = state.browserType; + const launchOptions = {...state.defaultBrowserOptions, + handleSIGINT: true, + handleSIGTERM: true, + handleSIGHUP: true, + executablePath: state.browserType.executablePath(), + logger: undefined, + }; + const options = { + playwrightFile: path.join(state.playwrightPath, 'index-for-dev'), + browserTypeName: state.browserType.name(), + launchOptions, + ...extraOptions, + }; + this._child = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), JSON.stringify(options)]); + this._child.on('error', (...args) => console.log("ERROR", ...args)); + this._exitPromise = new Promise(resolve => this._child.on('exit', resolve)); + + let outputString = ''; + this._child.stdout.on('data', data => { + outputString += data.toString(); + // Uncomment to debug. + // console.log(data.toString()); + let match; + while (match = outputString.match(/\(([^()]+)=>([^()]+)\)/)) { + const key = match[1]; + const value = match[2]; + this._addOutput(key, value); + outputString = outputString.substring(match.index + match[0].length); + } + }); + } + + _addOutput(key, value) { + this._output.set(key, value); + const cb = this._outputCallback.get(key); + this._outputCallback.delete(key); + if (cb) + cb(); + } + + async out(key) { + if (!this._output.has(key)) + await new Promise(f => this._outputCallback.set(key, f)); + return this._output.get(key); + } + + async connect() { + const wsEndpoint = await this.out('wsEndpoint'); + const browser = await this._browserType.connect({ wsEndpoint }); + this._exitAndDisconnectPromise = Promise.all([ + this._exitPromise, + new Promise(resolve => browser.once('disconnected', resolve)), + ]).then(([exitCode]) => exitCode); + } + + child() { + return this._child; + } + + async childExitCode() { + return await this._exitAndDisconnectPromise; + } +} + +async function setup(state, options = {}) { + const wrapper = new Wrapper(state, options); + await wrapper.connect(); + return wrapper; } describe('Fixtures', function() { it.slow()('should close the browser when the node process closes', async state => { - const result = await testSignal(state, child => { - if (WIN) - execSync(`taskkill /pid ${child.pid} /T /F`); - else - process.kill(child.pid); - }); - expect(result.exitCode).toBe(WIN ? 1 : 0); + const wrapper = await setup(state); + if (WIN) + execSync(`taskkill /pid ${wrapper.child().pid} /T /F`); + else + process.kill(wrapper.child().pid); + expect(await wrapper.childExitCode()).toBe(WIN ? 1 : 0); // We might not get browser exitCode in time when killing the parent node process, // so we don't check it here. }); @@ -77,65 +110,70 @@ describe('Fixtures', function() { describe.skip(WIN).skip(!HEADLESS)('signals', () => { // Cannot reliably send signals on Windows. it.slow()('should report browser close signal', async state => { - const result = await testSignal(state, (child, browserPid) => process.kill(browserPid), true); - expect(result.exitCode).toBe(0); - expect(result.browserExitCode).toBe('null'); - expect(result.browserSignal).toBe('SIGTERM'); + const wrapper = await setup(state); + const pid = await wrapper.out('pid'); + process.kill(-pid, 'SIGTERM'); + expect(await wrapper.out('exitCode')).toBe('null'); + expect(await wrapper.out('signal')).toBe('SIGTERM'); + process.kill(wrapper.child().pid); + await wrapper.childExitCode(); }); it.slow()('should report browser close signal 2', async state => { - const result = await testSignal(state, (child, browserPid) => process.kill(browserPid, 'SIGKILL'), true); - expect(result.exitCode).toBe(0); - expect(result.browserExitCode).toBe('null'); - expect(result.browserSignal).toBe('SIGKILL'); + const wrapper = await setup(state); + const pid = await wrapper.out('pid'); + process.kill(-pid, 'SIGKILL'); + expect(await wrapper.out('exitCode')).toBe('null'); + expect(await wrapper.out('signal')).toBe('SIGKILL'); + process.kill(wrapper.child().pid); + await wrapper.childExitCode(); }); it.slow()('should close the browser on SIGINT', async state => { - const result = await testSignal(state, child => process.kill(child.pid, 'SIGINT')); - expect(result.exitCode).toBe(130); - expect(result.browserExitCode).toBe('0'); - expect(result.browserSignal).toBe('null'); + const wrapper = await setup(state); + process.kill(wrapper.child().pid, 'SIGINT'); + expect(await wrapper.out('exitCode')).toBe('0'); + expect(await wrapper.out('signal')).toBe('null'); + expect(await wrapper.childExitCode()).toBe(130); }); it.slow()('should close the browser on SIGTERM', async state => { - const result = await testSignal(state, child => process.kill(child.pid, 'SIGTERM')); - expect(result.exitCode).toBe(0); - expect(result.browserExitCode).toBe('0'); - expect(result.browserSignal).toBe('null'); + const wrapper = await setup(state); + process.kill(wrapper.child().pid, 'SIGTERM'); + expect(await wrapper.out('exitCode')).toBe('0'); + expect(await wrapper.out('signal')).toBe('null'); + expect(await wrapper.childExitCode()).toBe(0); }); it.slow()('should close the browser on SIGHUP', async state => { - const result = await testSignal(state, child => process.kill(child.pid, 'SIGHUP')); - expect(result.exitCode).toBe(0); - expect(result.browserExitCode).toBe('0'); - expect(result.browserSignal).toBe('null'); + const wrapper = await setup(state); + process.kill(wrapper.child().pid, 'SIGHUP'); + expect(await wrapper.out('exitCode')).toBe('0'); + expect(await wrapper.out('signal')).toBe('null'); + expect(await wrapper.childExitCode()).toBe(0); }); it.slow()('should kill the browser on double SIGINT', async state => { - const result = await testSignal(state, child => { - process.kill(child.pid, 'SIGINT'); - process.kill(child.pid, 'SIGINT'); - }); - expect(result.exitCode).toBe(130); - // TODO: ideally, we would expect the SIGKILL on the browser from - // force kill, but that's racy with sending two signals. + const wrapper = await setup(state, { stallOnClose: true }); + process.kill(wrapper.child().pid, 'SIGINT'); + await wrapper.out('stalled'); + process.kill(wrapper.child().pid, 'SIGINT'); + expect(await wrapper.out('exitCode')).toBe('null'); + expect(await wrapper.out('signal')).toBe('SIGKILL'); + expect(await wrapper.childExitCode()).toBe(130); }); - // TODO: flaky - https://app.circleci.com/pipelines/github/microsoft/playwright/582/workflows/b49033ce-fe20-4029-b665-13fb331f842e/jobs/579 - it.slow().fail(FFOX && LINUX)('should kill the browser on SIGINT + SIGTERM', async state => { - const result = await testSignal(state, child => { - process.kill(child.pid, 'SIGINT'); - process.kill(child.pid, 'SIGTERM'); - }); - expect(result.exitCode).toBe(130); - // TODO: ideally, we would expect the SIGKILL on the browser from - // force kill, but that's racy with sending two signals. + it.slow()('should kill the browser on SIGINT + SIGTERM', async state => { + const wrapper = await setup(state, { stallOnClose: true }); + process.kill(wrapper.child().pid, 'SIGINT'); + await wrapper.out('stalled'); + process.kill(wrapper.child().pid, 'SIGTERM'); + expect(await wrapper.out('exitCode')).toBe('null'); + expect(await wrapper.out('signal')).toBe('SIGKILL'); + expect(await wrapper.childExitCode()).toBe(0); }); - // TODO: flaky! - // - firefox: https://github.com/microsoft/playwright/pull/1911/checks?check_run_id=607148951 - // - chromium: https://travis-ci.com/github/microsoft/playwright/builds/161356178 - it.slow().fail((FFOX || CHROMIUM) && LINUX)('should kill the browser on SIGTERM + SIGINT', async state => { - const result = await testSignal(state, child => { - process.kill(child.pid, 'SIGTERM'); - process.kill(child.pid, 'SIGINT'); - }); - expect(result.exitCode).toBe(130); - // TODO: ideally, we would expect the SIGKILL on the browser from - // force kill, but that's racy with sending two signals. + it.slow()('should kill the browser on SIGTERM + SIGINT', async state => { + const wrapper = await setup(state, { stallOnClose: true }); + process.kill(wrapper.child().pid, 'SIGTERM'); + await wrapper.out('stalled'); + process.kill(wrapper.child().pid, 'SIGINT'); + expect(await wrapper.out('exitCode')).toBe('null'); + expect(await wrapper.out('signal')).toBe('SIGKILL'); + expect(await wrapper.childExitCode()).toBe(130); }); }); }); diff --git a/test/fixtures/closeme.js b/test/fixtures/closeme.js index 10790c7d89..b46c95bdf8 100644 --- a/test/fixtures/closeme.js +++ b/test/fixtures/closeme.js @@ -1,11 +1,16 @@ (async() => { - const [, , playwrightIndex, browserType, options, exitOnClose] = process.argv; - const browserServer = await require(playwrightIndex)[browserType].launchServer(JSON.parse(options)); + const { playwrightFile, browserTypeName, launchOptions, stallOnClose } = JSON.parse(process.argv[2]); + if (stallOnClose) { + launchOptions.__testHookGracefullyClose = () => { + console.log(`(stalled=>true)`); + return new Promise(() => {}); + }; + } + const browserServer = await require(playwrightFile)[browserTypeName].launchServer(launchOptions); browserServer.on('close', (exitCode, signal) => { - console.log(`browserClose:${exitCode}:${signal}:browserClose`); - if (exitOnClose) - process.exit(0); + console.log(`(exitCode=>${exitCode})`); + console.log(`(signal=>${signal})`); }); - console.log(`browserPid:${browserServer.process().pid}:browserPid`); - console.log(`browserWS:${browserServer.wsEndpoint()}:browserWS`); + console.log(`(pid=>${browserServer.process().pid})`); + console.log(`(wsEndpoint=>${browserServer.wsEndpoint()})`); })();