Skip to content

[MV3 Debug Extension] Handle unexpected extension debugger disconnection events without crashing app #2021

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

Merged
merged 6 commits into from
Mar 8, 2023

Conversation

elliette
Copy link
Contributor

@elliette elliette commented Mar 7, 2023

Fixes b/271611587

Before this PR, you could cause an app to crash by:

  • launching the debugger through the Dart Debug Extension
  • immediately afterwards, disconnecting the debugger by closing the banner added by Chrome

Depending on the timing, it might crash with an error thrown by DDS:

"Uncaught error, shutting down: DartDevelopmentServiceException: JSON-RPC error 100: Feature is disabled. data: Existing VM service clients prevent DDS from taking control."

Or with an error thrown by package:sse:

"Uncaught error, shutting down: Bad state: Cannot add event after closing"

This handles both cases by catching those errors, logging what went wrong, and closing the extension debugger.

}
}

/// Starts a [DebugService] for Dart Debug Extension.
Future<void> _startExtensionDebugService() async {
Future<void> _startExtensionDebugService(
ExtensionDebugger extensionDebugger) async {
final extensionDebugger = await _extensionBackend!.extensionDebugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the extensionDebugger passed to the function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops yes! That was the whole point of passing in the param...

Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

I wonder if hot restart work well with the extension (ie not closing the debugger).

Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

LTGM with one ask (could be a separate PR) - can we add tests for hot restart with the extension if we don't have any?

@elliette
Copy link
Contributor Author

elliette commented Mar 8, 2023

LTGM with one ask (could be a separate PR) - can we add tests for hot restart with the extension if we don't have any?

Sure! Opened #2023 to track

@elliette elliette merged commit 442639d into dart-lang:master Mar 8, 2023
@elliette elliette changed the title Handle unexpected extension debugger disconnection events without crashing app [MV3 Debug Extension] Handle unexpected extension debugger disconnection events without crashing app Mar 27, 2023
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.

2 participants