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

Fix Flutter crash by catching WipError on resume and mapping to RPC error #2188

Merged
merged 2 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- Update SDK constraint to `>=3.1.0-254.0.dev <4.0.0`. - [#2169](https://github.com/dart-lang/webdev/pull/2169)
- Require min `build_web_compilers` version `4.0.4` - [#2171](https://github.com/dart-lang/webdev/pull/2171)
- Switch to using new debugging API from DDC to support new type system. - [#2159](https://github.com/dart-lang/webdev/pull/2159)
- Fix Flutter crash when calling `resume` when app is not paused. - [#2188](https://github.com/dart-lang/webdev/pull/2188)

## 19.0.2

Expand Down
61 changes: 37 additions & 24 deletions dwds/lib/src/debugging/debugger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -121,32 +121,45 @@ class Debugger extends Domain {
/// Note that stepping will automatically continue until Chrome is paused at
/// a location for which we have source information.
Future<Success> resume({String? step, int? frameIndex}) async {
if (frameIndex != null) {
throw ArgumentError('FrameIndex is currently unsupported.');
}
WipResponse? result;
if (step != null) {
_isStepping = true;
switch (step) {
case 'Over':
result = await _remoteDebugger.stepOver();
break;
case 'Out':
result = await _remoteDebugger.stepOut();
break;
case 'Into':
result = await _remoteDebugger.stepInto();
break;
default:
throwInvalidParam('resume', 'Unexpected value for step: $step');
try {
if (frameIndex != null) {
throw ArgumentError('FrameIndex is currently unsupported.');
}
} else {
_isStepping = false;
_previousSteppingLocation = null;
result = await _remoteDebugger.resume();
WipResponse? result;
if (step != null) {
_isStepping = true;
switch (step) {
case 'Over':
result = await _remoteDebugger.stepOver();
break;
case 'Out':
result = await _remoteDebugger.stepOut();
break;
case 'Into':
result = await _remoteDebugger.stepInto();
break;
default:
throwInvalidParam('resume', 'Unexpected value for step: $step');
}
} else {
_isStepping = false;
_previousSteppingLocation = null;
result = await _remoteDebugger.resume();
}
handleErrorIfPresent(result);
return Success();
} on WipError catch (e) {
final errorMessage = e.message;
if (errorMessage != null &&
errorMessage.contains('Can only perform operation while paused')) {
throw RPCError(
'resume',
RPCErrorKind.kIsolateMustBePaused.code,
errorMessage,
);
}
rethrow;
}
handleErrorIfPresent(result);
return Success();
}

/// Returns the current Dart stack for the paused debugger.
Expand Down
41 changes: 14 additions & 27 deletions dwds/lib/src/services/chrome_proxy_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1088,33 +1088,20 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
String? step,
int? frameIndex,
}) async {
try {
if (inspector.appConnection.isStarted) {
return captureElapsedTime(
() async {
await isInitialized;
await isStarted;
_checkIsolate('resume', isolateId);
return await (await debuggerFuture)
.resume(step: step, frameIndex: frameIndex);
},
(result) => DwdsEvent.resume(step),
);
} else {
inspector.appConnection.runMain();
return Success();
}
} on WipError catch (e) {
final errorMessage = e.message;
if (errorMessage != null &&
errorMessage.contains('Can only perform operation while paused')) {
throw RPCError(
'resume',
RPCErrorKind.kIsolateMustBePaused.code,
errorMessage,
);
}
rethrow;
if (inspector.appConnection.isStarted) {
return captureElapsedTime(
() async {
await isInitialized;
await isStarted;
_checkIsolate('resume', isolateId);
return await (await debuggerFuture)
.resume(step: step, frameIndex: frameIndex);
},
(result) => DwdsEvent.resume(step),
);
} else {
inspector.appConnection.runMain();
return Success();
}
}

Expand Down
11 changes: 8 additions & 3 deletions dwds/test/chrome_proxy_service_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1339,12 +1339,17 @@ void main() {
expect(pauseBreakpoints, hasLength(1));
expect(pauseBreakpoints.first.id, bp.id);
await service.removeBreakpoint(isolateId!, bp.id!);
});

tearDown(() async {
// Resume execution to not impact other tests.
await service.resume(isolateId!);
});

test('resuming throws kIsolateMustBePaused error if not paused',
() async {
await expectLater(
service.resume(isolateId!),
throwsRPCErrorWithCode(RPCErrorKind.kIsolateMustBePaused.code),
);
});
});

group('Step', () {
Expand Down
4 changes: 4 additions & 0 deletions dwds/test/fixtures/context.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ Matcher isRPCErrorWithMessage(String message) =>
Matcher throwsRPCErrorWithMessage(String message) =>
throwsA(isRPCErrorWithMessage(message));

Matcher isRPCErrorWithCode(int code) =>
isA<RPCError>().having((e) => e.code, 'code', equals(code));
Matcher throwsRPCErrorWithCode(int code) => throwsA(isRPCErrorWithCode(code));

enum CompilationMode { buildDaemon, frontendServer }

class TestContext {
Expand Down
Loading