Fix hot restart and breakpoints races

dwds/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 11.2.3-dev
+
+- Fix race causing intermittent `Aww, snap` errors on starting debugger
+  with multiple breakpoints in source.
+- Fix needing chrome to be focus in order to wait for the isolate to
+  exit on hot restart.
+
 ## 11.2.2
 
 - Depend on `dds` version `2.1.1`.

dwds/lib/src/debugging/debugger.dart

@@ -9,6 +9,7 @@ import 'dart:math' as math;
 
 import 'package:logging/logging.dart';
 import 'package:meta/meta.dart';
+import 'package:pool/pool.dart';
 import 'package:vm_service/vm_service.dart';
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'
     hide StackTrace;
@@ -187,6 +188,7 @@ class Debugger extends Domain {
     runZonedGuarded(() {
       _remoteDebugger?.onPaused?.listen(_pauseHandler);
       _remoteDebugger?.onResumed?.listen(_resumeHandler);
+      _remoteDebugger?.onTargetCrashed?.listen(_crashHandler);
     }, (e, StackTrace s) {
       logger.warning('Error handling Chrome event', e, s);
     });
@@ -580,6 +582,23 @@ class Debugger extends Domain {
     _streamNotify('Debug', event);
   }
 
+  /// Handles targetCrashed events coming from the Chrome connection.
+  Future<void> _crashHandler(TargetCrashedEvent _) async {
+    // We can receive a resume event in the middle of a reload which will result
+    // in a null isolate.
+    var isolate = inspector?.isolate;
+    if (isolate == null) return;
+
+    stackComputer = null;
+    var event = Event(
+        kind: EventKind.kIsolateExit,
+        timestamp: DateTime.now().millisecondsSinceEpoch,
+        isolate: inspector.isolateRef);
+    isolate.pauseEvent = event;
+    _streamNotify('Isolate', event);
+    logger.severe('Target crashed!');
+  }
+
   /// Evaluate [expression] by calling Chrome's Runtime.evaluate
   Future<RemoteObject> evaluate(String expression) async {
     try {
@@ -655,6 +674,8 @@ class _Breakpoints extends Domain {
 
   final _bpByDartId = <String, Future<Breakpoint>>{};
 
+  final _pool = Pool(1);
+
   final Locations locations;
   final RemoteDebugger remoteDebugger;
 
@@ -722,12 +743,16 @@ class _Breakpoints extends Domain {
 
     // The module can be loaded from a nested path and contain an ETAG suffix.
     var urlRegex = '.*${location.jsLocation.module}.*';
-    var response = await remoteDebugger
-        .sendCommand('Debugger.setBreakpointByUrl', params: {
-      'urlRegex': urlRegex,
-      'lineNumber': location.jsLocation.line - 1,
+    // Prevent `Aww, snap!` errors when setting multiple breakpoints
+    // simultaneously by serializing the requests.
+    return _pool.withResource(() async {
+      var response = await remoteDebugger
+          .sendCommand('Debugger.setBreakpointByUrl', params: {
+        'urlRegex': urlRegex,
+        'lineNumber': location.jsLocation.line - 1,
+      });
+      return response.result['breakpointId'] as String;
     });
-    return response.result['breakpointId'] as String;
   }
 
   /// Records the internal Dart <=> JS breakpoint id mapping and adds the

dwds/lib/src/debugging/remote_debugger.dart

@@ -6,6 +6,10 @@
 
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 
+class TargetCrashedEvent extends WipEvent {
+  TargetCrashedEvent(Map<String, dynamic> json) : super(json);
+}
+
 /// A generic debugger used in remote debugging.
 abstract class RemoteDebugger {
   Stream<ConsoleAPIEvent> get onConsoleAPICalled;
@@ -20,6 +24,8 @@ abstract class RemoteDebugger {
 
   Stream<ScriptParsedEvent> get onScriptParsed;
 
+  Stream<TargetCrashedEvent> get onTargetCrashed;
+
   Map<String, WipScript> get scripts;
 
   Stream<void> get onClose;

dwds/lib/src/debugging/webkit_debugger.dart

@@ -112,6 +112,11 @@ class WebkitDebugger implements RemoteDebugger {
   @override
   Stream<ScriptParsedEvent> get onScriptParsed => _wipDebugger.onScriptParsed;
 
+  @override
+  Stream<TargetCrashedEvent> get onTargetCrashed => _wipDebugger.eventStream(
+      'Inspector.targetCrashed',
+      (WipEvent event) => TargetCrashedEvent(event.json));
+
   @override
   Map<String, WipScript> get scripts => _wipDebugger.scripts;
 

dwds/lib/src/dwds_vm_client.dart

@@ -194,22 +194,28 @@ Future<void> _disableBreakpointsAndResume(
   if (vm.isolates.isEmpty) throw StateError('No active isolate to resume.');
   var isolateRef = vm.isolates.first;
 
-  // Pause the app to prevent it from hitting a breakpoint
-  // during hot restart and stalling hot restart execution.
-  // Then wait for the app to pause or to hit a breakpoint.
-  var debug = chromeProxyService.onEvent('Debug').firstWhere((event) =>
-      event.kind == EventKind.kPauseInterrupted ||
-      event.kind == EventKind.kPauseBreakpoint);
-
-  await client.pause(isolateRef.id);
-
-  var isolate = await client.getIsolate(isolateRef.id);
-  if (isolate.pauseEvent.kind != EventKind.kPauseInterrupted &&
-      isolate.pauseEvent.kind != EventKind.kPauseBreakpoint) {
-    await debug;
-  }
-
   await chromeProxyService.disableBreakpoints();
-  await client.resume(isolateRef.id);
+  try {
+    // Any checks for paused status result in race conditions or hangs
+    // at this point:
+    //
+    // - `getIsolate()` and check for status:
+    //    the app migth still pause on existing breakpoint.
+    //
+    // - `pause()` and wait for `Debug.paused` event:
+    //   chrome does not send the `Debug.Paused `notification
+    //   without shifting focus to chrome.
+    //
+    // Instead, just try resuming and
+    // ignore failures indicating that the app is already running:
+    //
+    // WipError -32000 Can only perform operation while paused.
+    await client.resume(isolateRef.id);
+  } on RPCError catch (e, s) {
+    if (!e.message.contains('Can only perform operation while paused')) {
+      _logger.severe('Hot restart failed to resume exiting isolate', e, s);
+      rethrow;
+    }
+  }
   _logger.info('Successfully disabled breakpoints and resumed the isolate');
 }

dwds/lib/src/handlers/dev_handler.dart

@@ -142,6 +142,9 @@ class DevHandler {
         tabConnection.onReceive.listen((message) {
           _log('  wip', '<== $message');
         });
+        tabConnection.onNotification.listen((message) {
+          _log('  wip', '<== $message');
+        });
       }
       var contextIds = tabConnection.runtime.onExecutionContextCreated
           .map((context) => context.id)

dwds/lib/src/servers/extension_debugger.dart

@@ -279,6 +279,11 @@ class ExtensionDebugger implements RemoteDebugger {
       'Debugger.scriptParsed',
       (WipEvent event) => ScriptParsedEvent(event.json));
 
+  @override
+  Stream<TargetCrashedEvent> get onTargetCrashed => eventStream(
+      'Inspector.targetCrashed',
+      (WipEvent event) => TargetCrashedEvent(event.json));
+
   @override
   Map<String, WipScript> get scripts => UnmodifiableMapView(_scripts);
 

dwds/pubspec.yaml

@@ -1,6 +1,6 @@
 name: dwds
 # Every time this changes you need to run `pub run build_runner build`.
-version: 11.2.2
+version: 11.2.3-dev
 homepage: https://github.com/dart-lang/webdev/tree/master/dwds
 description: >-
   A service that proxies between the Chrome debug protocol and the Dart VM

dwds/test/fixtures/fakes.dart

@@ -11,6 +11,7 @@ import 'package:dwds/src/debugging/execution_context.dart';
 import 'package:dwds/src/debugging/inspector.dart';
 import 'package:dwds/src/debugging/instance.dart';
 import 'package:dwds/src/debugging/modules.dart';
+import 'package:dwds/src/debugging/remote_debugger.dart';
 import 'package:dwds/src/debugging/webkit_debugger.dart';
 import 'package:dwds/src/loaders/strategy.dart';
 import 'package:dwds/src/utilities/domain.dart';
@@ -174,6 +175,9 @@ class FakeWebkitDebugger implements WebkitDebugger {
   @override
   Stream<ScriptParsedEvent> get onScriptParsed => null;
 
+  @override
+  Stream<TargetCrashedEvent> get onTargetCrashed => null;
+
   @override
   Future<WipResponse> pause() => null;