fix: Ensure orphaned browser instance is killed before connecting to a new one

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:**
 
@@ -13,9 +13,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

@@ -155,6 +155,26 @@ export = {
 
     debug('browser opened')
 
+    // in most cases, we'll kill any running browser instance before launching
+    // a new one when we call `kill(true)` on the first line of this function.
+    // however, the code that calls this sets a timeout and, if that timeout
+    // hits, it catches the timeout error and retries launcing 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 while we're "opening" the browser
+    // on the 2nd attempt (during `browserLaunch.open()` directly above), the
+    // 1st instance gets created but then orphaned when we override the
+    // `instance` singleton after the 2nd attempt succeeds. this causes wonky
+    // things to occur because we're connected to and receiving messages from
+    // 2 browser instances. attempting to kill again ensures we kill the 1st
+    // browser and only connect to the 2nd one. if we didn't run into the race
+    // condition where there's an orphaned browser instance, this call ends up
+    // a harmless no-op.
+    // https://github.com/cypress-io/cypress/issues/24377
+    await kill(true)
+
     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,54 @@ 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('kills browser if one 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
+      browsers.open(...browserOptions)
+
+      const launchBrowser2 = deferred()
+      const browserInstance2 = new EventEmitter()
+
+      browserInstance2.kill = sinon.stub()
+      chrome.open.onCall(1).returns(launchBrowser2.promise)
+
+      // 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)
+      // and 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 // Don't await this because it would have timed out?
+      await openBrowser2
+      // clear out instance or afterEach hook will try to kill it and
+      // it won't resolve
+      browsers._setInstance(null)
+
+      expect(browserInstance1.kill.callCount).to.equal(1)
+    })
   })
 
   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>')
 }