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

core: handle target crash at any point #15985

Merged
merged 7 commits into from May 14, 2024
Merged

core: handle target crash at any point #15985

merged 7 commits into from May 14, 2024

Conversation

connorjclark
Copy link
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
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
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
Collaborator Author

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

Copy link
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

core/gather/session.js Outdated Show resolved Hide resolved
@connorjclark connorjclark merged commit 9c33bf5 into main May 14, 2024
27 checks passed
@connorjclark connorjclark deleted the crash-handle branch May 14, 2024 02:02
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.

None yet

4 participants