Re-establish breakpoints on hot-restart (#7205)
diff --git a/packages/devtools_app/integration_test/test/live_connection/debugger_panel_test.dart b/packages/devtools_app/integration_test/test/live_connection/debugger_panel_test.dart index 360a8b1..b524563 100644 --- a/packages/devtools_app/integration_test/test/live_connection/debugger_panel_test.dart +++ b/packages/devtools_app/integration_test/test/live_connection/debugger_panel_test.dart
@@ -3,8 +3,10 @@ // found in the LICENSE file. import 'package:devtools_app/devtools_app.dart'; +import 'package:devtools_app/src/screens/debugger/breakpoints.dart'; import 'package:devtools_app/src/screens/debugger/call_stack.dart'; import 'package:devtools_app/src/screens/debugger/codeview.dart'; +import 'package:devtools_app/src/service/service_extension_widgets.dart'; import 'package:devtools_test/helpers.dart'; import 'package:devtools_test/integration_test.dart'; import 'package:flutter/material.dart'; @@ -59,27 +61,44 @@ isTrue, ); - logStatus('opening the "more" menu'); + logStatus('Navigating to line 57...'); - final moreMenuFinder = find.byType(PopupMenuButton<ScriptPopupMenuOption>); - expect(moreMenuFinder, findsOneWidget); - await tester.tap(moreMenuFinder); - await tester.pumpAndSettle(safePumpDuration); + await goToLine(tester, lineNumber: 57); - logStatus('selecting the go-to-line menu option'); + logStatus('looking for line 57'); - final goToLineOptionFinder = find.textContaining('Go to line number'); - expect(goToLineOptionFinder, findsOneWidget); - await tester.tap(goToLineOptionFinder); - await tester.pumpAndSettle(safePumpDuration); + // Look for the line 57 gutter item: + final gutter57Finder = findGutterItemWithText('57'); + expect(gutter57Finder, findsOneWidget); - logStatus('entering line number in the go-to-line dialog'); + // Look for the line 57 line item: + final line57Finder = findLineItemWithText("print('Hello!');"); + expect(line57Finder, findsOneWidget); - final goToLineInputFinder = find.widgetWithText(TextField, 'Line Number'); - expect(goToLineInputFinder, findsOneWidget); - await tester.enterText(goToLineInputFinder, '30'); - await tester.testTextInput.receiveAction(TextInputAction.done); - await tester.pumpAndSettle(safePumpDuration); + // Verify that the gutter item and line item are aligned: + expect( + areHorizontallyAligned( + gutter57Finder, + line57Finder, + tester: tester, + ), + isTrue, + ); + + logStatus('setting a breakpoint'); + + // Tap on the gutter for the line to set a breakpoint: + await tester.tap(gutter57Finder); + await tester.pumpAndSettle(longPumpDuration); + + logStatus('performing a hot restart'); + + await tester.tap(find.byType(HotRestartButton)); + await tester.pumpAndSettle(longPumpDuration); + + logStatus('Navigating to line 30...'); + + await goToLine(tester, lineNumber: 30); logStatus('looking for line 30'); @@ -107,6 +126,11 @@ await tester.tap(gutter30Finder); await tester.pumpAndSettle(longPumpDuration); + logStatus('verifying breakpoints'); + + final bpSetBeforeRestart = findBreakpointWithText('main.dart:57'); + expect(bpSetBeforeRestart, findsOneWidget); + logStatus('pausing at breakpoint'); final topFrameFinder = findStackFrameWithText('incrementCounter'); @@ -170,6 +194,30 @@ return widgetACenter.dy == widgetBCenter.dy; } +Future<void> goToLine(WidgetTester tester, {required int lineNumber}) async { + logStatus('opening the "more" menu'); + + final moreMenuFinder = find.byType(PopupMenuButton<ScriptPopupMenuOption>); + expect(moreMenuFinder, findsOneWidget); + await tester.tap(moreMenuFinder); + await tester.pumpAndSettle(safePumpDuration); + + logStatus('selecting the go-to-line menu option'); + + final goToLineOptionFinder = find.textContaining('Go to line number'); + expect(goToLineOptionFinder, findsOneWidget); + await tester.tap(goToLineOptionFinder); + await tester.pumpAndSettle(safePumpDuration); + + logStatus('entering line number $lineNumber in the go-to-line dialog'); + + final goToLineInputFinder = find.widgetWithText(TextField, 'Line Number'); + expect(goToLineInputFinder, findsOneWidget); + await tester.enterText(goToLineInputFinder, '$lineNumber'); + await tester.testTextInput.receiveAction(TextInputAction.done); + await tester.pumpAndSettle(safePumpDuration); +} + T getWidgetFromFinder<T>(Finder finder) => finder.first.evaluate().first.widget as T; @@ -192,3 +240,8 @@ of: find.byType(CallStack), matching: find.richTextContaining(text), ); + +Finder findBreakpointWithText(String text) => find.descendant( + of: find.byType(Breakpoints), + matching: find.richTextContaining(text), + );
diff --git a/packages/devtools_app/lib/src/framework/framework_core.dart b/packages/devtools_app/lib/src/framework/framework_core.dart index f8792e1..fa52410 100644 --- a/packages/devtools_app/lib/src/framework/framework_core.dart +++ b/packages/devtools_app/lib/src/framework/framework_core.dart
@@ -103,7 +103,7 @@ service, onClosed: finishedCompleter.future, ); - breakpointManager.initialize(); + await breakpointManager.initialize(); return true; } catch (e, st) { if (logException) {
diff --git a/packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart b/packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart index 77bbd42..01835e1 100644 --- a/packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart +++ b/packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart
@@ -35,19 +35,21 @@ String get _isolateRefId => _isolateRef?.id ?? ''; - void initialize() { + final _previousIsolateBreakpoints = <BreakpointAndSourcePosition>[]; + + Future<void> initialize() async { final isolate = serviceConnection.serviceManager.isolateManager.selectedIsolate.value; if (initialSwitchToIsolate && isolate != null) { - switchToIsolate( + await switchToIsolate( serviceConnection.serviceManager.isolateManager.selectedIsolate.value, ); } addAutoDisposeListener( serviceConnection.serviceManager.isolateManager.selectedIsolate, - () { - switchToIsolate( + () async { + await switchToIsolate( serviceConnection.serviceManager.isolateManager.selectedIsolate.value, ); }, @@ -60,40 +62,38 @@ ); } - void switchToIsolate(IsolateRef? isolateRef) async { + Future<void> switchToIsolate(IsolateRef? isolateRef) async { _isolateRef = isolateRef; if (isolateRef == null) { - _breakpoints.value.clear(); - _breakpointsWithLocation.value.clear(); + _saveAndClearCurrentBreakpoints(); return; } - final isolate = await _service.getIsolate(_isolateRefId); - if (isolate.id != _isolateRefId) { - // Current request is obsolete. - return; + final breakpointsForIsolate = + await _getBreakpointsForIsolate(_isolateRefId); + if (breakpointsForIsolate.isNotEmpty) { + // If the isolate already has breakpoints, then update them: + await _updateBreakpoints( + breakpoints: breakpointsForIsolate, + isolateId: _isolateRefId, + ); + } else { + // Otherwise, re-establish the breakpoints from the previous isolate: + await _setUpBreakpoints( + breakpoints: _previousIsolateBreakpoints, + isolateRef: isolateRef, + ); } - - _breakpoints.value = isolate.breakpoints ?? []; - - // Build _breakpointsWithLocation from _breakpoints. - // ignore: unawaited_futures - Future.wait( - _breakpoints.value.map(breakpointManager.createBreakpointWithLocation), - ).then((list) { - if (isolate.id != _isolateRefId) { - // Current request is obsolete. - return; - } - _breakpointsWithLocation.value = list.toList()..sort(); - }); } - void clearCache() { + void clearCache({required bool isServiceShutdown}) { _breakPositionsMap.clear(); _breakpoints.value = []; _breakpointsWithLocation.value = []; + if (isServiceShutdown) { + _previousIsolateBreakpoints.clear(); + } } Future<void> clearBreakpoints() async { @@ -138,6 +138,16 @@ } } + void _saveAndClearCurrentBreakpoints() { + if (breakpointsWithLocation.value.isNotEmpty) { + _previousIsolateBreakpoints + ..clear() + ..addAll(_breakpointsWithLocation.value); + } + _breakpoints.value = []; + _breakpointsWithLocation.value = []; + } + void _updateAfterIsolateReload( Event _, ) async { @@ -175,6 +185,87 @@ ]); } + Future<List<Breakpoint>> _getBreakpointsForIsolate(String isolateId) async { + final isolate = await _service.getIsolate(isolateId); + if (isolate.id != _isolateRefId) { + // Current request is obsolete. + return []; + } + + // Ignore attempts from DWDS to re-establish breakpoints because DevTools is + // now in charge of re-establishing breakpoints: + final connectedToDwds = + serviceConnection.serviceManager.connectedApp?.isDartWebAppNow ?? false; + if (connectedToDwds) return []; + + return isolate.breakpoints ?? []; + } + + Future<void> _updateBreakpoints({ + required List<Breakpoint> breakpoints, + required String isolateId, + }) async { + _breakpoints.value = breakpoints; + // Build _breakpointsWithLocation from _breakpoints. + await Future.wait( + _breakpoints.value.map(breakpointManager.createBreakpointWithLocation), + ).then((list) { + if (isolateId != _isolateRefId) { + // Current request is obsolete. + return; + } + _breakpointsWithLocation.value = list.toList()..sort(); + }); + } + + Future<void> _setUpBreakpoints({ + required List<BreakpointAndSourcePosition> breakpoints, + required IsolateRef isolateRef, + }) async { + final scriptUriToRef = await _scriptRefsForBreakpoints( + breakpoints: breakpoints, + isolateRef: isolateRef, + ); + + for (final breakpoint in breakpoints) { + final newScriptRef = scriptUriToRef[breakpoint.scriptUri]; + final breakpointLine = breakpoint.line; + + final scriptId = newScriptRef?.id; + if (scriptId != null && breakpointLine != null) { + await addBreakpoint(scriptId, breakpointLine); + } + } + } + + Future<Map<String, ScriptRef>> _scriptRefsForBreakpoints({ + required List<BreakpointAndSourcePosition> breakpoints, + required IsolateRef isolateRef, + }) async { + final bpScriptUris = breakpoints.fold( + <String>{}, + (scriptSet, breakpoint) { + final scriptUri = breakpoint.scriptUri; + if (scriptUri != null) { + scriptSet.add(scriptUri); + } + return scriptSet; + }, + ); + + final newScripts = await scriptManager.retrieveAndSortScripts(isolateRef); + final scriptUriToRef = + newScripts.fold(<String, ScriptRef>{}, (scriptMap, script) { + final scriptUri = script.uri; + if (scriptUri != null && bpScriptUris.contains(scriptUri)) { + scriptMap[scriptUri] = script; + } + return scriptMap; + }); + + return scriptUriToRef; + } + /// Return the list of valid positions for breakpoints for a given script. Future<List<SourcePosition>> getBreakablePositions( IsolateRef? isolateRef, @@ -239,7 +330,7 @@ } } - void _handleDebugEvent(Event event) { + Future<void> _handleDebugEvent(Event event) async { if (event.isolate!.id != _isolateRefId) return; switch (event.kind) { @@ -247,18 +338,20 @@ // that knows how to notify when performing a list edit operation. case EventKind.kBreakpointAdded: final breakpoint = event.breakpoint!; + final isDuplicate = + _breakpoints.value.any((bp) => bp.id == breakpoint.id); + if (isDuplicate) break; _breakpoints.value = [..._breakpoints.value, breakpoint]; - unawaited( - breakpointManager.createBreakpointWithLocation(breakpoint).then((bp) { - final list = [ - ..._breakpointsWithLocation.value, - bp, - ]..sort(); - - _breakpointsWithLocation.value = list; - }), - ); + await breakpointManager + .createBreakpointWithLocation(breakpoint) + .then((bp) { + final list = [ + ..._breakpointsWithLocation.value, + bp, + ]..sort(); + _breakpointsWithLocation.value = list; + }); break; case EventKind.kBreakpointResolved: @@ -269,21 +362,30 @@ breakpoint, ]; - unawaited( - breakpointManager.createBreakpointWithLocation(breakpoint).then((bp) { - final list = _breakpointsWithLocation.value.toList(); - // Remote the bp with the older, unresolved information from the list. - list.removeWhere((breakpoint) => bp.breakpoint.id == bp.id); - // Add the bp with the newer, resolved information. - list.add(bp); - list.sort(); - _breakpointsWithLocation.value = list; - }), - ); + await breakpointManager + .createBreakpointWithLocation(breakpoint) + .then((bp) { + final list = _breakpointsWithLocation.value; + // Remove the bp with the older, unresolved information from the list. + list.removeWhere((breakpoint) => breakpoint.id == bp.id); + // Add the bp with the newer, resolved information. + list.add(bp); + list.sort(); + _breakpointsWithLocation.value = list; + }); break; case EventKind.kBreakpointRemoved: + // Ignore any breakpoints removed during a hot restart, because the VM + // service removes them before resuming the isolate and then performing + // the restart. Note we only track hot restarts triggered by DevTools, + // if a hot-restart was triggered by another client we won't know. + // See https://github.com/flutter/flutter/issues/134470 + final hotRestartInProgress = serviceConnection + .serviceManager.isolateManager.hotRestartInProgress; + if (hotRestartInProgress) break; + final breakpoint = event.breakpoint; _breakpoints.value = [
diff --git a/packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart b/packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart index 4801769..9214117 100644 --- a/packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart +++ b/packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart
@@ -82,7 +82,7 @@ /// Method to call after the vm service shuts down. void _onServiceShutdown() { - _clearCaches(); + _clearCaches(isServiceShutdown: true); _hasTruncatedFrames.value = false; unawaited(_getStackOperation?.cancel()); @@ -484,9 +484,9 @@ await _populateFrameInfo(stackInfo.frames, truncated: stackInfo.truncated); } - void _clearCaches() { + void _clearCaches({bool isServiceShutdown = false}) { _lastEvent = null; - breakpointManager.clearCache(); + breakpointManager.clearCache(isServiceShutdown: isServiceShutdown); } Future<void> _populateScripts(Isolate isolate) async {
diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index cc61007..cca4832 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
@@ -46,6 +46,7 @@ * Improved contrast of profiling details when displaying profiler hits in dark mode. - [#7178](https://github.com/flutter/devtools/pull/7178) * Fixed syntax highlighting for comments when the source file uses `\r\n` line endings [#7190](https://github.com/flutter/devtools/pull/7190) +* Re-establish breakpoints after a hot-restart. - [#7205](https://github.com/flutter/devtools/pull/7205) ## Network profiler updates
diff --git a/packages/devtools_app/test/test_infra/fixtures/flutter_app/lib/main.dart b/packages/devtools_app/test/test_infra/fixtures/flutter_app/lib/main.dart index 07d9ec9..1f7bfdb 100644 --- a/packages/devtools_app/test/test_infra/fixtures/flutter_app/lib/main.dart +++ b/packages/devtools_app/test/test_infra/fixtures/flutter_app/lib/main.dart
@@ -43,7 +43,17 @@ body: const Center( child: Text('Hello, World!'), ), + floatingActionButton: FloatingActionButton( + onPressed: _printHello, + tooltip: 'Say hi', + child: const Icon(Icons.abc), + ), ), ); } + + void _printHello() { + // ignore: avoid_print, for testing. + print('Hello!'); + } }
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_animated_physical_model_selected.png b/packages/devtools_app/test/test_infra/goldens/integration_animated_physical_model_selected.png index 96e0b31..70a6872 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_animated_physical_model_selected.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_animated_physical_model_selected.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_inspector_initial_load.png b/packages/devtools_app/test/test_infra/goldens/integration_inspector_initial_load.png index 1d80bdd..ee3d0e1 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_inspector_initial_load.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_inspector_initial_load.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_inspector_richtext_selected.png b/packages/devtools_app/test/test_infra/goldens/integration_inspector_richtext_selected.png index c9c3b3a..a9f6174 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_inspector_richtext_selected.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_inspector_richtext_selected.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_inspector_scaffold_selected.png b/packages/devtools_app/test/test_infra/goldens/integration_inspector_scaffold_selected.png index b3c4024..2b66f6a 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_inspector_scaffold_selected.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_inspector_scaffold_selected.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center.png b/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center.png index 995c7b6..0c2309d 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center_details_tree.png b/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center_details_tree.png index 3cab036..2162267 100644 --- a/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center_details_tree.png +++ b/packages/devtools_app/test/test_infra/goldens/integration_inspector_select_center_details_tree.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/memory_diff_empty1.png b/packages/devtools_app/test/test_infra/goldens/memory_diff_empty1.png index e73f0d8..8f259c2 100644 --- a/packages/devtools_app/test/test_infra/goldens/memory_diff_empty1.png +++ b/packages/devtools_app/test/test_infra/goldens/memory_diff_empty1.png Binary files differ
diff --git a/packages/devtools_app/test/test_infra/goldens/memory_diff_empty2.png b/packages/devtools_app/test/test_infra/goldens/memory_diff_empty2.png index e73f0d8..8f259c2 100644 --- a/packages/devtools_app/test/test_infra/goldens/memory_diff_empty2.png +++ b/packages/devtools_app/test/test_infra/goldens/memory_diff_empty2.png Binary files differ
diff --git a/packages/devtools_app_shared/lib/src/service/isolate_manager.dart b/packages/devtools_app_shared/lib/src/service/isolate_manager.dart index 2548443..5aa305e 100644 --- a/packages/devtools_app_shared/lib/src/service/isolate_manager.dart +++ b/packages/devtools_app_shared/lib/src/service/isolate_manager.dart
@@ -50,6 +50,10 @@ final _isolateRunnableCompleters = <String?, Completer<void>>{}; + // TODO(https://github.com/flutter/flutter/issues/134470): Track hot-restarts + // triggered by other clients. + bool hotRestartInProgress = false; + Future<void> init(List<IsolateRef> isolates) async { await initIsolates(isolates); } @@ -132,6 +136,9 @@ () => Completer<void>(), ); isolateRunnable.complete(); + if (hotRestartInProgress) { + hotRestartInProgress = false; + } } else if (event.kind == EventKind.kIsolateStart && !event.isolate!.isSystemIsolate!) { await _registerIsolate(event.isolate!);
diff --git a/packages/devtools_app_shared/lib/src/service/service_manager.dart b/packages/devtools_app_shared/lib/src/service/service_manager.dart index e4d7d85..19a9232 100644 --- a/packages/devtools_app_shared/lib/src/service/service_manager.dart +++ b/packages/devtools_app_shared/lib/src/service/service_manager.dart
@@ -433,7 +433,12 @@ /// This can throw an [RPCError]. Future<void> performHotRestart() async { - await callServiceOnMainIsolate(hotRestartServiceName); + isolateManager.hotRestartInProgress = true; + try { + await callServiceOnMainIsolate(hotRestartServiceName); + } catch (_) { + isolateManager.hotRestartInProgress = false; + } } }
