Skip to content

Commit

Permalink
Do not persist breakpoints across hot restarts or page refreshes (#2371)
Browse files Browse the repository at this point in the history
  • Loading branch information
elliette authored Feb 26, 2024
1 parent 51b5484 commit c29e5f8
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 291 deletions.
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
142 changes: 57 additions & 85 deletions dwds/lib/src/debugging/debugger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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');

Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) =>
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
});
}

Expand Down
24 changes: 21 additions & 3 deletions dwds/lib/src/debugging/location.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class Location {
TargetLineEntry lineEntry,
TargetEntry entry,
DartUri dartUri,
String? runtimeScriptId,
) {
final dartLine = entry.sourceLine;
final dartColumn = entry.sourceColumn;
Expand All @@ -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),
);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -339,12 +355,14 @@ class Locations {
);

final dartUri = DartUri(path, _root);

result.add(
Location.from(
modulePath,
lineEntry,
entry,
dartUri,
runtimeScriptId,
),
);
}
Expand Down
10 changes: 10 additions & 0 deletions dwds/lib/src/debugging/modules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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 =
Expand Down
15 changes: 0 additions & 15 deletions dwds/lib/src/services/chrome_proxy_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -295,12 +292,6 @@ class ChromeProxyService implements VmServiceInterface {

safeUnawaited(_prewarmExpressionCompilerCache());

await debugger.reestablishBreakpoints(
_previousBreakpoints,
_disabledBreakpoints,
);
_disabledBreakpoints.clear();

safeUnawaited(
appConnection.onStart.then((_) {
debugger.resumeFromStart();
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}

Expand Down
7 changes: 7 additions & 0 deletions dwds/test/fixtures/fakes.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit c29e5f8

Please sign in to comment.