Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): close Firefox completely between specs on Windows #7106

Merged
4 changes: 4 additions & 0 deletions packages/server/lib/browsers/firefox-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ export default {

const { browser } = foxdriver

browser.on('error', (err) => {
debug('received error from foxdriver connection, ignoring %o', err)
})

forceGcCc = () => {
let gcDuration; let ccDuration

Expand Down
31 changes: 28 additions & 3 deletions packages/server/lib/browsers/firefox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import FirefoxProfile from 'firefox-profile'
import firefoxUtil from './firefox-util'
import utils from './utils'
import * as launcherDebug from '@packages/launcher/lib/log'
import { Browser } from './types'
import { Browser, BrowserInstance } from './types'
import { EventEmitter } from 'events'
import os from 'os'
import treeKill from 'tree-kill'
const errors = require('../errors')

const debug = Debug('cypress:server:browsers:firefox')
Expand Down Expand Up @@ -287,7 +290,23 @@ const defaultPreferences = {
'marionette.log.level': launcherDebug.log.enabled ? 'Debug' : undefined,
}

export async function open (browser: Browser, url, options: any = {}) {
export function _createDetachedInstance (browserInstance: BrowserInstance): 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 => {
treeKill(browserInstance.pid, (err?, result?) => {
debug('force-exit of process tree complete %o', { err, result })
detachedInstance.emit('exit')
})
}

return detachedInstance
}

export async function open (browser: Browser, url, options: any = {}): Bluebird<BrowserInstance> {
const defaultLaunchOptions = utils.getDefaultLaunchOptions({
extensions: [] as string[],
preferences: _.extend({}, defaultPreferences),
Expand Down Expand Up @@ -347,7 +366,7 @@ export async function open (browser: Browser, url, options: any = {}) {
launchOptions,
] = await Bluebird.all([
utils.ensureCleanCache(browser, options.isTextTerminal),
utils.writeExtension(browser, options.isTextTerminal, options.proxyUrl, options.socketIoRoute, options.onScreencastFrame),
utils.writeExtension(browser, options.isTextTerminal, options.proxyUrl, options.socketIoRoute),
utils.executeBeforeBrowserLaunch(browser, defaultLaunchOptions, options),
])

Expand Down Expand Up @@ -419,5 +438,11 @@ export async function open (browser: Browser, url, options: any = {}) {
errors.throw('FIREFOX_COULD_NOT_CONNECT', err)
})

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

return browserInstance
}
6 changes: 6 additions & 0 deletions packages/server/lib/browsers/types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { FoundBrowser } from '@packages/launcher'
import { EventEmitter } from 'events'

export type Browser = FoundBrowser & {
majorVersion: number
isHeadless: boolean
isHeaded: boolean
}

export type BrowserInstance = EventEmitter & {
kill: () => void
pid: number
}
6 changes: 3 additions & 3 deletions packages/server/lib/browsers/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,12 @@ export = {

launch: launcher.launch,

writeExtension (browser, isTextTerminal, proxyUrl, socketIoRoute, onScreencastFrame) {
writeExtension (browser, isTextTerminal, proxyUrl, socketIoRoute) {
debug('writing extension')

// debug('writing extension to chrome browser')
// get the string bytes for the final extension file
return extension.setHostAndPath(proxyUrl, socketIoRoute, onScreencastFrame)
return extension.setHostAndPath(proxyUrl, socketIoRoute)
.then((str) => {
const extensionDest = getExtensionDir(browser, isTextTerminal)
const extensionBg = path.join(extensionDest, 'background.js')
Expand All @@ -231,7 +231,7 @@ export = {
debug('getBrowsers')

return launcher.detect()
.then((browsers = []) => {
.then((browsers: FoundBrowser[] = []) => {
let majorVersion

debug('found browsers %o', { browsers })
Expand Down
1 change: 1 addition & 0 deletions packages/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
"term-size": "2.1.0",
"tough-cookie": "4.0.0",
"trash": "5.2.0",
"tree-kill": "1.2.2",
"ts-node": "8.5.4",
"underscore": "1.9.1",
"underscore.string": "3.3.5",
Expand Down
8 changes: 8 additions & 0 deletions packages/server/test/e2e/1_firefox_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,12 @@ describe('e2e firefox', function () {
expect(stderr).not.to.include('foxdriver')
},
})

// NOTE: only an issue on windows
// https://github.com/cypress-io/cypress/issues/6392
e2e.it.skip('can run multiple specs', {
bahmutov marked this conversation as resolved.
Show resolved Hide resolved
browser: 'firefox',
project: Fixtures.projectPath('e2e'),
spec: 'simple_spec.coffee,simple_passing_spec.coffee',
})
})
33 changes: 31 additions & 2 deletions packages/server/test/unit/browsers/firefox_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as firefox from '../../../lib/browsers/firefox'
import { EventEmitter } from 'events'
import Marionette from 'marionette-client'
import Foxdriver from '@benmalka/foxdriver'
import os from 'os'
const mockfs = require('mock-fs')
const FirefoxProfile = require('firefox-profile')
const utils = require('../../../lib/browsers/utils')
Expand Down Expand Up @@ -67,6 +68,7 @@ describe('lib/browsers/firefox', () => {
const browser = {
listTabs: sinon.stub().resolves([foxdriverTab]),
request: sinon.stub().withArgs('listTabs').resolves({ tabs: [foxdriverTab] }),
on: sinon.stub(),
}

foxdriver = {
Expand Down Expand Up @@ -102,14 +104,18 @@ describe('lib/browsers/firefox', () => {
browser: this.browser,
}

this.browserInstance = {
// should be high enough to not kill any real PIDs
pid: Number.MAX_SAFE_INTEGER,
}

sinon.stub(process, 'pid').value(1111)

protocol.foo = 'bar'

sinon.stub(plugins, 'has')
sinon.stub(plugins, 'execute')
sinon.stub(utils, 'writeExtension').resolves('/path/to/ext')
this.browserInstance = {}
sinon.stub(utils, 'launch').resolves(this.browserInstance)
sinon.spy(FirefoxProfile.prototype, 'setPreference')
sinon.spy(FirefoxProfile.prototype, 'updatePreferences')
Expand Down Expand Up @@ -201,7 +207,7 @@ describe('lib/browsers/firefox', () => {

it('writes extension', function () {
return firefox.open(this.browser, 'http://', this.options).then(() => {
expect(utils.writeExtension).to.be.calledWith(this.options.browser, this.options.isTextTerminal, this.options.proxyUrl, this.options.socketIoRoute, this.options.onScreencastFrame)
expect(utils.writeExtension).to.be.calledWith(this.options.browser, this.options.isTextTerminal, this.options.proxyUrl, this.options.socketIoRoute)
})
})

Expand Down Expand Up @@ -322,6 +328,29 @@ describe('lib/browsers/firefox', () => {
expect(wrapperErr.message).to.include(err.message)
})
})

context('returns BrowserInstance', function () {
it('from browsers.launch', async function () {
const instance = await firefox.open(this.browser, 'http://', this.options)

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)

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()
})
})
})
})

context('firefox-util', () => {
Expand Down
13 changes: 2 additions & 11 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -13952,16 +13952,7 @@ http-proxy-agent@^2.1.0:
agent-base "4"
debug "3.1.0"

http-proxy@^1.17.0:
version "1.18.1"
resolved "https://registry.yarnpkg.com/http-proxy/-/http-proxy-1.18.1.tgz#401541f0534884bbf95260334e72f88ee3976549"
integrity sha512-7mz/721AbnJwIVbnaSv1Cz3Am0ZLT/UBwkC92VlxhXv/k/BBQfM2fXElQNC27BVGr0uwUpplYPQM9LnaBMR5NQ==
dependencies:
eventemitter3 "^4.0.0"
follow-redirects "^1.0.0"
requires-port "^1.0.0"

http-proxy@cypress-io/node-http-proxy#9322b4b69b34f13a6f3874e660a35df3305179c6:
http-proxy@^1.17.0, http-proxy@cypress-io/node-http-proxy#9322b4b69b34f13a6f3874e660a35df3305179c6:
version "1.18.0"
resolved "https://codeload.github.com/cypress-io/node-http-proxy/tar.gz/9322b4b69b34f13a6f3874e660a35df3305179c6"
dependencies:
Expand Down Expand Up @@ -24421,7 +24412,7 @@ traverse-chain@~0.1.0:
resolved "https://registry.yarnpkg.com/traverse-chain/-/traverse-chain-0.1.0.tgz#61dbc2d53b69ff6091a12a168fd7d433107e40f1"
integrity sha1-YdvC1Ttp/2CRoSoWj9fUMxB+QPE=

tree-kill@^1.1.0:
tree-kill@1.2.2, tree-kill@^1.1.0:
version "1.2.2"
resolved "https://registry.yarnpkg.com/tree-kill/-/tree-kill-1.2.2.tgz#4ca09a9092c88b73a7cdc5e8a01b507b0790a0cc"
integrity sha512-L0Orpi8qGpRG//Nd+H90vFB+3iHnue1zSSGmNOOCh1GLJ7rUKVwV2HvijphGQS2UmhUZewS9VgvxYIdgr+fG1A==
Expand Down