From 42f1cd38937990990427b14ff8963ba38761d0f7 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Fri, 16 Jun 2023 16:19:18 -0400 Subject: [PATCH] fix: fix chrome crash when CRI client is disconnected --- .../server/lib/browsers/cdp_automation.ts | 9 +- packages/server/lib/browsers/cri-client.ts | 5 + packages/server/lib/browsers/index.ts | 2 - packages/server/lib/modes/run.ts | 25 ++-- packages/server/lib/open_project.ts | 8 +- .../test/unit/browsers/cri-client_spec.ts | 14 +++ .../browser_crash_handling_spec.js | 114 ++++++++++++++++-- .../test/browser_crash_handling_spec.js | 23 +++- 8 files changed, 172 insertions(+), 28 deletions(-) diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts index b43e65763f19..732a63cc5036 100644 --- a/packages/server/lib/browsers/cdp_automation.ts +++ b/packages/server/lib/browsers/cdp_automation.ts @@ -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) diff --git a/packages/server/lib/browsers/cri-client.ts b/packages/server/lib/browsers/cri-client.ts index c40026f94bc9..761db83f0065 100644 --- a/packages/server/lib/browsers/cri-client.ts +++ b/packages/server/lib/browsers/cri-client.ts @@ -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 } } diff --git a/packages/server/lib/browsers/index.ts b/packages/server/lib/browsers/index.ts index c17d24970d42..10d0df4b29fd 100644 --- a/packages/server/lib/browsers/index.ts +++ b/packages/server/lib/browsers/index.ts @@ -241,8 +241,6 @@ export = { } await options.onError(err) - - await options.relaunchBrowser!() } }) diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts index 9ce36c8070a7..b0ae32088110 100644 --- a/packages/server/lib/modes/run.ts +++ b/packages/server/lib/modes/run.ts @@ -458,7 +458,7 @@ function listenForProjectEnd (project, exit): Bluebird { }) } -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 @@ -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() @@ -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({ @@ -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) @@ -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, 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, estimated, isFirstSpecInBrowser, isLastSpec) { const { project, browser, onError } = options const { isHeadless } = browser @@ -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 }) @@ -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, }), ]) diff --git a/packages/server/lib/open_project.ts b/packages/server/lib/open_project.ts index a278189c6ff1..dbfbf4879f72 100644 --- a/packages/server/lib/open_project.ts +++ b/packages/server/lib/open_project.ts @@ -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 () { diff --git a/packages/server/test/unit/browsers/cri-client_spec.ts b/packages/server/test/unit/browsers/cri-client_spec.ts index 896d7fbe6876..5c3684e12d71 100644 --- a/packages/server/test/unit/browsers/cri-client_spec.ts +++ b/packages/server/test/unit/browsers/cri-client_spec.ts @@ -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') + }) + }) }) }) diff --git a/system-tests/__snapshots__/browser_crash_handling_spec.js b/system-tests/__snapshots__/browser_crash_handling_spec.js index dedf8453b995..6d714856bc66 100644 --- a/system-tests/__snapshots__/browser_crash_handling_spec.js +++ b/system-tests/__snapshots__/browser_crash_handling_spec.js @@ -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'] = ` ==================================================================================================== @@ -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) @@ -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) @@ -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 - - + + ` diff --git a/system-tests/test/browser_crash_handling_spec.js b/system-tests/test/browser_crash_handling_spec.js index 5852c5fc4b14..0fce49a5af1b 100644 --- a/system-tests/test/browser_crash_handling_spec.js +++ b/system-tests/test/browser_crash_handling_spec.js @@ -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', '') + }, }) })