Fix issue with --unsafe-package-serialization

--unsafe-package-serialization saves the frontend_server from having to
re-serialize everything all the time.

Before this CL, though, things could go wrong:

If you had a situation where you had previously compiled a file that
depend on a package A, and where (some of) package A depends on
package B this would happen:
- All of package A was serialized together.
- All of package B was serialized together.

When later, you compile something that depend less on package A - namely
on only on parts that does not depend on package B, but is included in
the previously serialized package A, the following would happen:
- The new (non-package) libraries would be serialized.
- Package A serialization would be reused.

This is basically fine: Running the app would be fine, everything it
actually depend on is there.

However, if the VM is forced to compile everything it now also compiles
stuff that was included - but really unused - from package A - namely
also the libraries that depend on package B --- which is not included.

This CL changes the last part by also including package B.

The result is that even more unused libraries are included, but that
the VM can survive a forceful compile.

Note that the --unsafe-package-serialization is only used for tests,
so the "including even more unused" part is probably not a big deal,
and all in all there's still a big speed advantage to doing this.

Change-Id: Iac06ba6f40c2caaacce641c5853e9491496dce53
Reviewed-on: https://dart-review.googlesource.com/c/78541
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>

pkg/vm/lib/frontend_server.dart

@@ -506,6 +506,14 @@ class FrontendCompiler implements CompilerInterface {
   /// [writePackagesToSinkAndTrimComponent].
   Map<Uri, List<int>> cachedPackageLibraries = new Map<Uri, List<int>>();
 
+  /// Map of dependencies for already serialized dill data.
+  /// E.g. if blob1 dependents on blob2, but only using a single file from blob1
+  /// that does not dependent on blob2, blob2 would not be included leaving the
+  /// dill file in a weird state that could cause the VM to crash if asked to
+  /// forcefully compile everything. Used by
+  /// [writePackagesToSinkAndTrimComponent].
+  Map<Uri, List<Uri>> cachedPackageDependencies = new Map<Uri, List<Uri>>();
+
   writePackagesToSinkAndTrimComponent(
       Component deltaProgram, Sink<List<int>> ioSink) {
     if (deltaProgram == null) return;
@@ -528,12 +536,21 @@ class FrontendCompiler implements CompilerInterface {
 
     Map<String, List<Library>> newPackages = new Map<String, List<Library>>();
     Set<List<int>> alreadyAdded = new Set<List<int>>();
+
+    addDataAndDependentData(List<int> data, Uri uri) {
+      if (alreadyAdded.add(data)) {
+        ioSink.add(data);
+        // Now also add all dependencies.
+        for (Uri dep in cachedPackageDependencies[uri]) {
+          addDataAndDependentData(cachedPackageLibraries[dep], dep);
+        }
+      }
+    }
+
     for (Library lib in packageLibraries) {
       List<int> data = cachedPackageLibraries[lib.fileUri];
       if (data != null) {
-        if (alreadyAdded.add(data)) {
-          ioSink.add(data);
-        }
+        addDataAndDependentData(data, lib.fileUri);
       } else {
         String package = lib.importUri.pathSegments.first;
         newPackages[package] ??= <Library>[];
@@ -550,9 +567,28 @@ class FrontendCompiler implements CompilerInterface {
       ByteSink byteSink = new ByteSink();
       final BinaryPrinter printer = printerFactory.newBinaryPrinter(byteSink);
       printer.writeComponentFile(singleLibrary);
+
+      // Record things this package blob dependent on.
+      Set<Uri> libraryUris = new Set<Uri>();
+      for (Library lib in libraries) {
+        libraryUris.add(lib.fileUri);
+      }
+      Set<Uri> deps = new Set<Uri>();
+      for (Library lib in libraries) {
+        for (LibraryDependency dep in lib.dependencies) {
+          Library dependencyLibrary = dep.importedLibraryReference.asLibrary;
+          if (dependencyLibrary.importUri.scheme != "package") continue;
+          Uri dependencyLibraryUri =
+              dep.importedLibraryReference.asLibrary.fileUri;
+          if (libraryUris.contains(dependencyLibraryUri)) continue;
+          deps.add(dependencyLibraryUri);
+        }
+      }
+
       List<int> data = byteSink.builder.takeBytes();
       for (Library lib in libraries) {
         cachedPackageLibraries[lib.fileUri] = data;
+        cachedPackageDependencies[lib.fileUri] = new List<Uri>.from(deps);
       }
       ioSink.add(data);
     }

pkg/vm/test/frontend_server_test.dart

@@ -792,6 +792,128 @@ true
       inputStreamController.close();
     });
 
+    test('unsafe-package-serialization', () async {
+      // Package A.
+      var file = new File('${tempDir.path}/pkgA/a.dart')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("pkgA() {}");
+
+      // Package B.
+      file = new File('${tempDir.path}/pkgB/.packages')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("pkgA: ../pkgA");
+      file = new File('${tempDir.path}/pkgB/a.dart')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("pkgB_a() {}");
+      file = new File('${tempDir.path}/pkgB/b.dart')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("import 'package:pkgA/a.dart';"
+          "pkgB_b() { pkgA(); }");
+
+      // Application.
+      file = new File('${tempDir.path}/app/.packages')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("pkgA:../pkgA\n"
+          "pkgB:../pkgB");
+
+      // Entry point A uses both package A and B.
+      file = new File('${tempDir.path}/app/a.dart')
+        ..createSync(recursive: true);
+      file.writeAsStringSync("import 'package:pkgB/b.dart';"
+          "import 'package:pkgB/a.dart';"
+          "appA() { pkgB_a(); pkgB_b(); }");
+
+      // Entry point B uses only package B.
+      var fileB = new File('${tempDir.path}/app/B.dart')
+        ..createSync(recursive: true);
+      fileB.writeAsStringSync("import 'package:pkgB/a.dart';"
+          "appB() { pkgB_a(); }");
+
+      // Other setup.
+      var dillFile = new File('${tempDir.path}/app.dill');
+      expect(dillFile.existsSync(), equals(false));
+
+      // First compile app entry point A.
+      final List<String> args = <String>[
+        '--sdk-root=${sdkRoot.toFilePath()}',
+        '--strong',
+        '--incremental',
+        '--platform=${platformKernel.path}',
+        '--output-dill=${dillFile.path}',
+        '--unsafe-package-serialization',
+      ];
+
+      final StreamController<List<int>> inputStreamController =
+          new StreamController<List<int>>();
+      final StreamController<List<int>> stdoutStreamController =
+          new StreamController<List<int>>();
+      final IOSink ioSink = new IOSink(stdoutStreamController.sink);
+      StreamController<String> receivedResults = new StreamController<String>();
+
+      String boundaryKey;
+      stdoutStreamController.stream
+          .transform(utf8.decoder)
+          .transform(const LineSplitter())
+          .listen((String s) {
+        const String RESULT_OUTPUT_SPACE = 'result ';
+        if (boundaryKey == null) {
+          if (s.startsWith(RESULT_OUTPUT_SPACE)) {
+            boundaryKey = s.substring(RESULT_OUTPUT_SPACE.length);
+          }
+        } else {
+          if (s.startsWith(boundaryKey)) {
+            receivedResults.add(s.substring(boundaryKey.length + 1));
+            boundaryKey = null;
+          }
+        }
+      });
+      Future<int> result =
+          starter(args, input: inputStreamController.stream, output: ioSink);
+      inputStreamController.add('compile ${file.path}\n'.codeUnits);
+      int count = 0;
+      receivedResults.stream.listen((String outputFilenameAndErrorCount) {
+        CompilationResult result =
+            new CompilationResult.parse(outputFilenameAndErrorCount);
+        switch (count) {
+          case 0:
+            expect(dillFile.existsSync(), equals(true));
+            expect(result.filename, dillFile.path);
+            expect(result.errorsCount, 0);
+            count += 1;
+            inputStreamController.add('accept\n'.codeUnits);
+            inputStreamController.add('reset\n'.codeUnits);
+
+            inputStreamController.add('recompile ${fileB.path} abc\n'
+                '${fileB.path}\n'
+                'abc\n'
+                .codeUnits);
+            break;
+          case 1:
+            expect(result.filename, dillFile.path);
+            expect(result.errorsCount, 0);
+            inputStreamController.add('quit\n'.codeUnits);
+
+            // Loadable.
+            Component component = loadComponentFromBinary(dillFile.path);
+
+            // Contains (at least) the 2 files we want.
+            component.libraries
+                    .where((l) =>
+                        l.importUri.toString() == "package:pkgB/a.dart" ||
+                        l.fileUri.toString().contains(fileB.path))
+                    .length ==
+                2;
+
+            // Verifiable (together with the platform file).
+            component =
+                loadComponentFromBinary(platformKernel.toFilePath(), component);
+            verifyComponent(component);
+        }
+      });
+      expect(await result, 0);
+      inputStreamController.close();
+    });
+
     test('compile and recompile report non-zero error count', () async {
       var file = new File('${tempDir.path}/foo.dart')..createSync();
       file.writeAsStringSync("main() { foo(); bar(); }\n");