[dds/dap] Improve the error message when lookupResolvedPackageUris returns an invalid response

+ add a failing test for https://github.com/dart-lang/sdk/issues/52632 / https://github.com/flutter/flutter/issues/137163

This does not resolve the issue, it only makes it clearer from the error message what the problem is. Unfortunately there is no good workaround when we hit this bug because we can't tell which items in the response are valid/invalid. If we treat them all as invalid (eg. complete all of the completes with null), many things that involve mappings of URIs<->files won't work correctly (breakpoints, opening the right file when we break, etc.).

A workaround is for users not to have commas in their filenames, although it's not clear if there may be other characters triggering this right now.

Change-Id: I6ac7cbd82b726fac76c27d1e00a06f2a07d4757f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/342800
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Helin Shiah <helinx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>

pkg/dds/lib/src/dap/isolate_manager.dart

@@ -1157,6 +1157,19 @@ class ThreadInfo with FileUtils {
         if (results == null) {
           // If no result, all of the results are null.
           completers.forEach((uri, completer) => completer.complete(null));
+        } else if (results.length != requiredUris.length) {
+          // If the lengths of the lists are different, we have an invalid
+          // response from the VM. This is a bug in the VM/VM Service:
+          // https://github.com/dart-lang/sdk/issues/52632
+
+          final reason =
+              results.length > requiredUris.length ? 'more' : 'fewer';
+          final message =
+              'lookupResolvedPackageUris result contained $reason results than '
+              'the request. See https://github.com/dart-lang/sdk/issues/52632';
+          final error = Exception(message);
+          completers
+              .forEach((uri, completer) => completer.completeError(error));
         } else {
           // Otherwise, complete each one by index with the corresponding value.
           results.map(_convertUriToFilePath).forEachIndexed((i, result) {
@@ -1167,7 +1180,15 @@ class ThreadInfo with FileUtils {
       } catch (e) {
         // We can't leave dangling completers here because others may already
         // be waiting on them, so propagate the error to them.
-        completers.forEach((uri, completer) => completer.completeError(e));
+        completers.forEach((uri, completer) {
+          // Only complete if not already completed. It's possible an exception
+          // occurred above inside the loop and that some of the completers have
+          // already completed. We don't want to replace a good exception with
+          // "Future already completed".
+          if (!completer.isCompleted) {
+            completer.completeError(e);
+          }
+        });
 
         // Don't rethrow here, because it will cause these completers futures
         // to not have error handlers attached which can cause their errors to

pkg/dds/test/dap/integration/debug_test.dart

@@ -98,6 +98,37 @@ main() {
       expect(proc!.exitCode, completes);
     });
 
+    test('runs a simple script with commas in the filename', () async {
+      final packageUri = await dap.createFooPackage('foo,foo.dart');
+      final testFile = dap.createTestFile(
+        '''
+          import '$packageUri';
+          void main() {
+            foo();
+          }
+        ''',
+      );
+
+      final outputEvents = await dap.client.collectOutput(
+        launch: () => dap.client.launch(
+          testFile.path,
+          args: ['one', 'two'],
+        ),
+      );
+
+      // Expect the normal applications output. This means we set up the
+      // debugger without crashing, even though we imported files with commas
+      // in the name.
+      final output = outputEvents.skip(1).map((e) => e.output).join();
+      expectLines(output, [
+        'Hello!',
+        'World!',
+        'args: [one, two]',
+        '',
+        'Exited.',
+      ]);
+    }, skip: 'Fails because of https://github.com/dart-lang/sdk/issues/52632');
+
     test('does not resume isolates if user passes --pause-isolates-on-exit',
         () async {
       // Internally we always pass --pause-isolates-on-exit and resume the

pkg/dds/test/dap/integration/test_support.dart

@@ -175,7 +175,7 @@ class DapTestSession {
   }
 
   /// Create a simple package named `foo` that has an empty `foo` function.
-  Future<Uri> createFooPackage() {
+  Future<Uri> createFooPackage([String? filename]) {
     return createSimplePackage(
       'foo',
       '''
@@ -183,6 +183,7 @@ foo() {
   // Does nothing.
 }
       ''',
+      filename,
     );
   }
 
@@ -203,8 +204,10 @@ environment:
   /// .dart_tool/package_config.json
   Future<Uri> createSimplePackage(
     String name,
-    String content,
-  ) async {
+    String content, [
+    String? filename,
+  ]) async {
+    filename ??= '$name.dart';
     final packageDir = Directory(path.join(testPackagesDir.path, name))
       ..createSync(recursive: true);
     final packageLibDir = Directory(path.join(packageDir.path, 'lib'))
@@ -212,21 +215,21 @@ environment:
 
     // Create a pubspec and a implementation file in the lib folder.
     createPubspec(packageDir, name);
-    final testFile = File(path.join(packageLibDir.path, '$name.dart'));
+    final testFile = File(path.join(packageLibDir.path, filename));
     testFile.writeAsStringSync(content);
 
     // Add this new package as a dependency for the app.
     final fileUri = Uri.file('${packageDir.path}/');
     await addPackageDependency(testAppDir, name, fileUri);
 
-    return Uri.parse('package:$name/$name.dart');
+    return Uri.parse('package:$name/$filename');
   }
 
   /// Creates a file in a temporary folder to be used as an application for testing.
   ///
   /// The file will be deleted at the end of the test run.
-  File createTestFile(String content) {
-    final testFile = File(path.join(testAppDir.path, 'test_file.dart'));
+  File createTestFile(String content, [String filename = 'test_file.dart']) {
+    final testFile = File(path.join(testAppDir.path, filename));
     testFile.writeAsStringSync(content);
     return testFile;
   }