From 45221e51c37ec34e10bb321f5ef2c12e9898f0a0 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Thu, 16 May 2024 21:57:18 +0000 Subject: [PATCH] [VM/Debugger] Prevent FindBestFit from considering closures that were compiled prior to the latest reload TEST=verified that pkg/vm_service/test/breakpoint_resolution_after_reloading_test.dart fails without the changes in this CL and passes with them, verified that none of the existing debugger tests got broken by this CL Change-Id: Ie816a3ad65a17ef9f497f209f2203d679728cb75 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366860 Reviewed-by: Alexander Aprelev Commit-Queue: Derek Xu --- ...point_resolution_after_reloading_test.dart | 183 ++++++++++++++++++ runtime/vm/debugger.cc | 3 +- runtime/vm/debugger.h | 10 +- 3 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 pkg/vm_service/test/breakpoint_resolution_after_reloading_test.dart diff --git a/pkg/vm_service/test/breakpoint_resolution_after_reloading_test.dart b/pkg/vm_service/test/breakpoint_resolution_after_reloading_test.dart new file mode 100644 index 00000000000..2e0310e3b12 --- /dev/null +++ b/pkg/vm_service/test/breakpoint_resolution_after_reloading_test.dart @@ -0,0 +1,183 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:developer' show debugger; +import 'dart:io' show Directory, File; +import 'dart:isolate' as i; + +import 'package:path/path.dart' show join; +import 'package:test/test.dart'; +import 'package:vm_service/vm_service.dart'; + +import 'common/service_test_common.dart'; +import 'common/test_helper.dart'; + +// AUTOGENERATED START +// +// Update these constants by running: +// +// dart pkg/vm_service/test/update_line_numbers.dart +// +const LINE_A = 78; +// AUTOGENERATED END + +const v0Contents = ''' +import 'dart:developer'; + +void f() {} + +void main() { + debugger(); + f(); + f(); +} +'''; + +const v1Contents = ''' +import 'dart:developer'; + +void f() { + (() { + (() { + print('v1'); + })(); + })(); +} + +void main() { + f(); + f(); +} +'''; + +const v2Contents = ''' +import 'dart:developer'; + +void f() { + (() { + print('v2.a'); + print('v2.b'); + })(); +} + +void main() { + f(); + f(); +} +'''; + +Future testeeMain() async { + // Spawn the child isolate. + final tempDir = Directory.systemTemp.createTempSync(); + try { + final rootLib = File(join(tempDir.path, 'main.dart')); + rootLib.writeAsStringSync(v0Contents); + + await i.Isolate.spawnUri(rootLib.uri, [], null); + debugger(); // LINE_A + tempDir.deleteSync(recursive: true); + } catch (_) { + tempDir.deleteSync(recursive: true); + rethrow; + } +} + +final tests = [ + // Ensure that the main isolate has stopped at the [debugger] statement at the + // end of [testeeMain]. + hasStoppedAtBreakpoint, + stoppedAtLine(LINE_A), + (VmService service, IsolateRef isolateRef) async { + final tempDir = Directory.systemTemp.createTempSync(); + try { + // This test is a regression test against a bug caused by comparing script + // URLs instead of script pointers in the debugger. To produce a situation + // in which the bug used to occur, this test loads + // [spawnedIsolateRootLib], modifies [spawnedIsolateRootLib], and then + // reloads [spawnedIsolateRootLib]. + final spawnedIsolateRootLib = File(join(tempDir.path, 'main.dart')); + + // Find the spawned isolate. + final vm = await service.getVM(); + final isolates = vm.isolates!; + expect(isolates.length, 2); + final spawnedIsolateRef = isolates.firstWhere( + (i) => i != isolateRef, + ); + final spawnedIsolateId = spawnedIsolateRef.id!; + + // Load [v1Contents] into the spawned isolate. + spawnedIsolateRootLib.writeAsStringSync(v1Contents); + await service.reloadSources( + spawnedIsolateId, + rootLibUri: spawnedIsolateRootLib.uri.toString(), + force: true, + ); + + Isolate spawnedIsolate = await service.getIsolate(spawnedIsolateId); + Library rootLib = await service.getObject( + spawnedIsolateId, + spawnedIsolate.rootLib!.id!, + ) as Library; + String scriptId = rootLib.scripts![0].id!; + + // Add a breakpoint at `print('v1');`. + await service.addBreakpoint(spawnedIsolateId, scriptId, 6); + + // Resuming the spawned isolate should let it run until it gets paused at + // the breakpoint at `print('v1');`. + await resumeIsolate(service, spawnedIsolateRef); + await hasStoppedAtBreakpoint(service, spawnedIsolateRef); + await stoppedAtLine(6)(service, spawnedIsolateRef); + + // Load [v2Contents] into the spawned isolate. + spawnedIsolateRootLib.writeAsStringSync(v2Contents); + await service.reloadSources( + spawnedIsolateId, + rootLibUri: spawnedIsolateRootLib.uri.toString(), + force: true, + ); + + spawnedIsolate = await service.getIsolate(spawnedIsolateId); + rootLib = await service.getObject( + spawnedIsolateId, + spawnedIsolate.rootLib!.id!, + ) as Library; + scriptId = rootLib.scripts![0].id!; + + // Add a breakpoint at `print('v2.a');`. + await service.addBreakpoint(spawnedIsolateId, scriptId, 5); + + // Resuming the spawned isolate should let it run until it gets paused at + // the breakpoint at `print('v2.a');`. + await resumeIsolate(service, spawnedIsolateRef); + await hasStoppedAtBreakpoint(service, spawnedIsolateRef); + await stoppedAtLine(5)(service, spawnedIsolateRef); + + // Add a breakpoint at `print('v2.b');`. + final breakpoint3 = + await service.addBreakpoint(spawnedIsolateId, scriptId, 6); + expect(breakpoint3.breakpointNumber, 3); + + // We previously had a bug that would have made the breakpoint resolution + // code get confused by the old closure that was defined in [v1Contents]. + // We prevent a reintroduction of that bug by ensuring that the newly set + // breakpoint has been resolved immediately. + expect(breakpoint3.resolved, true); + + await resumeIsolate(service, spawnedIsolateRef); + tempDir.deleteSync(recursive: true); + } catch (_) { + tempDir.deleteSync(recursive: true); + rethrow; + } + }, +]; + +void main([args = const []]) => runIsolateTests( + args, + tests, + 'breakpoint_resolution_after_reloading_test.dart', + testeeConcurrent: testeeMain, + ); diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc index 64da86a8e19..d7ec91e3f5c 100644 --- a/runtime/vm/debugger.cc +++ b/runtime/vm/debugger.cc @@ -2408,7 +2408,8 @@ bool Debugger::FindBestFit(const Script& script, const String& script_url = String::Handle(zone, script.url()); ClosureFunctionsCache::ForAllClosureFunctions([&](const Function& fun) { - if (FunctionOverlaps(fun, script_url, token_pos, last_token_pos)) { + if (fun.script() == script.ptr() && + FunctionOverlaps(fun, script_url, token_pos, last_token_pos)) { // Select the inner most closure. UpdateBestFit(best_fit, fun); } diff --git a/runtime/vm/debugger.h b/runtime/vm/debugger.h index 7a3e7d8e8ef..099263e0f96 100644 --- a/runtime/vm/debugger.h +++ b/runtime/vm/debugger.h @@ -98,10 +98,12 @@ class Breakpoint { // BreakpointLocation represents a collection of breakpoint conditions at the // same token position in Dart source. There may be more than one CodeBreakpoint // object per BreakpointLocation. -// An unresolved breakpoint is one where the underlying code has not -// been compiled yet. Since the code has not been compiled, we don't know -// the definitive source location yet. The requested source location may -// change when the underlying code gets compiled. +// +// An unresolved breakpoint, also known as a pending breakpoint, is one where +// the underlying code has not been compiled yet. Since the code has not been +// compiled, we can't determine the definitive token position to associate with +// the breakpoint yet. +// // A latent breakpoint represents a breakpoint location in Dart source // that is not loaded in the VM when the breakpoint is requested. // When a script with matching url is loaded, a latent breakpoint