Do not persist breakpoints across hot restarts or page refreshes

dwds/CHANGELOG.md

@@ -2,6 +2,7 @@
 
 - Rename `dart_library.js` to `ddc_module_loader.js` to match SDK naming changes. - [#2360](https://github.com/dart-lang/webdev/pull/2360)
 - Implement `setFlag` when it is called with `pause_isolates_on_start`. - [#2373](https://github.com/dart-lang/webdev/pull/2373)
+- Do not persist breakpoints across hot restarts or page reloads. - [#2371](https://github.com/dart-lang/webdev/pull/2371)
 
 ## 23.3.0
 

dwds/lib/src/debugging/debugger.dart

@@ -18,6 +18,7 @@ import 'package:dwds/src/utilities/server.dart';
 import 'package:dwds/src/utilities/shared.dart';
 import 'package:dwds/src/utilities/synchronized.dart';
 import 'package:logging/logging.dart';
+import 'package:path/path.dart' as p;
 import 'package:vm_service/vm_service.dart';
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'
     hide StackTrace;
@@ -36,6 +37,14 @@ const _pauseModePauseStates = {
   'unhandled': PauseState.uncaught,
 };
 
+/// Mapping from the path of a script in Chrome to the Runtime.ScriptId Chrome
+/// uses to reference it.
+///
+/// See https://chromedevtools.github.io/devtools-protocol/tot/Runtime/#type-ScriptId
+///
+/// e.g. 'packages/myapp/main.dart.lib.js' -> '12'
+final chromePathToRuntimeScriptId = <String, String>{};
+
 class Debugger extends Domain {
   static final logger = Logger('Debugger');
 
@@ -44,18 +53,17 @@ class Debugger extends Domain {
   final StreamNotify _streamNotify;
   final Locations _locations;
   final SkipLists _skipLists;
-  final String _root;
 
   Debugger._(
     this._remoteDebugger,
     this._streamNotify,
     this._locations,
     this._skipLists,
-    this._root,
+    root,
   ) : _breakpoints = _Breakpoints(
           locations: _locations,
           remoteDebugger: _remoteDebugger,
-          root: _root,
+          root: root,
         );
 
   /// The breakpoints we have set so far, indexable by either
@@ -207,6 +215,7 @@ class Debugger extends Domain {
     // miss events.
     // Allow a null debugger/connection for unit tests.
     runZonedGuarded(() {
+      _remoteDebugger.onScriptParsed.listen(_scriptParsedHandler);
       _remoteDebugger.onPaused.listen(_pauseHandler);
       _remoteDebugger.onResumed.listen(_resumeHandler);
       _remoteDebugger.onTargetCrashed.listen(_crashHandler);
@@ -253,67 +262,6 @@ class Debugger extends Domain {
     return breakpoint;
   }
 
-  Future<ScriptRef?> _updatedScriptRefFor(Breakpoint breakpoint) async {
-    final oldRef = (breakpoint.location as SourceLocation).script;
-    final uri = oldRef?.uri;
-    if (uri == null) return null;
-    final dartUri = DartUri(uri, _root);
-    return await inspector.scriptRefFor(dartUri.serverPath);
-  }
-
-  Future<void> reestablishBreakpoints(
-    Set<Breakpoint> previousBreakpoints,
-    Set<Breakpoint> disabledBreakpoints,
-  ) async {
-    // Previous breakpoints were never removed from Chrome since we use
-    // `setBreakpointByUrl`. We simply need to update the references.
-    for (var breakpoint in previousBreakpoints) {
-      final dartBpId = breakpoint.id!;
-      final scriptRef = await _updatedScriptRefFor(breakpoint);
-      final scriptUri = scriptRef?.uri;
-      if (scriptRef != null && scriptUri != null) {
-        final jsBpId = _breakpoints.jsIdFor(dartBpId)!;
-        final updatedLocation = await _locations.locationForDart(
-          DartUri(scriptUri, _root),
-          _lineNumberFor(breakpoint),
-          _columnNumberFor(breakpoint),
-        );
-        if (updatedLocation != null) {
-          final updatedBreakpoint = _breakpoints._dartBreakpoint(
-            scriptRef,
-            updatedLocation,
-            dartBpId,
-          );
-          _breakpoints._note(bp: updatedBreakpoint, jsId: jsBpId);
-          _notifyBreakpoint(updatedBreakpoint);
-        } else {
-          logger.warning('Cannot update breakpoint $dartBpId:'
-              ' cannot update location.');
-        }
-      } else {
-        logger.warning('Cannot update breakpoint $dartBpId:'
-            ' cannot find script ref.');
-      }
-    }
-
-    // Disabled breakpoints were actually removed from Chrome so simply add
-    // them back.
-    for (var breakpoint in disabledBreakpoints) {
-      final scriptRef = await _updatedScriptRefFor(breakpoint);
-      final scriptId = scriptRef?.id;
-      if (scriptId != null) {
-        await addBreakpoint(
-          scriptId,
-          _lineNumberFor(breakpoint),
-          column: _columnNumberFor(breakpoint),
-        );
-      } else {
-        logger.warning('Cannot update disabled breakpoint ${breakpoint.id}:'
-            ' cannot find script ref.');
-      }
-    }
-  }
-
   void _notifyBreakpoint(Breakpoint breakpoint) {
     final event = Event(
       kind: EventKind.kBreakpointAdded,
@@ -520,6 +468,31 @@ class Debugger extends Domain {
     return dartFrame;
   }
 
+  void _scriptParsedHandler(ScriptParsedEvent e) {
+    final scriptPath = _pathForChromeScript(e.script.url);
+    if (scriptPath != null) {
+      chromePathToRuntimeScriptId[scriptPath] = e.script.scriptId;
+    }
+  }
+
+  String? _pathForChromeScript(String scriptUrl) {
+    final scriptPathSegments = Uri.parse(scriptUrl).pathSegments;
+    if (scriptPathSegments.isEmpty) {
+      return null;
+    }
+
+    final isInternal = globalToolConfiguration.appMetadata.isInternalBuild;
+    const packagesDir = 'packages';
+    if (isInternal && scriptUrl.contains(packagesDir)) {
+      final packagesIdx = scriptPathSegments.indexOf(packagesDir);
+      return p.joinAll(scriptPathSegments.sublist(packagesIdx));
+    }
+
+    // Note: Replacing "\" with "/" is necessary because `joinAll` uses "\" if
+    // the platform is Windows. However, only "/" is expected by the browser.
+    return p.joinAll(scriptPathSegments).replaceAll('\\', '/');
+  }
+
   /// Handles pause events coming from the Chrome connection.
   Future<void> _pauseHandler(DebuggerPausedEvent e) async {
     final isolate = inspector.isolate;
@@ -738,14 +711,6 @@ Future<T> sendCommandAndValidateResult<T>(
   return result;
 }
 
-/// Returns the Dart line number for the provided breakpoint.
-int _lineNumberFor(Breakpoint breakpoint) =>
-    int.parse(breakpoint.id!.split('#').last.split(':').first);
-
-/// Returns the Dart column number for the provided breakpoint.
-int _columnNumberFor(Breakpoint breakpoint) =>
-    int.parse(breakpoint.id!.split('#').last.split(':').last);
-
 /// Returns the breakpoint ID for the provided Dart script ID and Dart line
 /// number.
 String breakpointIdFor(String scriptId, int line, int column) =>
@@ -779,8 +744,10 @@ class _Breakpoints extends Domain {
     int line,
     int column,
   ) async {
+    print('creating breakpoint at $scriptId:$line:$column)');
     final dartScript = inspector.scriptWithId(scriptId);
     final dartScriptUri = dartScript?.uri;
+    print('dart script uri is $dartScriptUri');
     Location? location;
     if (dartScriptUri != null) {
       final dartUri = DartUri(dartScriptUri, root);
@@ -853,22 +820,27 @@ class _Breakpoints extends Domain {
 
   /// Calls the Chrome protocol setBreakpoint and returns the remote ID.
   Future<String?> _setJsBreakpoint(Location location) {
-    // The module can be loaded from a nested path and contain an ETAG suffix.
-    final urlRegex = '.*${location.jsLocation.module}.*';
     // Prevent `Aww, snap!` errors when setting multiple breakpoints
     // simultaneously by serializing the requests.
     return _queue.run(() async {
-      final breakPointId = await sendCommandAndValidateResult<String>(
-        remoteDebugger,
-        method: 'Debugger.setBreakpointByUrl',
-        resultField: 'breakpointId',
-        params: {
-          'urlRegex': urlRegex,
-          'lineNumber': location.jsLocation.line,
-          'columnNumber': location.jsLocation.column,
-        },
-      );
-      return breakPointId;
+      final scriptId = location.jsLocation.runtimeScriptId;
+      if (scriptId != null) {
+        return sendCommandAndValidateResult<String>(
+          remoteDebugger,
+          method: 'Debugger.setBreakpoint',
+          resultField: 'breakpointId',
+          params: {
+            'location': {
+              'lineNumber': location.jsLocation.line,
+              'columnNumber': location.jsLocation.column,
+              'scriptId': scriptId,
+            },
+          },
+        );
+      } else {
+        _logger.fine('No runtime script ID for location $location');
+        return null;
+      }
     });
   }
 

dwds/lib/src/debugging/location.dart

@@ -33,6 +33,7 @@ class Location {
     TargetLineEntry lineEntry,
     TargetEntry entry,
     DartUri dartUri,
+    String? runtimeScriptId,
   ) {
     final dartLine = entry.sourceLine;
     final dartColumn = entry.sourceColumn;
@@ -42,7 +43,7 @@ class Location {
     // lineEntry data is 0 based according to:
     // https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k
     return Location._(
-      JsLocation.fromZeroBased(module, jsLine, jsColumn),
+      JsLocation.fromZeroBased(module, jsLine, jsColumn, runtimeScriptId),
       DartLocation.fromZeroBased(dartUri, dartLine ?? 0, dartColumn ?? 0),
     );
   }
@@ -104,10 +105,16 @@ class JsLocation {
   /// 0 based column offset within the JS source code.
   final int column;
 
+  /// The Runtime.ScriptId of a script in Chrome.
+  ///
+  /// See https://chromedevtools.github.io/devtools-protocol/tot/Runtime/#type-ScriptId
+  String? runtimeScriptId;
+
   JsLocation._(
     this.module,
     this.line,
     this.column,
+    this.runtimeScriptId,
   );
 
   int compareTo(JsLocation other) => compareToLine(other.line, other.column);
@@ -122,8 +129,13 @@ class JsLocation {
 
   // JS Location is 0 based according to:
   // https://chromedevtools.github.io/devtools-protocol/tot/Debugger#type-Location
-  factory JsLocation.fromZeroBased(String module, int line, int column) =>
-      JsLocation._(module, line, column);
+  factory JsLocation.fromZeroBased(
+    String module,
+    int line,
+    int column,
+    String? runtimeScriptId,
+  ) =>
+      JsLocation._(module, line, column, runtimeScriptId);
 }
 
 /// Contains meta data for known [Location]s.
@@ -321,6 +333,10 @@ class Locations {
           p.url.dirname('/${stripLeadingSlashes(modulePath)}');
 
       if (sourceMapContents == null) return result;
+
+      final runtimeScriptId =
+          await _modules.getRuntimeScriptIdForModule(_entrypoint, module);
+
       // This happens to be a [SingleMapping] today in DDC.
       final mapping = parse(sourceMapContents);
       if (mapping is SingleMapping) {
@@ -339,12 +355,14 @@ class Locations {
             );
 
             final dartUri = DartUri(path, _root);
+
             result.add(
               Location.from(
                 modulePath,
                 lineEntry,
                 entry,
                 dartUri,
+                runtimeScriptId,
               ),
             );
           }

dwds/lib/src/debugging/modules.dart

@@ -4,6 +4,7 @@
 
 import 'package:async/async.dart';
 import 'package:dwds/src/config/tool_configuration.dart';
+import 'package:dwds/src/debugging/debugger.dart';
 import 'package:dwds/src/utilities/dart_uri.dart';
 import 'package:logging/logging.dart';
 
@@ -62,6 +63,15 @@ class Modules {
     return _sourceToModule;
   }
 
+  Future<String?> getRuntimeScriptIdForModule(
+    String entrypoint,
+    String module,
+  ) async {
+    final serverPath = await globalToolConfiguration.loadStrategy
+        .serverPathForModule(entrypoint, module);
+    return chromePathToRuntimeScriptId[serverPath];
+  }
+
   /// Initializes [_sourceToModule] and [_sourceToLibrary].
   Future<void> _initializeMapping() async {
     final provider =

dwds/lib/src/services/chrome_proxy_service.dart

@@ -99,9 +99,6 @@ class ChromeProxyService implements VmServiceInterface {
   Stream<bool> get pauseIsolatesOnStartStream =>
       _pauseIsolatesOnStartController.stream;
 
-  final _disabledBreakpoints = <Breakpoint>{};
-  final _previousBreakpoints = <Breakpoint>{};
-
   final _logger = Logger('ChromeProxyService');
 
   final ExpressionCompiler? _compiler;
@@ -295,12 +292,6 @@ class ChromeProxyService implements VmServiceInterface {
 
     safeUnawaited(_prewarmExpressionCompilerCache());
 
-    await debugger.reestablishBreakpoints(
-      _previousBreakpoints,
-      _disabledBreakpoints,
-    );
-    _disabledBreakpoints.clear();
-
     safeUnawaited(
       appConnection.onStart.then((_) {
         debugger.resumeFromStart();
@@ -376,19 +367,15 @@ class ChromeProxyService implements VmServiceInterface {
     );
     _vm.isolates?.removeWhere((ref) => ref.id == isolate.id);
     _inspector = null;
-    _previousBreakpoints.clear();
-    _previousBreakpoints.addAll(isolate.breakpoints ?? []);
     _expressionEvaluator?.close();
     _consoleSubscription?.cancel();
     _consoleSubscription = null;
   }
 
   Future<void> disableBreakpoints() async {
-    _disabledBreakpoints.clear();
     if (!_isIsolateRunning) return;
     final isolate = inspector.isolate;
 
-    _disabledBreakpoints.addAll(isolate.breakpoints ?? []);
     for (var breakpoint in isolate.breakpoints?.toList() ?? []) {
       await (await debuggerFuture).removeBreakpoint(breakpoint.id);
     }
@@ -1121,8 +1108,6 @@ ${globalToolConfiguration.loadStrategy.loadModuleSnippet}("dart_sdk").developer.
   ) async {
     await isInitialized;
     _checkIsolate('removeBreakpoint', isolateId);
-    _disabledBreakpoints
-        .removeWhere((breakpoint) => breakpoint.id == breakpointId);
     return (await debuggerFuture).removeBreakpoint(breakpointId);
   }
 

dwds/test/fixtures/fakes.dart

@@ -169,6 +169,13 @@ class FakeModules implements Modules {
 
   @override
   Future<String> moduleForLibrary(String libraryUri) async => _module;
+
+  @override
+  Future<String?> getRuntimeScriptIdForModule(
+    String entrypoint,
+    String module,
+  ) async =>
+      null;
 }
 
 class FakeWebkitDebugger implements WebkitDebugger {