Skip to content

core: handle target crash at any point#15985

Merged
connorjclark merged 7 commits intomainfrom
crash-handle
May 14, 2024
Merged

core: handle target crash at any point#15985
connorjclark merged 7 commits intomainfrom
crash-handle

Conversation

@connorjclark
Copy link
Copy Markdown
Collaborator

@connorjclark connorjclark commented May 7, 2024

#11840 added a specific error code when the chrome target crashes, but it only applied to navigations within the waitFor condition check. Any other runner, or during teardown in navigation, and a crash would still result in a PROTOCOL_TIMEOUT.

This PR adds the crash promise to ProtocolSession, and checks during any sendCommand that there was not a crash.

Alternatively, we can introduce a wrapper around gatherFn and check if the Driver is in a crash state. This PR has both approaches (thus Draft).

Copy link
Copy Markdown
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bringing fatal promise over to the session is awesome. works great. let's go with that

for my own posterity.. i test this with adding this to gatherers/trace.js right after the Tracing.end line:

      setTimeout(() => {
        session.sendCommand('Page.navigate', {url: 'chrome://crash'});
      }, 100);

@connorjclark
Copy link
Copy Markdown
Collaborator Author

bringing fatal promise over to the session is awesome. works great. let's go with that

OK

I realized the Driver shouldn't be the thing handling this crash promise. I just moved it to ProtocolSession, dropped all the wiring being used to get it there, and added a session.onCrashPromise() for use by navigation's wait-for-conditions.

@connorjclark connorjclark marked this pull request as ready for review May 13, 2024 17:51
@connorjclark connorjclark requested a review from a team as a code owner May 13, 2024 17:51
@connorjclark connorjclark requested review from adamraine and removed request for a team May 13, 2024 17:51
@connorjclark
Copy link
Copy Markdown
Collaborator Author

Oh, this new approach also means we handle crashes in all sessions, not just the root.

Copy link
Copy Markdown
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not gonna lie this is like.. sooooooooooooooo good.

net negative line count change.. and expanded the functionality. and its easier to follow.

just a huge win all around.

tested it a few ways and it looks good. i get an LHR regardless, whenever the crash happens.

NIIIIIIIIIIIIIIIIIIICE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants