Skip to content

Find best locations for breakpoints, expression evaluation, and call frames. #1590

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

Merged
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 @@ -9,6 +9,7 @@
- Fix crash when using flutter tools with web server device.
- Remove clearing all scripts on page load for extension debugger.
- Fix breakpoints not hitting after changing a base in index.html.
- Find best locations for call frames, breakpoints, or expression evaluation.

**Breaking changes:**
- Add `basePath` parameter to `FrontendServerRequireStrategy`.
Expand Down
27 changes: 10 additions & 17 deletions dwds/lib/src/debugging/debugger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ class Debugger extends Domain {
}

/// Returns Chrome script uri for Chrome script ID.
String _urlForScriptId(String scriptId) =>
String urlForScriptId(String scriptId) =>
_remoteDebugger.scripts[scriptId]?.url;

/// Returns source [Location] for the paused event.
Expand All @@ -331,7 +331,7 @@ class Debugger extends Domain {
var line = location['lineNumber'] as int;
var column = location['columnNumber'] as int;

var url = _urlForScriptId(scriptId);
var url = urlForScriptId(scriptId);
return _locations.locationForJs(url, line, column);
}

Expand Down Expand Up @@ -467,25 +467,14 @@ class Debugger extends Domain {
var line = location.lineNumber;
var column = location.columnNumber;

var url = _urlForScriptId(location.scriptId);
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}');
return null;
}

// TODO(sdk/issues/37240) - ideally we look for an exact location instead
// of the closest location on a given line.
Location bestLocation;
for (var location in await _locations.locationsForUrl(url)) {
if (location.jsLocation.line == line) {
bestLocation ??= location;
if ((location.jsLocation.column - column).abs() <
(bestLocation.jsLocation.column - column).abs()) {
bestLocation = location;
}
}
}
var bestLocation = await _locations.locationForJs(url, line, column);
if (bestLocation == null) return null;

var script =
Expand All @@ -506,7 +495,11 @@ class Debugger extends Domain {
kind: CodeKind.kDart,
id: createId(),
),
location: SourceLocation(tokenPos: bestLocation.tokenPos, script: script),
location: SourceLocation(
line: bestLocation.dartLocation.line,
column: bestLocation.dartLocation.column,
tokenPos: bestLocation.tokenPos,
script: script),
kind: FrameKind.kRegular,
);

