fix sigkill / treekill issues on windows with firefox binary being a …

…dangling process [run ci]

cli/lib/exec/spawn.js

@@ -11,6 +11,7 @@ const state = require('../tasks/state')
 const xvfb = require('./xvfb')
 const verify = require('../tasks/verify')
 const errors = require('../errors')
+const readline = require('readline')
 
 const isXlibOrLibudevRe = /^(?:Xlib|libudev)/
 const isHighSierraWarningRe = /\*\*\* WARNING/
@@ -236,6 +237,21 @@ module.exports = {
         child.on('exit', resolveOn('exit'))
         child.on('error', reject)
 
+        if (isPlatform('win32')) {
+          const rl = readline.createInterface({
+            input: process.stdin,
+            output: process.stdout,
+          })
+
+          // on windows, SIGINT does not propagate to the child process when ctrl+c is pressed
+          // this makes sure all nested processes are closed(ex: firefox inside the server)
+          rl.on('SIGINT', function () {
+            let kill = require('tree-kill')
+
+            kill(child.pid, 'SIGINT')
+          })
+        }
+
         // if stdio options is set to 'pipe', then
         //   we should set up pipes:
         //  process STDIN (read stream) => child STDIN (writeable)

cli/package.json

@@ -60,6 +60,7 @@
     "semver": "^7.5.3",
     "supports-color": "^8.1.1",
     "tmp": "~0.2.3",
+    "tree-kill": "1.2.2",
     "untildify": "^4.0.0",
     "yauzl": "^2.10.0"
   },

cli/test/lib/exec/spawn_spec.js

@@ -7,6 +7,9 @@ const tty = require('tty')
 const path = require('path')
 const EE = require('events')
 const mockedEnv = require('mocked-env')
+const readline = require('readline')
+const proxyquire = require('proxyquire')
+
 const debug = require('debug')('test')
 
 const state = require(`${lib}/tasks/state`)
@@ -22,6 +25,7 @@ const execPath = process.execPath
 const nodeVersion = process.versions.node
 
 const defaultBinaryDir = '/default/binary/dir'
