fix: Ensure orphaned browser instance is killed before connecting to a new one (#25898)

Author: chrisbreiding

cli/CHANGELOG.md

@@ -1,7 +1,7 @@
 <!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
 ## 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:**
 

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<void>((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<BrowserInstance | null> {
-    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
 

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 }
+}

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', () => {

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, '<color-code>')
 }