Skip to content

Commit

Permalink
fix: fix chrome crash when CRI client is disconnected
Browse files Browse the repository at this point in the history
  • Loading branch information
@AtofStryker
AtofStryker committed Jun 23, 2023
1 parent 0055214 commit 42f1cd3
Show file tree
Hide file tree
Showing 8 changed files with 172 additions and 28 deletions.
9 changes: 8 additions & 1 deletion packages/server/lib/browsers/cdp_automation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,14 @@ export class CdpAutomation {
async startVideoRecording (writeVideoFrame: WriteVideoFrame, screencastOpts) {
this.onFn('Page.screencastFrame', async (e) => {
writeVideoFrame(Buffer.from(e.data, 'base64'))
await this.sendDebuggerCommandFn('Page.screencastFrameAck', { sessionId: e.sessionId })
try {
await this.sendDebuggerCommandFn('Page.screencastFrameAck', { sessionId: e.sessionId })
} catch (e) {
// swallow this error if the CRI connection was reset
if (!e.message.includes('browser CRI connection was reset')) {
throw e
}
}
})

await this.sendDebuggerCommandFn('Page.startScreencast', screencastOpts)
Expand Down
5 changes: 5 additions & 0 deletions packages/server/lib/browsers/cri-client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ export const create = async (target: string, onAsynchronousError: Function, host

await reconnect()

// if enqueued commands were wiped out from the reconnect and the socket is already closed, reject the command as it will never be run
if (enqueuedCommands.length === 0 && closed) {
return Promise.reject(new Error(`${command} will not run as browser CRI connection was reset`))
}

return p
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/server/lib/browsers/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,6 @@ export = {
}

await options.onError(err)

await options.relaunchBrowser!()
}
})

Expand Down
25 changes: 15 additions & 10 deletions packages/server/lib/modes/run.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ function listenForProjectEnd (project, exit): Bluebird<any> {
})
}

