Fix hot restart and breakpoints races (#1384)

* Fix hot restart and breakpoints races

Multiple breakpoints restored ad the isolate start can cause
the app to crash in chrome, serialize them using a mutex to
prevent the crash.

Hot restart runs main immediately, followed by createIsolate
that restores breakpoints, sometimes too late, causing non-
removable breakpoints and failures in the UI. Make the dwds
run main after createIsolate is done, to make sure breakpoints
are all set before the run.

Hot restart tries to resume the exiting isolate, and checks if
it resumed before that, which causes various races or chrome
requiring to be in focus, depending on the way the check is done.
Remove the check and just catch exception in case the isolate
is already running.

* Use local dwds in webdev, update changelog

* Rebase, update version and build

* Increase timeout for failing tests

* Update frontend_sever_client_test.dart

revert timeout on frontend_server_client tests, it is fixed in another PR.

* Remove no running main on hot restart

* Addressed CR comments

Author: Anna Gringauze

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/lib/src/version.dart

@@ -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/pubspec.yaml

@@ -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;