Wait until all scripts are parsed before recomputing metadata on a hot reload

Fixes #2640

On a hot reload, we wait until all the scripts are downloaded, but we
don't wait until all of them are parsed and a script ID is created for
them to refer to. This then results in incorrect metadata being computed.
Instead, return the srcs that are being loaded during a hot reload,
use a controller to determine when scripts are parsed, and only
compute metadata once we have parsed all the reloaded scripts.

In order to compare the parsed scripts' URLs with the reloaded scripts'
URLs, we now require full URLs in the hot reload sources metadata.
This is already true in Flutter tools, so this just canonicalizes that
and modifies the tests to do the same.

To be consistent, hot restart also provides the full URL in the DDC
library bundle format and the bootstrap is modified to reflect that
(and to clean up some unused code around module tracking).

Author: srujzs

dwds/lib/src/debugging/debugger.dart

@@ -56,6 +56,9 @@ class Debugger extends Domain {
 
   int _frameErrorCount = 0;
 
+  final StreamController<String> parsedScriptsController =
+      StreamController.broadcast();
+
   Debugger._(
     this._remoteDebugger,
     this._streamNotify,
@@ -489,6 +492,9 @@ class Debugger extends Domain {
     final scriptPath = _pathForChromeScript(e.script.url);
     if (scriptPath != null) {
       chromePathToRuntimeScriptId[scriptPath] = e.script.scriptId;
+      if (parsedScriptsController.hasListener) {
+        parsedScriptsController.sink.add(e.script.url);
+      }
     }
   }
 

dwds/lib/src/debugging/modules.dart

@@ -98,9 +98,6 @@ class Modules {
   ) async {
     final serverPath = await globalToolConfiguration.loadStrategy
         .serverPathForModule(entrypoint, module);
-    // TODO(srujzs): We should wait until all scripts are parsed before
-    // accessing after a hot reload. See
-    // https://github.com/dart-lang/webdev/issues/2640.
     return chromePathToRuntimeScriptId[serverPath];
   }
 

dwds/lib/src/injected/client.js

@@ -110,14 +110,16 @@ class DdcLibraryBundleStrategy extends LoadStrategy {
   /// ```json
   /// [
   ///   {
-  ///     "src": "<file_name>",
+  ///     "src": "<base_uri>/<file_name>",
+  ///     "module": "<module_name>",
   ///     "libraries": ["<lib1>", "<lib2>"],
   ///   },
   /// ]
   /// ```
   ///
   /// `src`: A string that corresponds to the file path containing a DDC library
   /// bundle.
+  /// `module`: The name of the library bundle in `src`.
   /// `libraries`: An array of strings containing the libraries that were
   /// compiled in `src`.
   ///

dwds/lib/src/loaders/ddc_library_bundle.dart

@@ -1221,15 +1221,42 @@ class ChromeProxyService implements VmServiceInterface {
   Future<void> _performClientSideHotReload(String isolateId) async {
     _logger.info('Attempting a hot reload');
 
+    final debugger = await debuggerFuture;
+    final reloadedSrcs = <String>{};
+    final computedReloadedSrcs = Completer<void>();
+    final parsedAllReloadedSrcs = Completer<void>();
+    // Wait until all the reloaded scripts are parsed before we reinitialize
+    // metadata below.
+    final parsedScriptsSubscription = debugger.parsedScriptsController.stream
+        .listen((url) {
+          computedReloadedSrcs.future.then((_) {
+            reloadedSrcs.remove(Uri.parse(url).normalizePath().path);
+            if (reloadedSrcs.isEmpty) {
+              parsedAllReloadedSrcs.complete();
+            }
+          });
+        });
+
     // Initiate a hot reload.
     _logger.info('Issuing \$dartHotReloadStartDwds request');
     final remoteObject = await inspector.jsEvaluate(
       '\$dartHotReloadStartDwds();',
       awaitPromise: true,
       returnByValue: true,
     );
-    final reloadedModulesToLibraries =
-        (remoteObject.value as Map).cast<String, List>();
+    final reloadedSrcModuleLibraries = (remoteObject.value as List).cast<Map>();
+    final reloadedModulesToLibraries = <String, List<String>>{};
+    for (final srcModuleLibrary in reloadedSrcModuleLibraries) {
+      final srcModuleLibraryCast = srcModuleLibrary.cast<String, Object>();
+      reloadedSrcs.add(
+        Uri.parse(srcModuleLibraryCast['src'] as String).normalizePath().path,
+      );
+      reloadedModulesToLibraries[srcModuleLibraryCast['module'] as String] =
+          (srcModuleLibraryCast['libraries'] as List).cast<String>();
+    }
+    computedReloadedSrcs.complete();
+    if (reloadedSrcs.isNotEmpty) await parsedAllReloadedSrcs.future;
+    await parsedScriptsSubscription.cancel();
 
     if (!pauseIsolatesOnStart) {
       // Finish hot reload immediately.

dwds/lib/src/services/chrome_proxy_service.dart

@@ -339,11 +339,12 @@ class TestContext {
             _hostname = appMetadata.hostname;
             await webRunner.run(
               frontendServerFileSystem,
-              _hostname,
-              assetServerPort,
-              filePathToServe,
+              hostname: _hostname,
+              port: assetServerPort,
+              index: filePathToServe,
               initialCompile: true,
               fullRestart: false,
+              fileServerUri: null,
             );
 
             if (testSettings.enableExpressionEvaluation) {
@@ -414,7 +415,7 @@ class TestContext {
                   'remote-debugging-port=$debugPort',
                   if (enableDebugExtension)
                     '--load-extension=debug_extension/prod_build',
-                  if (headless) '--headless',
+                  // if (headless) '--headless',
                 ],
               },
             });
@@ -433,6 +434,10 @@ class TestContext {
       final appConnectionCompleter = Completer();
       final connection = ChromeConnection('localhost', debugPort);
 
+      // TODO(srujzs): In the case of the frontend server, it doesn't make sense
+      // that we initialize a new HTTP server instead of reusing the one in
+      // `TestAssetServer`. Why not just use that one? That seems to match with
+      // what Flutter tools is doing.
       _testServer = await TestServer.start(
         debugSettings: debugSettings.copyWith(
           expressionCompiler: expressionCompiler,
@@ -592,11 +597,9 @@ class TestContext {
   Future<void> recompile({required bool fullRestart}) async {
     await webRunner.run(
       frontendServerFileSystem,
-      _hostname,
-      await findUnusedPort(),
-      webCompatiblePath([project.directoryToServe, project.filePathToServe]),
       initialCompile: false,
       fullRestart: fullRestart,
+      fileServerUri: Uri.parse('http://${testServer.host}:${testServer.port}'),
     );
     return;
   }

dwds/test/fixtures/context.dart

@@ -8,7 +8,6 @@
 library;
 
 import 'dart:async';
-import 'dart:io';
 
 import 'package:dwds/expression_compiler.dart';
 import 'package:test/test.dart';
@@ -207,6 +206,37 @@ void main() {
     Future<void> waitForBreakpoint() =>
         expectLater(stream, emitsThrough(_hasKind(EventKind.kPauseBreakpoint)));
 
+    test('empty hot reload keeps breakpoints', () async {
+      final genString = 'main gen0';
+
+      final bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: callLogMarker,
+      );
+
+      var breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genString);
+
+      await context.recompile(fullRestart: false);
+
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: callLogMarker, bp: bp),
+      ]);
+
+      breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genString);
+    });
+
     test('after edit and hot reload, breakpoint is in new file', () async {
       final oldString = 'main gen0';
       final newString = 'main gen1';
@@ -359,52 +389,109 @@ void main() {
       },
     );
 
-    test(
-      'breakpoint in captured code is deleted',
-      () async {
-        var bp = await addBreakpoint(
-          file: mainFile,
-          breakpointMarker: capturedStringMarker,
-        );
+    // Test that we wait for all scripts to be parsed first before computing
+    // location metadata.
+    test('after adding many files and putting breakpoint in the last one,'
+        'breakpoint is correctly registered', () async {
+      final genLog = 'main gen0';
 
-        final oldLog = "log('\$mainValue');";
-        final newLog = "log('\${closure()}');";
-        await makeEditAndRecompile(mainFile, oldLog, newLog);
+      final bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: callLogMarker,
+      );
 
-        bp =
-            (await hotReloadAndHandlePausePost([
-              (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
-            ])).first;
+      var breakpointFuture = waitForBreakpoint();
 
-        final breakpointFuture = waitForBreakpoint();
+      await callEvaluate();
 
-        await callEvaluate();
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genLog);
 
-        // Should break at `capturedString`.
-        await breakpointFuture;
-        final oldCapturedString = 'captured closure gen0';
-        // Closure gets evaluated for the first time.
-        await resumeAndExpectLog(oldCapturedString);
-
-        final newCapturedString = 'captured closure gen1';
-        await makeEditAndRecompile(
-          mainFile,
-          oldCapturedString,
-          newCapturedString,
+      // Add library files, import them, but only refer to the last one in main.
+      final numFiles = 50;
+      for (var i = 1; i <= numFiles; i++) {
+        final libFile = 'library$i.dart';
+        context.addLibraryFile(
+          libFileName: libFile,
+          contents: '''String get libraryValue$i {
+            return 'lib gen$i'; // Breakpoint: libValue$i
+          }''',
         );
+        final oldImports = "import 'dart:js_interop';";
+        final newImports =
+            '$oldImports\n'
+            "import 'package:_test_hot_reload_breakpoints/$libFile';";
+        makeEdit(mainFile, oldImports, newImports);
+      }
+      final oldLog = "log('\$mainValue');";
+      final newLog = "log('\$libraryValue$numFiles');";
+      await makeEditAndRecompile(mainFile, oldLog, newLog);
 
-        await hotReloadAndHandlePausePost([
-          (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
-        ]);
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: callLogMarker, bp: bp),
+        (
+          file: 'library$numFiles.dart',
+          breakpointMarker: 'libValue$numFiles',
+          bp: null,
+        ),
+      ]);
 
-        // Breakpoint should not be hit as it's now deleted. We should also see
-        // the old string still as the closure has not been reevaluated.
-        await callEvaluateAndExpectLog(oldCapturedString);
-      },
-      // TODO(srujzs): Re-enable after
-      // https://github.com/dart-lang/webdev/issues/2640.
-      skip: Platform.isWindows,
-    );
+      breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+
+      breakpointFuture = waitForBreakpoint();
+
+      await resume();
+      // Should break at the breakpoint in the last file.
+      await breakpointFuture;
+      await resumeAndExpectLog('lib gen$numFiles');
+    });
+
+    test('breakpoint in captured code is deleted', () async {
+      var bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: capturedStringMarker,
+      );
+
+      final oldLog = "log('\$mainValue');";
+      final newLog = "log('\${closure()}');";
+      await makeEditAndRecompile(mainFile, oldLog, newLog);
+
+      bp =
+          (await hotReloadAndHandlePausePost([
+            (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
+          ])).first;
+
+      final breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `capturedString`.
+      await breakpointFuture;
+      final oldCapturedString = 'captured closure gen0';
+      // Closure gets evaluated for the first time.
+      await resumeAndExpectLog(oldCapturedString);
+
+      final newCapturedString = 'captured closure gen1';
+      await makeEditAndRecompile(
+        mainFile,
+        oldCapturedString,
+        newCapturedString,
+      );
+
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
+      ]);
+
+      // Breakpoint should not be hit as it's now deleted. We should also see
+      // the old string still as the closure has not been reevaluated.
+      await callEvaluateAndExpectLog(oldCapturedString);
+    });
   }, timeout: Timeout.factor(2));
 
   group('when pause_isolates_on_start is false', () {