async function waitForBrowserToConnect (options: { project: Project, socketId: string, onError: (err: Error) => void, spec: SpecWithRelativeRoot, isFirstSpec: boolean, testingType: string, experimentalSingleTabRunMode: boolean, browser: Browser, screenshots: ScreenshotMetadata[], projectRoot: string, shouldLaunchNewTab: boolean, webSecurity: boolean, videoRecording?: VideoRecording }) {
async function waitForBrowserToConnect (options: { project: Project, socketId: string, onError: (err: Error) => void, spec: SpecWithRelativeRoot, isFirstSpecInBrowser: boolean, testingType: string, experimentalSingleTabRunMode: boolean, browser: Browser, screenshots: ScreenshotMetadata[], projectRoot: string, shouldLaunchNewTab: boolean, webSecurity: boolean, videoRecording?: VideoRecording }) {
if (globalThis.CY_TEST_MOCK?.waitForBrowserToConnect) return Promise.resolve()

const { project, socketId, onError, spec } = options
Expand All @@ -480,7 +480,7 @@ async function waitForBrowserToConnect (options: { project: Project, socketId: s
return currentSetScreenshotMetadata(data)
}

if (options.experimentalSingleTabRunMode && options.testingType === 'component' && !options.isFirstSpec) {
if (options.experimentalSingleTabRunMode && options.testingType === 'component' && !options.isFirstSpecInBrowser) {
// reset browser state to match default behavior when opening/closing a new tab
await openProject.resetBrowserState()

Expand Down Expand Up @@ -768,7 +768,7 @@ async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, hea
})
}

let isFirstSpec = true
let isFirstSpecInBrowser = true

async function runEachSpec (spec: SpecWithRelativeRoot, index: number, length: number, estimated: number) {
const span = telemetry.startSpan({
Expand All @@ -779,16 +779,21 @@ async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, hea
span?.setAttributes({
specName: spec.name,
type: spec.specType,
firstSpec: isFirstSpec,
firstSpec: isFirstSpecInBrowser,
})

if (!options.quiet) {
printResults.displaySpecHeader(spec.relativeToCommonRoot, index + 1, length, estimated)
}

const { results } = await runSpec(config, spec, options, estimated, isFirstSpec, index === length - 1)
const { results } = await runSpec(config, spec, options, estimated, isFirstSpecInBrowser, index === length - 1)

isFirstSpec = false
if (results?.error?.includes('We detected that the Chrome process just crashed with code')) {
// If the browser has crashed, make sure isFirstSpecInBrowser is set to true as the browser will be relaunching
isFirstSpecInBrowser = true
} else {
isFirstSpecInBrowser = false
}

debug('spec results %o', results)

Expand Down Expand Up @@ -910,7 +915,7 @@ async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, hea
return results
}

async function runSpec (config, spec: SpecWithRelativeRoot, options: { project: Project, browser: Browser, onError: (err: Error) => void, config: Cfg, quiet: boolean, exit: boolean, testingType: TestingType, socketId: string, webSecurity: boolean, projectRoot: string } & Pick<Cfg, 'video' | 'videosFolder' | 'videoCompression' | 'videoUploadOnPasses'>, estimated, isFirstSpec, isLastSpec) {
async function runSpec (config, spec: SpecWithRelativeRoot, options: { project: Project, browser: Browser, onError: (err: Error) => void, config: Cfg, quiet: boolean, exit: boolean, testingType: TestingType, socketId: string, webSecurity: boolean, projectRoot: string } & Pick<Cfg, 'video' | 'videosFolder' | 'videoCompression' | 'videoUploadOnPasses'>, estimated, isFirstSpecInBrowser, isLastSpec) {
const { project, browser, onError } = options

const { isHeadless } = browser
Expand All @@ -935,7 +940,7 @@ async function runSpec (config, spec: SpecWithRelativeRoot, options: { project:

telemetry.startSpan({ name: 'video:capture' })

if (config.experimentalSingleTabRunMode && !isFirstSpec && project.videoRecording) {
if (config.experimentalSingleTabRunMode && !isFirstSpecInBrowser && project.videoRecording) {
// in single-tab mode, only the first spec needs to create a videoRecording object
// which is then re-used between specs
return await startVideoRecording({ ...opts, previous: project.videoRecording })
Expand Down Expand Up @@ -976,9 +981,9 @@ async function runSpec (config, spec: SpecWithRelativeRoot, options: { project:
webSecurity: options.webSecurity,
projectRoot: options.projectRoot,
testingType: options.testingType,
isFirstSpec,
isFirstSpecInBrowser,
experimentalSingleTabRunMode: config.experimentalSingleTabRunMode,
shouldLaunchNewTab: !isFirstSpec,
shouldLaunchNewTab: !isFirstSpecInBrowser,
}),
])

Expand Down
8 changes: 7 additions & 1 deletion packages/server/lib/open_project.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,13 @@ export class OpenProject {
}

async resetBrowserTabsForNextTest (shouldKeepTabOpen: boolean) {
return this.projectBase?.resetBrowserTabsForNextTest(shouldKeepTabOpen)
try {
await this.projectBase?.resetBrowserTabsForNextTest(shouldKeepTabOpen)
} catch (e) {
// If the CRI client disconnected or crashed, we want to no-op here so that anything
// depending on resetting the browser tabs can continue with further operations
return
}
}

async resetBrowserState () {
Expand Down
14 changes: 14 additions & 0 deletions packages/server/test/unit/browsers/cri-client_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,20 @@ describe('lib/browsers/cri-client', function () {
})
})
})

context('closed', () => {
it(`when socket is closed mid send'`, async function () {
const err = new Error('WebSocket is not open: readyState 3 (CLOSED)')

send.onFirstCall().rejects(err)

const client = await getClient()

await client.close()

expect(client.send('Browser.getVersion', { baz: 'quux' })).to.be.rejectedWith('Browser.getVersion will not run as browser CRI connection was reset')
})
})
})
})

Expand Down
114 changes: 101 additions & 13 deletions system-tests/__snapshots__/browser_crash_handling_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ https://on.cypress.io/renderer-process-crashed
`

exports['Browser Crash Handling / when the browser process crashes in chrome / fails'] = `
exports['Browser Crash Handling / when the browser process crashes in chrome / fails w/ video off'] = `
====================================================================================================
Expand Down Expand Up @@ -237,17 +237,12 @@ This can happen for many different reasons:
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Video: false │
│ Duration: X seconds │
│ Spec Ran: chrome_process_crash.cy.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
(Video)
- Video output: /XXX/XXX/XXX/cypress/videos/chrome_process_crash.cy.js.mp4
────────────────────────────────────────────────────────────────────────────────────────────────────
Running: simple.cy.js (2 of 2)
Expand All @@ -267,17 +262,12 @@ This can happen for many different reasons:
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Video: false │
│ Duration: X seconds │
│ Spec Ran: simple.cy.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
(Video)
- Video output: /XXX/XXX/XXX/cypress/videos/simple.cy.js.mp4
====================================================================================================
(Run Finished)
Expand Down Expand Up @@ -314,4 +304,102 @@ exports['Browser Crash Handling / when the browser process crashes in electron /
`

exports['Browser Crash Handling / when the browser process crashes in chrome / fails w/ video on'] = `
====================================================================================================
(Run Starting)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 2 found (chrome_process_crash.cy.js, simple.cy.js) │
│ Searched: cypress/e2e/chrome_process_crash.cy.js, cypress/e2e/simple.cy.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
────────────────────────────────────────────────────────────────────────────────────────────────────
Running: chrome_process_crash.cy.js (1 of 2)
We detected that the Chrome process just crashed with code 'null' and signal 'SIGTRAP'.
We have failed the current test and have relaunched Chrome.
This can happen for many different reasons:
- You wrote an endless loop and you must fix your own code
- You are running lots of tests on a memory intense application
- You are running in a memory starved VM environment
- There are problems with your GPU / GPU drivers
- There are browser bugs
(Results)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 0 │
│ Passing: 0 │
│ Failing: 1 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: chrome_process_crash.cy.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
(Video)
- Video output: /XXX/XXX/XXX/cypress/videos/chrome_process_crash.cy.js.mp4
────────────────────────────────────────────────────────────────────────────────────────────────────
Running: simple.cy.js (2 of 2)
✓ is true
1 passing
(Results)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 1 │
│ Passing: 1 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: simple.cy.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
(Video)
- Video output: /XXX/XXX/XXX/cypress/videos/simple.cy.js.mp4
====================================================================================================
(Run Finished)
Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✖ chrome_process_crash.cy.js XX:XX - - 1 - - │
├────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ✔ simple.cy.js XX:XX 1 1 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✖ 1 of 2 failed (50%) XX:XX 1 1 1 - -
`
23 changes: 22 additions & 1 deletion system-tests/test/browser_crash_handling_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,32 @@ describe('Browser Crash Handling', () => {

// It should fail the chrome_tab_crash spec, but the simple spec should run and succeed
context('when the browser process crashes in chrome', () => {
systemTests.it('fails', {
systemTests.it('fails w/ video off', {
browser: 'chrome',
spec: 'chrome_process_crash.cy.js,simple.cy.js',
snapshot: true,
expectedExitCode: 1,
config: {
video: false,
},
onStdout: (stdout) => {
// the location of this warning is non-deterministic
return stdout.replace('The automation client disconnected. Cannot continue running tests.\n', '')
},
})

systemTests.it('fails w/ video on', {
browser: 'chrome',
spec: 'chrome_process_crash.cy.js,simple.cy.js',
snapshot: true,
expectedExitCode: 1,
config: {
video: true,
},
onStdout: (stdout) => {
// the location of this warning is non-deterministic
return stdout.replace('The automation client disconnected. Cannot continue running tests.\n', '')
},
})
})

Expand Down

0 comments on commit 42f1cd3

Please sign in to comment.