Expand Down Expand Up @@ -579,7 +572,7 @@ class Debugger extends Domain {
var frame = e.params['callFrames'][0];
var scriptId = '${frame["location"]["scriptId"]}';

var url = _urlForScriptId(scriptId);
var url = urlForScriptId(scriptId);
if (url == null) {
logger.severe('Stepping failed: '
'cannot find location for script $scriptId');
Expand Down
20 changes: 12 additions & 8 deletions dwds/lib/src/debugging/frame_computer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,24 @@

// @dart = 2.9

import 'package:logging/logging.dart';
import 'package:pool/pool.dart';
import 'package:vm_service/vm_service.dart';
import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';

import 'dart_scope.dart';
import 'debugger.dart';

class FrameComputer {
final Debugger debugger;

final _logger = Logger('FrameComputer');

// To ensure that the frames are computed only once, we use a pool to guard
// the work. Frames are computed sequentially.
final _pool = Pool(1);

final List<WipCallFrame> _callFrames;
final _computedFrames = <Frame>[];
final List<Frame> _computedFrames = [];

var _frameIndex = 0;

Expand All @@ -36,12 +38,6 @@ class FrameComputer {
return frameIndex < _callFrames.length ? _callFrames[frameIndex] : null;
}

/// Return the WipScopes for the given JavaScript frame index that are
/// pertinent for Dart debugging.
List<WipScope> getWipScopesForFrameIndex(int frameIndex) {
return filterScopes(jsFrameForIndex(frameIndex));
}

/// Translates Chrome callFrames contained in [DebuggerPausedEvent] into Dart
/// [Frame]s.
Future<List<Frame>> calculateFrames({int limit}) async {
Expand Down Expand Up @@ -101,6 +97,14 @@ class FrameComputer {
var location = WipLocation.fromValues(
callFrame.scriptId, callFrame.lineNumber,
columnNumber: callFrame.columnNumber);

var url = callFrame.url ?? debugger.urlForScriptId(location.scriptId);
if (url == null) {
_logger.severe(
'Failed to create dart frame for ${callFrame.functionName}: '
'cannot find location for script ${callFrame.scriptId}');
}

var tempWipFrame = WipCallFrame({
'url': callFrame.url,
'functionName': callFrame.functionName,
Expand Down
115 changes: 103 additions & 12 deletions dwds/lib/src/debugging/location.dart
Original file line number Diff line number Diff line change
Expand Up @@ -152,22 +152,113 @@ class Locations {
/// Find the [Location] for the given Dart source position.
///
/// The [line] number is 1-based.
Future<Location> locationForDart(DartUri uri, int line, int column) async =>
(await locationsForDart(uri.serverPath)).firstWhere(
(location) =>
location.dartLocation.line == line &&
location.dartLocation.column >= column,
orElse: () => null);
Future<Location> locationForDart(DartUri uri, int line, int column) async {
var locations = await locationsForDart(uri.serverPath);
return _bestDartLocation(locations, line, column);
}

/// Find the [Location] for the given JS source position.
///
/// The [line] number is 0-based.
Future<Location> locationForJs(String url, int line, int column) async =>
(await locationsForUrl(url)).firstWhere(
(location) =>
location.jsLocation.line == line &&
location.jsLocation.column >= column,
orElse: () => null);
Future<Location> locationForJs(String url, int line, int column) async {
var locations = await locationsForUrl(url);
return _bestJsLocation(locations, line, column);
}

/// Find closest existing Dart location for the line and column.
///
/// Dart columns for breakpoints are either exact or start at the
/// beginning of the line - return the first existing location
/// that comes after the given column.
Location _bestDartLocation(
Iterable<Location> locations, int line, int column) {
Location bestLocation;
for (var location in locations) {
if (location.dartLocation.line == line &&
location.dartLocation.column >= column) {
bestLocation ??= location;
if (location.dartLocation.column < bestLocation.dartLocation.column) {
bestLocation = location;
}
}
}
return bestLocation;
}

/// 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.
///
/// 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;
}
}
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;
}

/// Returns the tokenPosTable for the provided Dart script path as defined
/// in:
Expand Down
2 changes: 1 addition & 1 deletion dwds/lib/src/debugging/metadata/provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class MetadataProvider {
_soundNullSafety = hasSoundNullSafety;
}
_logger.info('Loaded debug metadata '
'(${_soundNullSafety ? "" : "no "}sound null safety)');
'(${_soundNullSafety ? "sound" : "weak"} null safety)');
}
});
}
Expand Down
10 changes: 3 additions & 7 deletions dwds/lib/src/services/expression_evaluator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,9 @@ class ExpressionEvaluator {
var jsFrame = _inspector.debugger.jsFrameForIndex(frameIndex);
if (jsFrame == null) {
return _createError(
ErrorKind.internal, 'No frame with index $frameIndex');
ErrorKind.internal,
'Expression evaluation in async frames '
'is not supported. No frame with index $frameIndex.');
}

var functionName = jsFrame.functionName;
Expand All @@ -165,12 +167,6 @@ class ExpressionEvaluator {
var jsScope = await _collectLocalJsScope(jsFrame);

// Find corresponding dart location and scope.
//
// TODO(annagrin): handle unknown dart locations
// Debugger does not map every js location to a dart location,
// so this will result in expressions not evaluated in some
// cases. Invent location matching strategy for those cases.
// [issue 890](https://github.com/dart-lang/webdev/issues/890)
var url = _urlForScriptId(jsScriptId);
var locationMap = await _locations.locationForJs(url, jsLine, jsColumn);
if (locationMap == null) {
Expand Down
22 changes: 0 additions & 22 deletions dwds/test/build_daemon_breakpoint_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -74,28 +74,6 @@ void main() {
await service.removeBreakpoint(isolate.id, bp.id);
});

test('set breakpoint inside a JavaScript line succeeds on', () async {
var line = await context.findBreakpointLine(
'printNestedObjectMultiLine', isolate.id, mainScript);
var column = 0;
var bp = await service.addBreakpointWithScriptUri(
isolate.id, mainScript.uri, line,
column: column);
await stream.firstWhere(
(Event event) => event.kind == EventKind.kPauseBreakpoint);

expect(bp, isNotNull);
expect(
bp.location,
isA<SourceLocation>()
.having((loc) => loc.script, 'script', equals(mainScript))
.having((loc) => loc.line, 'line', equals(line))
.having((loc) => loc.column, 'column', greaterThan(column)));

// Remove breakpoint so it doesn't impact other tests.
await service.removeBreakpoint(isolate.id, bp.id);
});

test('set breakpoint again', () async {
var line = await context.findBreakpointLine(
'printLocal', isolate.id, mainScript);
Expand Down
Loading