Wait until all scripts are parsed before recomputing metadata on a hot reload (#2650)

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.

Author: srujzs

dwds/CHANGELOG.md

@@ -1,5 +1,8 @@
 ## 24.4.1-wip
 
+- Fixed an issue where we didn't wait until all scripts were parsed before
+  recomputing metadata on a hot reload.
+
 ## 24.4.0
 
 - Added support for breakpoint registering on a hot reload with the DDC library bundle format using PausePostRequests.

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

@@ -2225,27 +2225,6 @@ void runTests({
       );
     });
 
-    test('reloadSources', () async {
-      final service = context.service;
-      final vm = await service.getVM();
-      final isolateId = vm.isolates!.first.id!;
-      final report = await service.reloadSources(isolateId);
-      // TODO(srujzs): This naturally fails regardless of the module format
-      // because we didn't set up prerequisite state (the reload scripts). We
-      // should create new tests for hot reload within DWDS and remove this
-      // test.
-      expect(report.success, false);
-      // Make sure that a notice was provided in the expected format.
-      final notices = report.json?['notices'];
-      expect(notices, isNotNull);
-      expect(notices is List, isTrue);
-      final noticeList = (notices as List).cast<Map>();
-      expect(noticeList, isNotEmpty);
-      final message = noticeList[0]['message'];
-      expect(message, isNotNull);
-      expect(message is String && message.isNotEmpty, isTrue);
-    });
-
     test('setIsolatePauseMode', () async {
       final service = context.service;
       final vm = await service.getVM();

dwds/test/common/chrome_proxy_service_common.dart

@@ -339,11 +339,9 @@ class TestContext {
             _hostname = appMetadata.hostname;
             await webRunner.run(
               frontendServerFileSystem,
-              _hostname,
-              assetServerPort,
-              filePathToServe,
-              initialCompile: true,
-              fullRestart: false,
+              hostname: _hostname,
+              port: assetServerPort,
+              index: filePathToServe,
             );
 
             if (testSettings.enableExpressionEvaluation) {
@@ -433,6 +431,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`. We should instead use that one to align with Flutter
+      // tools.
       _testServer = await TestServer.start(
         debugSettings: debugSettings.copyWith(
           expressionCompiler: expressionCompiler,
@@ -590,13 +592,9 @@ class TestContext {
   }
 
   Future<void> recompile({required bool fullRestart}) async {
-    await webRunner.run(
-      frontendServerFileSystem,
-      _hostname,
-      await findUnusedPort(),
-      webCompatiblePath([project.directoryToServe, project.filePathToServe]),
-      initialCompile: false,
+    await webRunner.rerun(
       fullRestart: fullRestart,
+      fileServerUri: Uri.parse('http://${testServer.host}:${testServer.port}'),
     );
     return;
   }