[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 <aam@google.com>
Commit-Queue: Derek Xu <derekx@google.com>

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 <test.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<void> 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 = <IsolateTest>[
+  // 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 <String>[]]) => runIsolateTests(
+      args,
+      tests,
+      'breakpoint_resolution_after_reloading_test.dart',
+      testeeConcurrent: testeeMain,
+    );

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);
       }

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