fix: detect chrome browser process and tab crashes to no longer hang in CI #24338

mjhenkes · 2022-10-21T14:14:14Z

Closes Detect browser/renderer crashes #6170
Related to Detect and Recover when Browser Hangs/Crashes/Dies #22631

User facing changelog

When a chromium based browser tab or process crashes, Cypress will no longer hang indefinitely but close fail the current test and move on to the next.

Additional details

We are detecting tab crashes when cdp tells us it crashed and process crashes when the process exits with specific code/signal combinations.

This affects run mode much more than open mode. In run mode we will move on to the next spec. In open mode we will re-launch the browser to the home screen if it crashes.

Steps to test

I've created to specs that use CDP to crash the browser process and tab in the system tests e2e project. These work for any chromium based browser. This fix 'may' work for firefox but we have no way to test to see what signal/code combination we should be handling.

How has the user experience changed?

Browser tab crash - before: process hangs indefinitely

tab.hang.-.before.mov

Browser tab crash - after: process fails current spec and moves on to the next.

tab.hang.mov

Browser process crash - before: process hangs indefinitely

process.crash.-.before.mov

Browser process crash - after: process fails current spec and moves on to the next.

process.crash.-.after.mov

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

cypress-bot · 2022-10-21T14:14:18Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-10-21T14:21:45Z

Test summary

45945 • 0 • 4498 • 0 • 19

Run details


Project	cypress
Status	Passed
Commit	`dd3ec67`
Started	Oct 21, 2022 7:36 PM
Ended	Oct 21, 2022 7:53 PM
Duration	17:40 💡
OS	Linux Debian - 11.4
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/e2e/origin/cookie_behavior.cy.ts		5
1	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
2	... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
3	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
4	... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
5	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
This comment includes only the first 5 flaky tests. See all 19 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

mjhenkes · 2022-10-21T15:00:12Z

packages/errors/src/errors.ts

