[ VM ] Fix issue where dartdev's script_uri was being freed prematurely

This was causing flaky failures when initializing DDS as it was invoking
the getVM RPC which in turn sometimes accessed the script_uri after it
had been freed.

Change-Id: I4454b6fa2da3ad6767938ed12b1013223a667af7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155740
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

pkg/dartdev/test/smoke/implicit_smoke_test.dart

@@ -0,0 +1,55 @@
+// Copyright (c) 2020, 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:io';
+
+import 'package:test/test.dart';
+
+const numRuns = 10;
+final script = Platform.script.resolve('smoke.dart').toString();
+
+void main() {
+  group(
+    'implicit dartdev smoke -',
+    () {
+      test('dart smoke.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              script,
+            ],
+          );
+          expect(result.exitCode, 0);
+          expect(result.stdout, contains('Smoke test!'));
+          expect(result.stderr, isEmpty);
+        }
+      });
+
+      // This test forces dartdev to run implicitly and for
+      // DDS to spawn in a separate process.
+      test('dart --enable-vm-service smoke.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              '--enable-vm-service=0',
+              script,
+            ],
+          );
+          expect(result.exitCode, 0);
+          expect(result.stdout, contains('Smoke test!'));
+          expect(result.stderr, isEmpty);
+        }
+      });
+    },
+    timeout: Timeout.none,
+  );
+}

pkg/dartdev/test/smoke/invalid_smoke_test.dart

@@ -0,0 +1,119 @@
+// Copyright (c) 2020, 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:io';
+
+import 'package:test/test.dart';
+
+const numRuns = 10;
+final script = Platform.script.resolve('smoke.dart').toString();
+
+void main() {
+  group(
+    'implicit dartdev smoke -',
+    () {
+      test('dart invalid.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              'invalid.dart',
+            ],
+          );
+          expect(result.exitCode, 64);
+          expect(result.stdout, isEmpty);
+          expect(
+            result.stderr,
+            contains(
+              'Error when reading \'invalid.dart\':'
+              ' No such file or directory',
+            ),
+          );
+        }
+      });
+
+      // This test forces dartdev to run implicitly and for
+      // DDS to spawn in a separate process..
+      test('dart --enable-vm-service invalid.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              '--enable-vm-service=0',
+              'invalid.dart',
+            ],
+          );
+          expect(result.exitCode, 64);
+          expect(result.stdout, contains('Observatory listening'));
+          expect(
+            result.stderr,
+            contains(
+              'Error when reading \'invalid.dart\':'
+              ' No such file or directory',
+            ),
+          );
+        }
+      });
+
+      test('dart run invalid.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              'run',
+              'invalid.dart',
+            ],
+          );
+          expect(result.exitCode, 254);
+          expect(result.stdout, isEmpty);
+          expect(
+            result.stderr,
+            contains(
+              'Error when reading \'invalid.dart\':'
+              ' No such file or directory',
+            ),
+          );
+        }
+      });
+
+      // This test forces DDS to spawn in a separate process.
+      test('dart run --enable-vm-service invalid.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              'run',
+              '--enable-vm-service=0',
+              'invalid.dart',
+            ],
+          );
+          expect(result.exitCode, 254);
+          expect(result.stdout, contains('Observatory listening'));
+          expect(
+            result.stderr,
+            contains(
+              'Error when reading \'invalid.dart\':'
+              ' No such file or directory',
+            ),
+          );
+        }
+      });
+    },
+    timeout: Timeout.none,
+    // TODO(bkonyi): Fails consistently on bots, need to investigate.
+    skip: true,
+  );
+}

pkg/dartdev/test/smoke/smoke.dart

@@ -0,0 +1,7 @@
+// Copyright (c) 2020, 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.
+
+void main() {
+  print('Smoke test!');
+}

pkg/dartdev/test/smoke/smoke_test.dart

@@ -0,0 +1,56 @@
+// Copyright (c) 2020, 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:io';
+
+import 'package:test/test.dart';
+
+const numRuns = 10;
+final script = Platform.script.resolve('smoke.dart').toString();
+
+void main() {
+  group(
+    'explicit dartdev smoke -',
+    () {
+      test('dart run smoke.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              'run',
+              script,
+            ],
+          );
+          expect(result.exitCode, 0);
+          expect(result.stdout, contains('Smoke test!'));
+          expect(result.stderr, isEmpty);
+        }
+      });
+
+      // This test forces DDS to spawn in a separate process.
+      test('dart run --enable-vm-service smoke.dart', () async {
+        for (int i = 1; i <= numRuns; ++i) {
+          if (i % 5 == 0) {
+            print('Done [$i/$numRuns]');
+          }
+          final result = await Process.run(
+            Platform.executable,
+            [
+              'run',
+              '--enable-vm-service=0',
+              script,
+            ],
+          );
+          expect(result.exitCode, 0);
+          expect(result.stdout, contains('Smoke test!'));
+          expect(result.stderr, isEmpty);
+        }
+      });
+    },
+    timeout: Timeout.none,
+  );
+}

