Fix slow stepping into with deferred loaded modules (#1593)

* Fix slow stepping into with deferred loaded modules

* Only consider previous JS location for sourcemap matching

* Updated tests

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -11,9 +11,19 @@
 - Fix breakpoints not hitting after changing a base in index.html.
 - Find best locations for call frames, breakpoints, or expression evaluation.
 - Close the SSE connection when a DebugExtension.detached event is received.
+- Fix issues discovered when using legacy module system, debug extension,
+  and JIT modules:
+  - Improve step-into times by not stepping into library loading code.
+  - Fix incorrect skip lists due to unsorted locations.
+  - Fix memory leak in extension debugger by removing stale script IDs.
+  - Allow mapping JS locations to Dart locations matching other JS lines,
+    to match the behavior of Chrome DevTools.
+  - Fix expression evaluation failure if debugger is stopped in the middle
+    of a variable definition.
 
 **Breaking changes:**
 - Add `basePath` parameter to `FrontendServerRequireStrategy`.
+- Add `loadLibrariesModule` getter to `LoadStrategy` interface.
 
 ## 13.1.0
 - Update _fe_analyzer_shared to version ^38.0.0.

dwds/lib/src/debugging/debugger.dart

@@ -332,9 +332,16 @@ class Debugger extends Domain {
     var column = location['columnNumber'] as int;
 
     var url = urlForScriptId(scriptId);
+    if (url == null) return null;
     return _locations.locationForJs(url, line, column);
   }
 
+  /// Returns script ID for the paused event.
+  String _frameScriptId(DebuggerPausedEvent e) {
+    var frame = e.params['callFrames'][0];
+    return frame['location']['scriptId'] as String;
+  }
+
   /// The variables visible in a frame in Dart protocol [BoundVariable] form.
   Future<List<BoundVariable>> variablesFor(WipCallFrame frame) async {
     // TODO(alanknight): Can these be moved to dart_scope.dart?
@@ -470,7 +477,7 @@ class Debugger extends Domain {
     var url = urlForScriptId(location.scriptId);
     if (url == null) {
       logger.severe('Failed to create dart frame for ${frame.functionName}: '
-          'cannot find location for script ${location.scriptId}');
+          'cannot find url for script ${location.scriptId}');
       return null;
     }
 
@@ -567,26 +574,32 @@ class Debugger extends Domain {
         exception: exception,
       );
     } else {
-      // If we don't have source location continue stepping.
-      if (_isStepping && (await _sourceLocation(e)) == null) {
-        var frame = e.params['callFrames'][0];
-        var scriptId = '${frame["location"]["scriptId"]}';
-
+      // Continue stepping until we hit a dart location,
+      // avoiding stepping through library loading code.
+      if (_isStepping) {
+        var scriptId = _frameScriptId(e);
         var url = urlForScriptId(scriptId);
+
         if (url == null) {
           logger.severe('Stepping failed: '
-              'cannot find location for script $scriptId');
+              'cannot find url for script $scriptId');
+          throw StateError('Stepping failed in script $scriptId');
         }
 
-        // TODO(grouma) - In the future we should send all previously computed
-        // skipLists.
-        await _remoteDebugger.stepInto(params: {
-          'skipList': await _skipLists.compute(
-            scriptId,
-            await _locations.locationsForUrl(url),
-          )
-        });
-        return;
+        if (url.contains(globalLoadStrategy.loadLibrariesModule)) {
+          await _remoteDebugger.stepOut();
+          return;
+        } else if ((await _sourceLocation(e)) == null) {
+          // TODO(grouma) - In the future we should send all previously computed
+          // skipLists.
+          await _remoteDebugger.stepInto(params: {
+            'skipList': await _skipLists.compute(
+              scriptId,
+              await _locations.locationsForUrl(url),
+            )
+          });
+          return;
+        }
       }
       event = Event(
           kind: EventKind.kPauseInterrupted,

dwds/lib/src/debugging/location.dart

@@ -69,6 +69,13 @@ class DartLocation {
     this.column,
   );
 
+  int compareTo(DartLocation other) => compareToLine(other.line, other.column);
+
+  int compareToLine(int otherLine, int otherColumn) {
+    var result = line.compareTo(otherLine);
+    return result == 0 ? column.compareTo(otherColumn) : result;
+  }
+
   @override
   String toString() => '[${uri.serverPath}:$line:$column]';
 
@@ -92,6 +99,13 @@ class JsLocation {
     this.column,
   );
 
+  int compareTo(JsLocation other) => compareToLine(other.line, other.column);
+
+  int compareToLine(int otherLine, int otherColumn) {
+    var result = line.compareTo(otherLine);
+    return result == 0 ? column.compareTo(otherColumn) : result;
+  }
+
   @override
   String toString() => '[$module:$line:$column]';
 
@@ -177,7 +191,7 @@ class Locations {
       if (location.dartLocation.line == line &&
           location.dartLocation.column >= column) {
         bestLocation ??= location;
-        if (location.dartLocation.column < bestLocation.dartLocation.column) {
+        if (location.dartLocation.compareTo(bestLocation.dartLocation) < 0) {
           bestLocation = location;
         }
       }
@@ -188,76 +202,23 @@ class Locations {
   /// Find closest existing JavaScript location for the line and column.
   ///
   /// Some JS locations are not stored in the source maps, so we find the
-  /// closest existing location, preferring the one coming after the given
-  /// column, if the current break is at an expression statement, or the
-  /// one coming before if the current break is at a function call.
+  /// closest existing location coming before the given column.
   ///
   /// This is a known problem that other code bases solve using by finding
   /// the closest location to the current one:
   ///
   /// https://github.com/microsoft/vscode-js-debug/blob/536f96bae61a3d87546b61bc7916097904c81429/src/common/sourceUtils.ts#L286
-  ///
-  /// Unfortunately, this approach fails for Flutter code too often, as it
-  /// frequently contains multi-line statements with nested objects.
-  ///
-  /// For example:
-  ///
-  ///   - `t33 = main.doSomething()` in top frame:
-  ///      Current column is at `t33`. Return existing location starting
-  ///      at `main`.
-  ///   - `main.doSomething()` in top frame:
-  ///      Current column is at `main`. Return existing location starting
-  ///      at `main`.
-  ///   - `main.doSomething()` in a frame down the stack:
-  ///      Current column is at `doSomething`. Source map does not have a
-  ///      location stored that starts at `doSomething()`. Return existing
-  ///      location starting at `main`.
   Location _bestJsLocation(Iterable<Location> locations, int line, int column) {
-    Location bestLocationBefore;
-    Location bestLocationAfter;
-
-    var locationsAfter = locations.where((location) =>
-        location.jsLocation.line == line &&
-        location.jsLocation.column >= column);
-    var locationsBefore = locations.where((location) =>
-        location.jsLocation.line == line &&
-        location.jsLocation.column < column);
-
-    for (var location in locationsAfter) {
-      bestLocationAfter ??= location;
-      if (location.jsLocation.column < bestLocationAfter.jsLocation.column) {
-        bestLocationAfter = location;
-      }
-    }
-    for (var location in locationsBefore) {
-      bestLocationBefore ??= location;
-      if (location.jsLocation.column > bestLocationBefore.jsLocation.column) {
-        bestLocationBefore = location;
+    Location bestLocation;
+    for (var location in locations) {
+      if (location.jsLocation.compareToLine(line, column) <= 0) {
+        bestLocation ??= location;
+        if (location.jsLocation.compareTo(bestLocation.jsLocation) > 0) {
+          bestLocation = location;
+        }
       }
     }
-    if (bestLocationAfter == null) return bestLocationBefore;
-    if (bestLocationBefore == null) return bestLocationAfter;
-
-    if (bestLocationAfter.jsLocation.line == line &&
-        bestLocationAfter.jsLocation.column == column) {
-      // Prefer exact match.
-      return bestLocationAfter;
-    }
-
-    // Return the closest location after the current if the current location
-    // is at the beginning of the line (i.e. on expression statement).
-    // Return the closest location before the current if the current location
-    // is in the middle of the line (i.e. on function call).
-    if (locationsBefore.length == 1 &&
-        locationsBefore.first.jsLocation.column == 0) {
-      // Best guess on whether the the current location is at the beginning of
-      // the line (i.e. expression statement):
-      // The only location on the left has column 0, so only spaces are on the
-      // left.
-      return bestLocationAfter;
-    }
-    // Current column is in the middle of the line (i.e .function call).
-    return bestLocationBefore;
+    return bestLocation;
   }
 
   /// Returns the tokenPosTable for the provided Dart script path as defined

dwds/lib/src/debugging/skip_list.dart

@@ -26,11 +26,13 @@ class SkipLists {
   ) async {
     if (_idToList.containsKey(scriptId)) return _idToList[scriptId];
 
+    var sortedLocations = locations.toList()
+      ..sort((a, b) => a.jsLocation.compareTo(b.jsLocation));
+
     var ranges = <Map<String, dynamic>>[];
     var startLine = 0;
     var startColumn = 0;
-    for (var location in locations) {
-      // Account for 1 based.
+    for (var location in sortedLocations) {
       var endLine = location.jsLocation.line;
       var endColumn = location.jsLocation.column;
       // Stop before the known location.
@@ -40,8 +42,10 @@ class SkipLists {
         endColumn = maxValue;
       }
       if (endLine > startLine || endColumn > startColumn) {
-        ranges.add(
-            _rangeFor(scriptId, startLine, startColumn, endLine, endColumn));
+        if (endLine >= startLine) {
+          ranges.add(
+              _rangeFor(scriptId, startLine, startColumn, endLine, endColumn));
+        }
         startLine = location.jsLocation.line;
         startColumn = location.jsLocation.column + 1;
       }

dwds/lib/src/loaders/legacy.dart

@@ -79,6 +79,9 @@ class LegacyStrategy extends LoadStrategy {
   @override
   String get moduleFormat => 'ddc';
 
+  @override
+  String get loadLibrariesModule => 'dart_library.ddk.js';
+
   @override
   String get loadLibrariesSnippet =>
       'for(let module of dart_library.libraries()) {\n'

dwds/lib/src/loaders/require.dart

@@ -169,6 +169,9 @@ class RequireStrategy extends LoadStrategy {
   @override
   String get moduleFormat => 'amd';
 
+  @override
+  String get loadLibrariesModule => 'require.js';
+
   @override
   String get loadLibrariesSnippet =>
       'let libs = $loadModuleSnippet("dart_sdk").dart.getLibraries();\n';

dwds/lib/src/loaders/strategy.dart

@@ -36,6 +36,11 @@ abstract class LoadStrategy {
   /// expression evaluation.
   String get moduleFormat;
 
+  /// Module containing code for loading libraries.
+  ///
+  /// Used for preventing stepping into the library loading code.
+  String get loadLibrariesModule;
+
   /// Returns a snippet of JS code that loads all Dart libraries into a `libs`
   /// variable.
   String get loadLibrariesSnippet;

dwds/lib/src/servers/extension_debugger.dart

@@ -64,6 +64,7 @@ class ExtensionDebugger implements RemoteDebugger {
       (WipEvent event) => ExceptionThrownEvent(event.json));
 
   final _scripts = <String, WipScript>{};
+  final _scriptIds = <String, String>{};
 
   ExtensionDebugger(this.sseConnection) {
     sseConnection.stream.listen((data) {
@@ -113,7 +114,13 @@ class ExtensionDebugger implements RemoteDebugger {
       close();
     }, onDone: close);
     onScriptParsed.listen((event) {
+      // Remove stale scripts from cache.
+      if (event.script.url.isNotEmpty &&
+          _scriptIds.containsKey(event.script.url)) {
+        _scripts.remove(_scriptIds[event.script.url]);
+      }
       _scripts[event.script.scriptId] = event.script;
+      _scriptIds[event.script.url] = event.script.scriptId;
     });
     // Listens for a page reload.
     onGlobalObjectCleared.listen((_) {

dwds/lib/src/services/expression_evaluator.dart

@@ -229,6 +229,8 @@ class ExpressionEvaluator {
     return ret;
   }
 
+  bool _isUndefined(RemoteObject value) => value.type == 'undefined';
+
   RemoteObject _formatCompilationError(String error) {
     // Frontend currently gives a text message including library name
     // and function name on compilation error. Strip this information
@@ -284,6 +286,7 @@ class ExpressionEvaluator {
       for (var p in variables) {
         var name = p.name;
         var value = p.value;
+        if (_isUndefined(value)) continue;
 
         if (scopeType == 'closure') {
           // Substitute potentially unavailable captures with their values from