@@ -570,7 +570,6 @@ export const AllCypressErrors = {
        This can happen for a number of different reasons:

        - You wrote an endless loop and you must fix your own code
-        - There is a memory leak in Cypress (unlikely but possible)


Removing this to not first plant the idea that the user isn't at fault. They may not be at fault, but we'd like more investigation before they reach that conclusion.

mjhenkes · 2022-10-21T15:01:28Z

packages/server/lib/browsers/index.ts

      options.onBrowserClose()
+      browserLauncher.clearInstanceState()


we clear the instance state here because we don't 'kill' the browser since it's already dead.

mjhenkes · 2022-10-21T15:02:07Z

packages/server/lib/project-base.ts

@@ -103,7 +103,9 @@ export class ProjectBase<TServer extends Server> extends EE {
    this.options = {
      report: false,
      onFocusTests () {},
-      onError () {},
+      onError (error) {


this is a change to ensure that we're logging errors in open mode and not losing them or logging them twice.

mjhenkes · 2022-10-21T15:03:10Z

packages/server/lib/browsers/electron.ts

@@ -146,9 +146,10 @@ export = {
      onCrashed () {
        const err = errors.get('RENDERER_CRASHED')

-        errors.log(err)


by default 'onError' is defined in open mode, but it was defined as an empty function. this would lead to duplicate logging of errors in run mode. I modified the default onError to log errors and removed this call to remove the duplicate logs.

mjhenkes · 2022-10-21T15:03:39Z

system-tests/projects/e2e/cypress/e2e/chrome_process_crash.cy.js

@@ -0,0 +1,4 @@
+it('crashes the chrome process', () => {
+  Cypress.automation('remote:debugger:protocol', { command: 'Browser.crash', params: {} })


this uses cdp to crash the browser process

mjhenkes · 2022-10-21T15:03:51Z

system-tests/projects/e2e/cypress/e2e/chrome_tab_crash.cy.js

@@ -0,0 +1,4 @@
+it('crashes the chrome tab', () => {
+  Cypress.automation('remote:debugger:protocol', { command: 'Page.navigate', params: { url: 'chrome://crash', transitionType: 'typed' } })


this uses cdp to crash the browser tab

ryanthemanuel · 2022-10-21T15:09:54Z

packages/errors/src/errors.ts

@@ -570,7 +570,6 @@ export const AllCypressErrors = {
        This can happen for a number of different reasons:

        - You wrote an endless loop and you must fix your own code
-        - There is a memory leak in Cypress (unlikely but possible)


Is this no longer a concern?

Removing this to not first plant the idea that the user isn't at fault. They may not be at fault, but we'd like more investigation before they reach that conclusion.

emilyrohrbough · 2022-10-21T15:11:05Z

packages/server/lib/browsers/index.ts

      ctx.browser.setBrowserStatus('closed')
      // TODO: make this a required property
      if (!options.onBrowserClose) throw new Error('onBrowserClose did not exist in interactive mode')

+      const browserDisplayName = instance?.browser?.displayName || 'unknown'


Would Electron be the fallback here or when would we not know this value? User defied browser instance?

I'm really not sure in what cases we don't have the display name, just following typescripts lead to be honest. If we don't have a browser or a display name available i honestly don't know what could be happening.

packages/server/lib/browsers/index.ts

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

emilyrohrbough · 2022-10-21T15:13:34Z

packages/server/lib/browsers/index.ts

+
+      // We are being very narrow on when to restart the browser here. The only case we can reliably test the 'SIGTRAP' signal.
+      // We want to avoid adding signals in here that may intentionally be caused by a user.
+      // For example exiting firefox through either force quitting or quitting via cypress will fire a 'SIGTERM' event which


quitting via cypress will fire a 'SIGTERM' event which

Is this quitting like a hard kill app quit? I notice an errors randomly this when we click the close button or ctrl + c in the terminal for Cypress unexpectedly killing

Yeah sigterm is what you get when you hard kill the app. Specifically in this case we hard kill firefox. But we can't use the sigterm signal because when we force exit firefox via the app, we get cause the sigterm signal and you end up in a loop where you can never kill firefox because we restart it.

emilyrohrbough · 2022-10-21T15:21:41Z

packages/server/lib/open_project.ts

@@ -186,6 +186,8 @@ export class OpenProject {
        return await browsers.connectToNewSpec(browser, { onInitializeNewBrowserTab, ...options }, automation)


Do we need to update connectToNewSpec as well to create a new browser instance if one doesn't exist? Currently the browser instance doesn't exist, browsers.connectToNewSpec will see this isn't defined and throw:

async connectToNewSpec (browser: Browser, options: BrowserNewTabOpts, automation: Automation) { debug('connecting to new chrome tab in existing instance with url and debugging port', { url: options.url }) const browserCriClient = this._getBrowserCriClient() if (!browserCriClient) throw new Error('Missing browserCriClient in connectToNewSpec')

connectToNewSpec isn't run because 'options.shouldLaunchNewTab' is false, so we fall into the browser.open flow

When does this get reset to false? I thought this was set to true when it wasn't the first spec

i assume in onError which end up calling 'onEarlyExit' in run.ts. Theres a lot of redirections.

system-tests/test/browser_crash_handling_spec.js

packages/server/lib/browsers/index.ts

system-tests/test/browser_crash_handling_spec.js

Co-authored-by: Ryan Manuel <ryanm@cypress.io>

AtofStryker

Gave the system tests a run on Mac and Windows. looks like everything behaves the way I would expect!

AtofStryker · 2022-10-21T19:16:47Z

system-tests/test/browser_crash_handling_spec.js

+  // It should fail the chrome_tab_crash spec, but the simple spec should run and succeed
+  context('when the tab crashes in chrome', () => {
+    systemTests.it('fails', {
+      browser: 'chrome',


should we add one for edge too? Guessing no since we treat chrome/edge the same?

Yeah, edge and chrome behave the same, no harm in testing it though 🤔

Apparently edge isn't available in system tests? 🤷‍♂️

@mjhenkes Its likely because we don't run test on a window instance in CI

mjhenkes added 4 commits October 19, 2022 14:58

Handle chrome tab and browser crashes.

cc133b5

handle process crashes and add system tests

ae04388

handle crashing on windows

64f4f5d

updating comment

1c044aa

update unit tests

d50fb36

mjhenkes self-assigned this Oct 21, 2022

mjhenkes commented Oct 21, 2022

View reviewed changes

mjhenkes marked this pull request as ready for review October 21, 2022 15:04

mjhenkes changed the title ~~Matth/fix/detect chrome browser renderer crashes~~ fix: detect chrome browser process and tab crashes to no longer hang in CI Oct 21, 2022

ryanthemanuel self-requested a review October 21, 2022 15:06

AtofStryker self-requested a review October 21, 2022 15:08

ryanthemanuel reviewed Oct 21, 2022

View reviewed changes

emilyrohrbough reviewed Oct 21, 2022

View reviewed changes

packages/server/lib/browsers/index.ts Outdated Show resolved Hide resolved

Update packages/server/lib/browsers/index.ts

911102a

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

emilyrohrbough reviewed Oct 21, 2022

View reviewed changes

system-tests/test/browser_crash_handling_spec.js Show resolved Hide resolved

ryanthemanuel reviewed Oct 21, 2022

View reviewed changes

packages/server/lib/browsers/index.ts Outdated Show resolved Hide resolved

ryanthemanuel reviewed Oct 21, 2022

View reviewed changes

packages/server/lib/browsers/index.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Oct 21, 2022

View reviewed changes

system-tests/test/browser_crash_handling_spec.js Show resolved Hide resolved

Apply suggestions from code review

b0b55e0

Co-authored-by: Ryan Manuel <ryanm@cypress.io>

ryanthemanuel approved these changes Oct 21, 2022

View reviewed changes

fix flaky system test probably

dd3ec67

AtofStryker approved these changes Oct 21, 2022

View reviewed changes

emilyrohrbough approved these changes Oct 21, 2022

View reviewed changes

mjhenkes merged commit 382e0b5 into develop Oct 21, 2022

mjhenkes deleted the matth/fix/detect-chrome-browser-renderer-crashes branch October 21, 2022 20:10

cypress-bot bot mentioned this pull request Oct 21, 2022

Detect browser/renderer crashes #6170

Closed

modern-sapien mentioned this pull request Nov 4, 2022

Cypress takes forever to complete / hangs during tests #9350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: detect chrome browser process and tab crashes to no longer hang in CI #24338

fix: detect chrome browser process and tab crashes to no longer hang in CI #24338

mjhenkes commented Oct 21, 2022 •

edited by emilyrohrbough

Loading

cypress-bot bot commented Oct 21, 2022

cypress bot commented Oct 21, 2022 •

edited

Loading

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

ryanthemanuel Oct 21, 2022

mjhenkes Oct 21, 2022

emilyrohrbough Oct 21, 2022

mjhenkes Oct 21, 2022

emilyrohrbough Oct 21, 2022

mjhenkes Oct 21, 2022

emilyrohrbough Oct 21, 2022 •

edited

Loading

mjhenkes Oct 21, 2022 •

edited

Loading

emilyrohrbough Oct 21, 2022 •

edited

Loading

mjhenkes Oct 21, 2022

AtofStryker left a comment

AtofStryker Oct 21, 2022

mjhenkes Oct 21, 2022

mjhenkes Oct 21, 2022

emilyrohrbough Oct 21, 2022

		options.onBrowserClose()
		browserLauncher.clearInstanceState()

		@@ -0,0 +1,4 @@
		it('crashes the chrome process', () => {
		Cypress.automation('remote:debugger:protocol', { command: 'Browser.crash', params: {} })

		@@ -0,0 +1,4 @@
		it('crashes the chrome tab', () => {
		Cypress.automation('remote:debugger:protocol', { command: 'Page.navigate', params: { url: 'chrome://crash', transitionType: 'typed' } })

		@@ -186,6 +186,8 @@ export class OpenProject {
		return await browsers.connectToNewSpec(browser, { onInitializeNewBrowserTab, ...options }, automation)

fix: detect chrome browser process and tab crashes to no longer hang in CI #24338

fix: detect chrome browser process and tab crashes to no longer hang in CI #24338

Conversation

mjhenkes commented Oct 21, 2022 • edited by emilyrohrbough Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Oct 21, 2022

cypress bot commented Oct 21, 2022 • edited Loading

Test summary

Run details

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

mjhenkes Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjhenkes commented Oct 21, 2022 •

edited by emilyrohrbough

Loading

cypress bot commented Oct 21, 2022 •

edited

Loading

emilyrohrbough Oct 21, 2022 •

edited

Loading

mjhenkes Oct 21, 2022 •

edited

Loading

emilyrohrbough Oct 21, 2022 •

edited

Loading