pkg/pkg.status

@@ -62,6 +62,7 @@ compiler/test/sourcemaps/source_mapping_test: Slow, Pass
 compiler/test/sourcemaps/stacktrace_test: Slow, Pass
 dartdev/test/commands/analyze_test: Slow, Pass
 dartdev/test/commands/help_test: Slow, Pass
+dartdev/test/smoke/*: Slow, Pass
 dev_compiler/test/modular/*: Slow, Pass
 dev_compiler/test/options/*: Skip # test needs fixes
 dev_compiler/test/sourcemap/*: SkipByDesign # Skip sourcemap tests

runtime/bin/dartdev_isolate.cc

@@ -56,7 +56,7 @@ bool DartDevIsolate::ShouldParseCommand(const char* script_uri) {
           (strncmp(script_uri, "google3://", 10) != 0));
 }
 
-const char* DartDevIsolate::TryResolveDartDevSnapshotPath() {
+Utils::CStringUniquePtr DartDevIsolate::TryResolveDartDevSnapshotPath() {
   // |dir_prefix| includes the last path seperator.
   auto dir_prefix = EXEUtils::GetDirectoryPrefixFromExeName();
 
@@ -64,7 +64,7 @@ const char* DartDevIsolate::TryResolveDartDevSnapshotPath() {
   char* snapshot_path =
       Utils::SCreate("%ssnapshots/dartdev.dart.snapshot", dir_prefix.get());
   if (File::Exists(nullptr, snapshot_path)) {
-    return snapshot_path;
+    return Utils::CreateCStringUniquePtr(snapshot_path);
   }
   free(snapshot_path);
 
@@ -72,12 +72,12 @@ const char* DartDevIsolate::TryResolveDartDevSnapshotPath() {
   // directories. Try to use a snapshot rom a previously built SDK.
   snapshot_path = Utils::SCreate("%sdartdev.dart.snapshot", dir_prefix.get());
   if (File::Exists(nullptr, snapshot_path)) {
-    return snapshot_path;
+    return Utils::CreateCStringUniquePtr(snapshot_path);
   }
   free(snapshot_path);
 
   Syslog::PrintErr("Could not find DartDev snapshot.\n");
-  return nullptr;
+  return Utils::CreateCStringUniquePtr(nullptr);
 }
 
 void DartDevIsolate::DartDevRunner::Run(
@@ -176,7 +176,7 @@ void DartDevIsolate::DartDevRunner::RunCallback(uword args) {
 
   // TODO(bkonyi): bring up DartDev from kernel instead of a app-jit snapshot.
   // See https://github.com/dart-lang/sdk/issues/42804
-  const char* dartdev_path = DartDevIsolate::TryResolveDartDevSnapshotPath();
+  auto dartdev_path = DartDevIsolate::TryResolveDartDevSnapshotPath();
   if (dartdev_path == nullptr) {
     ProcessError("Failed to find DartDev snapshot.", kErrorExitCode);
     return;
@@ -192,9 +192,8 @@ void DartDevIsolate::DartDevRunner::RunCallback(uword args) {
 
   char* error;
   Dart_Isolate dartdev_isolate = runner->create_isolate_(
-      dartdev_path, "dartdev", nullptr, runner->packages_file_, &flags,
+      dartdev_path.get(), "dartdev", nullptr, runner->packages_file_, &flags,
       NULL /* callback_data */, const_cast<char**>(&error));
-  free(const_cast<char*>(dartdev_path));
 
   if (dartdev_isolate == nullptr) {
     ProcessError(error, kErrorExitCode);

runtime/bin/dartdev_isolate.h

@@ -13,6 +13,7 @@
 #include "include/dart_api.h"
 #include "include/dart_native_api.h"
 #include "platform/globals.h"
+#include "platform/utils.h"
 
 namespace dart {
 namespace bin {
@@ -83,7 +84,7 @@ class DartDevIsolate {
  private:
   // Attempts to find the DartDev snapshot. If the snapshot cannot be found,
   // the VM will shutdown.
-  static const char* TryResolveDartDevSnapshotPath();
+  static Utils::CStringUniquePtr TryResolveDartDevSnapshotPath();
 
   static DartDevRunner runner_;
   static bool should_run_dart_dev_;