From 8eb60b6ad5b959bd25b247e577618ee4ad5ceab2 Mon Sep 17 00:00:00 2001 From: Chris Breiding Date: Wed, 22 Feb 2023 15:47:56 -0500 Subject: [PATCH] fix: Ensure orphaned browser instance is killed before connecting to a new one (#25898) --- cli/CHANGELOG.md | 7 +- packages/server/lib/browsers/index.ts | 79 ++++++++++--- .../server/test/support/helpers/deferred.ts | 10 ++ .../test/unit/browsers/browsers_spec.js | 108 ++++++++++++++++++ .../unit/plugins/child/run_plugins_spec.js | 12 +- 5 files changed, 189 insertions(+), 27 deletions(-) create mode 100644 packages/server/test/support/helpers/deferred.ts diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index c9a09bc1386f..c7f3ad48bd28 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,7 +1,7 @@ ## 12.7.0 -_Released 03/1/2023 (PENDING)_ +_Released 02/28/2023 (PENDING)_ **Features:** @@ -14,9 +14,10 @@ _Released 03/1/2023 (PENDING)_ - Fixed an issue where cookies were being duplicated with the same hostname, but a prepended dot. Fixed an issue where cookies may not be expiring correctly. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174), [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). - Fixed an issue where cookies weren't being synced when the application was stable. Fixed in [#25855](https://github.com/cypress-io/cypress/pull/25855). Fixes [#25835](https://github.com/cypress-io/cypress/issues/25835). - Added missing TypeScript type definitions for the [`cy.reload()`](https://docs.cypress.io/api/commands/reload) command. Addressed in [#25779](https://github.com/cypress-io/cypress/pull/25779). -- Ensure Angular components are mounted inside the correct element. Fixes [#24385](https://github.com/cypress-io/cypress/issues/24385) -- Fix a bug where files outside the project root in a monorepo are not correctly served when using Vite. Addressed in [#25801](https://github.com/cypress-io/cypress/pull/25801) +- Ensure Angular components are mounted inside the correct element. Fixes [#24385](https://github.com/cypress-io/cypress/issues/24385). +- Fix a bug where files outside the project root in a monorepo are not correctly served when using Vite. Addressed in [#25801](https://github.com/cypress-io/cypress/pull/25801). - Fixed an issue where using [`cy.intercept`](https://docs.cypress.io/api/commands/intercept)'s `req.continue()` with a non-function parameter would not provide an appropriate error message. Fixed in [#25884](https://github.com/cypress-io/cypress/pull/25884). +- Fixed an issue where Cypress would erroneously launch and connect to multiple browser instances. Fixes [#24377](https://github.com/cypress-io/cypress/issues/24377). **Misc:** diff --git a/packages/server/lib/browsers/index.ts b/packages/server/lib/browsers/index.ts index 31e5b6e3c2e2..c17d24970d42 100644 --- a/packages/server/lib/browsers/index.ts +++ b/packages/server/lib/browsers/index.ts @@ -15,23 +15,39 @@ const debug = Debug('cypress:server:browsers') const isBrowserFamily = check.oneOf(BROWSER_FAMILY) let instance: BrowserInstance | null = null +let launchAttempt = 0 -const kill = function (unbind = true, isProcessExit = false) { - // Clean up the instance when the browser is closed - if (!instance) { +interface KillOptions { + instance?: BrowserInstance + isProcessExit?: boolean + nullOut?: boolean + unbind?: boolean +} + +const kill = (options: KillOptions = {}) => { + options = _.defaults({}, options, { + instance, + isProcessExit: false, + unbind: true, + nullOut: true, + }) + + const instanceToKill = options.instance + + if (!instanceToKill) { debug('browsers.kill called with no active instance') return Promise.resolve() } - const _instance = instance - - instance = null + if (options.nullOut) { + instance = null + } return new Promise((resolve) => { - _instance.once('exit', () => { - if (unbind) { - _instance.removeAllListeners() + instanceToKill.once('exit', () => { + if (options.unbind) { + instanceToKill.removeAllListeners() } debug('browser process killed') @@ -41,9 +57,9 @@ const kill = function (unbind = true, isProcessExit = false) { debug('killing browser process') - _instance.isProcessExit = isProcessExit + instanceToKill.isProcessExit = options.isProcessExit - _instance.kill() + instanceToKill.kill() }) } @@ -86,7 +102,7 @@ async function getBrowserLauncher (browser: Browser, browsers: FoundBrowser[]): return utils.throwBrowserNotFound(browser.name, browsers) } -process.once('exit', () => kill(true, true)) +process.once('exit', () => kill({ isProcessExit: true })) export = { ensureAndGetByNameOrPath: utils.ensureAndGetByNameOrPath, @@ -136,7 +152,16 @@ export = { }, async open (browser: Browser, options: BrowserLaunchOpts, automation: Automation, ctx): Promise { - await kill(true) + // this global helps keep track of which launch attempt is the latest one + launchAttempt++ + + // capture the launch attempt number for this attempt, so that if the global + // one changes in the course of launching, we know another attempt has been + // made that should supercede it. see the long comment below for more details + const thisLaunchAttempt = launchAttempt + + // kill any currently open browser instance before launching a new one + await kill() _.defaults(options, { onBrowserOpen () {}, @@ -155,6 +180,34 @@ export = { debug('browser opened') + // in most cases, we'll kill any running browser instance before launching + // a new one when we call `await kill()` early in this function. + // however, the code that calls this sets a timeout and, if that timeout + // hits, it catches the timeout error and retries launching the browser by + // calling this function again. that means any attempt to launch the browser + // isn't necessarily canceled; we just ignore its success. it's possible an + // original attempt to launch the browser eventually does succeed after + // we've already called this function again on retry. if the 1st + // (now timed-out) browser launch succeeds after this attempt to kill it, + // the 1st instance gets created but then orphaned when we override the + // `instance` singleton after the 2nd attempt succeeds. subsequent code + // expects only 1 browser to be connected at a time, so this causes wonky + // things to occur because we end up connected to and receiving messages + // from 2 browser instances. + // + // to counteract this potential race condition, we use the `launchAttempt` + // global to essentially track which browser launch attempt is the latest + // one. the latest one should always be the correct one we want to connect + // to, so if the `launchAttempt` global has changed in the course of launching + // this browser, it means it has been orphaned and should be terminated. + // + // https://github.com/cypress-io/cypress/issues/24377 + if (thisLaunchAttempt !== launchAttempt) { + await kill({ instance: _instance, nullOut: false }) + + return null + } + instance = _instance instance.browser = browser diff --git a/packages/server/test/support/helpers/deferred.ts b/packages/server/test/support/helpers/deferred.ts new file mode 100644 index 000000000000..cef58fcdd99a --- /dev/null +++ b/packages/server/test/support/helpers/deferred.ts @@ -0,0 +1,10 @@ +export const deferred = () => { + let reject + let resolve + const promise = new Promise((_resolve, _reject) => { + resolve = _resolve + reject = _reject + }) + + return { promise, resolve, reject } +} diff --git a/packages/server/test/unit/browsers/browsers_spec.js b/packages/server/test/unit/browsers/browsers_spec.js index d8ff751aade8..e5929a4e5ebd 100644 --- a/packages/server/test/unit/browsers/browsers_spec.js +++ b/packages/server/test/unit/browsers/browsers_spec.js @@ -12,7 +12,9 @@ const { exec } = require('child_process') const util = require('util') const { createTestDataContext } = require('@packages/data-context/test/unit/helper') const electron = require('../../../lib/browsers/electron') +const chrome = require('../../../lib/browsers/chrome') const Promise = require('bluebird') +const { deferred } = require('../../support/helpers/deferred') const normalizeSnapshot = (str) => { return snapshot(stripAnsi(str)) @@ -156,6 +158,112 @@ describe('lib/browsers/index', () => { expect(err).to.have.property('message').to.contain(`Browser: ${chalk.yellow('foo-bad-bang')} was not found on your system`) }) }) + + // https://github.com/cypress-io/cypress/issues/24377 + it('terminates orphaned browser if it connects while launching another instance', async () => { + const browserOptions = [{ + family: 'chromium', + }, { + url: 'http://example.com', + onBrowserOpen () {}, + }, null, ctx] + + const launchBrowser1 = deferred() + const browserInstance1 = new EventEmitter() + + browserInstance1.kill = sinon.stub() + sinon.stub(chrome, 'open').onCall(0).returns(launchBrowser1.promise) + + // attempt to launch browser + const openBrowser1 = browsers.open(...browserOptions) + const launchBrowser2 = deferred() + const browserInstance2 = new EventEmitter() + + browserInstance2.kill = sinon.stub() + chrome.open.onCall(1).returns(launchBrowser2.promise) + + // original browser launch times out, so we retry launching the browser + const openBrowser2 = browsers.open(...browserOptions) + + // in the meantime, the 1st browser launches + launchBrowser1.resolve(browserInstance1) + // allow time for 1st browser to set instance before allowing 2nd + // browser launch to move forward + await Promise.delay(10) + // the 2nd browser launches + launchBrowser2.resolve(browserInstance2) + // if we exit too soon, it will clear the instance in `open`'s exit + // handler and not trigger the condition we're looking for + await Promise.delay(10) + // finishes killing the 1st browser + browserInstance1.emit('exit') + + await openBrowser1 + await openBrowser2 + + const currentInstance = browsers.getBrowserInstance() + + // clear out instance or afterEach hook will try to kill it and + // it won't resolve. make sure this is before the assertions or + // a failing one will prevent it from happening + browsers._setInstance(null) + + expect(browserInstance1.kill).to.be.calledOnce + expect(currentInstance).to.equal(browserInstance2) + }) + + // https://github.com/cypress-io/cypress/issues/24377 + it('terminates orphaned browser if it connects after another instance launches', async () => { + const browserOptions = [{ + family: 'chromium', + }, { + url: 'http://example.com', + onBrowserOpen () {}, + }, null, ctx] + + const launchBrowser1 = deferred() + const browserInstance1 = new EventEmitter() + + browserInstance1.kill = sinon.stub() + sinon.stub(chrome, 'open').onCall(0).returns(launchBrowser1.promise) + + // attempt to launch browser + const openBrowser1 = browsers.open(...browserOptions) + const launchBrowser2 = deferred() + const browserInstance2 = new EventEmitter() + + browserInstance2.kill = sinon.stub() + chrome.open.onCall(1).returns(launchBrowser2.promise) + + // original browser launch times out, so we retry launching the browser + const openBrowser2 = browsers.open(...browserOptions) + + // the 2nd browser launches + launchBrowser2.resolve(browserInstance2) + + await openBrowser2 + + // but then the 1st browser launches + launchBrowser1.resolve(browserInstance1) + + // wait a tick for exit listener to be set up, then send 'exit' + await Promise.delay(10) + // it should be killed (asserted below) + // this finishes killing the 1st browser + browserInstance1.emit('exit') + + await openBrowser1 + + const currentInstance = browsers.getBrowserInstance() + + // clear out instance or afterEach hook will try to kill it and + // it won't resolve. make sure this is before the assertions or + // a failing one will prevent it from happening + browsers._setInstance(null) + + expect(browserInstance1.kill).to.be.calledOnce + expect(currentInstance).to.equal(browserInstance2) + }) }) context('.extendLaunchOptionsFromPlugins', () => { diff --git a/packages/server/test/unit/plugins/child/run_plugins_spec.js b/packages/server/test/unit/plugins/child/run_plugins_spec.js index 9d6eebd02077..285a9761bde8 100644 --- a/packages/server/test/unit/plugins/child/run_plugins_spec.js +++ b/packages/server/test/unit/plugins/child/run_plugins_spec.js @@ -10,21 +10,11 @@ const resolve = require(`../../../../lib/util/resolve`) const browserUtils = require(`../../../../lib/browsers/utils`) const Fixtures = require('@tooling/system-tests') const { RunPlugins } = require(`../../../../lib/plugins/child/run_plugins`) +const { deferred } = require('../../../support/helpers/deferred') const colorCodeRe = /\[[0-9;]+m/gm const pathRe = /\/?([a-z0-9_-]+\/)*[a-z0-9_-]+\/([a-z_]+\.\w+)[:0-9]+/gmi -const deferred = () => { - let reject - let resolve - const promise = new Promise((_resolve, _reject) => { - resolve = _resolve - reject = _reject - }) - - return { promise, resolve, reject } -} - const withoutColorCodes = (str) => { return str.replace(colorCodeRe, '') }