+let mockReadlineEE
 
 describe('lib/exec/spawn', function () {
   beforeEach(function () {
@@ -49,8 +53,11 @@ describe('lib/exec/spawn', function () {
 
     // process.stdin is both an event emitter and a readable stream
     this.processStdin = new EE()
+    mockReadlineEE = new EE()
+
     this.processStdin.pipe = sinon.stub().returns(undefined)
     sinon.stub(process, 'stdin').value(this.processStdin)
+    sinon.stub(readline, 'createInterface').returns(mockReadlineEE)
     sinon.stub(cp, 'spawn').returns(this.spawnedProcess)
     sinon.stub(xvfb, 'start').resolves()
     sinon.stub(xvfb, 'stop').resolves()
@@ -387,6 +394,22 @@ describe('lib/exec/spawn', function () {
       })
     })
 
+    it('propagates treeKill if SIGINT is detected in windows console', async function () {
+      this.spawnedProcess.pid = 7
+      this.spawnedProcess.on.withArgs('close').yieldsAsync(0)
+
+      os.platform.returns('win32')
+
+      const treeKillMock = sinon.stub().returns(0)
+
+      const spawn = proxyquire(`${lib}/exec/spawn`, { 'tree-kill': treeKillMock })
+
+      await spawn.start([], { env: {} })
+
+      mockReadlineEE.emit('SIGINT')
+      expect(treeKillMock).to.have.been.calledWith(7, 'SIGINT')
+    })
+
     it('does not set windowsHide property when in darwin', function () {
       this.spawnedProcess.on.withArgs('close').yieldsAsync(0)
 

packages/server/lib/browsers/firefox.ts

@@ -11,9 +11,7 @@ import * as errors from '../errors'
 import firefoxUtil from './firefox-util'
 import utils from './utils'
 import type { Browser, BrowserInstance, GracefulShutdownOptions } from './types'
-import { EventEmitter } from 'events'
 import os from 'os'
-import treeKill from 'tree-kill'
 import mimeDb from 'mime-db'
 import type { BrowserCriClient } from './browser-cri-client'
 import type { Automation } from '../automation'
@@ -356,27 +354,6 @@ toolbar {
 
 let browserCriClient: BrowserCriClient | undefined
 
-export function _createDetachedInstance (browserInstance: BrowserInstance, browserCriClient?: BrowserCriClient): BrowserInstance {
-  const detachedInstance: BrowserInstance = new EventEmitter() as BrowserInstance
-
-  detachedInstance.pid = browserInstance.pid
-
-  // kill the entire process tree, from the spawned instance up
-  detachedInstance.kill = (): void => {
-    // Close browser cri client socket. Do nothing on failure here since we're shutting down anyway
-    if (browserCriClient) {
-      clearInstanceState({ gracefulShutdown: true })
-    }
-
-    treeKill(browserInstance.pid as number, (err?, result?) => {
-      debug('force-exit of process tree complete %o', { err, result })
-      detachedInstance.emit('exit')
-    })
-  }
-
-  return detachedInstance
-}
-
 /**
 * Clear instance state for the chrome instance, this is normally called in on kill or on exit.
 */
@@ -427,6 +404,9 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
     // and the browser will spawn maximized. The user may still supply these args to override
     // defaultLaunchOptions.args.push('--width=1920')
     // defaultLaunchOptions.args.push('--height=1081')
+  } else if (os.platform() === 'win32' || os.platform() === 'darwin') {
+    // lets the browser come into focus. Only works on Windows or Mac
+    defaultLaunchOptions.args.push('-foreground')
   }
 
   debug('firefox open %o', options)
@@ -554,7 +534,7 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
   debug('launching geckodriver with browser envs %o', BROWSER_ENVS)
 
   // create the geckodriver process, which we will use WebDriver Classic to open the browser
-  const browserInstance = await GeckoDriver.create({
+  const geckoDriverInstance = await GeckoDriver.create({
     host: '127.0.0.1',
     port: geckoDriverPort,
     marionetteHost: '127.0.0.1',
@@ -629,16 +609,10 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
     debug('setting up firefox utils')
     browserCriClient = await firefoxUtil.setup({ automation, url, foxdriverPort, webDriverClassic: wdcInstance, remotePort: cdpPort, onError: options.onError })
 
-    if (os.platform() === 'win32') {
-      // override the .kill method for Windows so that the detached Firefox process closes between specs
-      // @see https://github.com/cypress-io/cypress/issues/6392
-      return _createDetachedInstance(browserInstance, browserCriClient)
-    }
-
     // monkey-patch the .kill method to that the CDP connection is closed
-    const originalBrowserKill = browserInstance.kill
+    const originalGeckoDriverKill = geckoDriverInstance.kill
 
-    browserInstance.kill = (...args) => {
+    geckoDriverInstance.kill = (...args) => {
       // Do nothing on failure here since we're shutting down anyway
       clearInstanceState({ gracefulShutdown: true })
 
@@ -648,7 +622,7 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
 
       debug('closing geckodriver')
 
-      return originalBrowserKill.apply(browserInstance, args)
+      return originalGeckoDriverKill.apply(geckoDriverInstance, args)
     }
 
     await utils.executeAfterBrowserLaunch(browser, {
@@ -658,7 +632,7 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
     errors.throwErr('FIREFOX_COULD_NOT_CONNECT', err)
   }
 
-  return browserInstance
+  return geckoDriverInstance
 }
 
 export async function closeExtraTargets () {

packages/server/package.json

@@ -125,7 +125,6 @@
     "through": "2.3.8",
     "tough-cookie": "4.1.3",
     "trash": "5.2.0",
-    "tree-kill": "1.2.2",
     "ts-node": "^10.9.2",
     "tslib": "2.3.1",
     "underscore.string": "3.3.6",

packages/server/test/unit/browsers/firefox_spec.ts

@@ -225,6 +225,7 @@ describe('lib/browsers/firefox', () => {
                     '-new-instance',
                     '-start-debugger-server',
                     '-no-remote',
+                    ...(os.platform() !== 'linux' ? ['-foreground'] : []),
                   ],
                 },
                 'moz:debuggerAddress': true,
@@ -453,21 +454,6 @@ describe('lib/browsers/firefox', () => {
 
         expect(instance).to.eq(this.browserInstance)
       })
-
-      // @see https://github.com/cypress-io/cypress/issues/6392
-      it('detached on Windows', async function () {
-        sinon.stub(os, 'platform').returns('win32')
-        const instance = await firefox.open(this.browser, 'http://', this.options, this.automation)
-
-        expect(instance).to.not.eq(this.browserInstance)
-        expect(instance.pid).to.eq(this.browserInstance.pid)
-
-        await new Promise((resolve) => {
-          // ensure events are wired as expected
-          instance.on('exit', resolve)
-          instance.kill()
-        })
-      })
     })
   })