Skip to content

Commit cb6ed67

Browse files
bkonyicommit-bot@chromium.org
bkonyi
authored and
[email protected]
committed
[ 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 <[email protected]> Reviewed-by: Siva Annamalai <[email protected]>
1 parent da663d6 commit cb6ed67

File tree

7 files changed

+246
-8
lines changed

7 files changed

+246
-8
lines changed

‎pkg/dartdev/test/smoke/implicit_smoke_test.dart‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'dart:io';
6+
7+
import 'package:test/test.dart';
8+
9+
const numRuns = 10;
10+
final script = Platform.script.resolve('smoke.dart').toString();
11+
12+
void main() {
13+
group(
14+
'implicit dartdev smoke -',
15+
() {
16+
test('dart smoke.dart', () async {
17+
for (int i = 1; i <= numRuns; ++i) {
18+
if (i % 5 == 0) {
19+
print('Done [$i/$numRuns]');
20+
}
21+
final result = await Process.run(
22+
Platform.executable,
23+
[
24+
script,
25+
],
26+
);
27+
expect(result.exitCode, 0);
28+
expect(result.stdout, contains('Smoke test!'));
29+
expect(result.stderr, isEmpty);
30+
}
31+
});
32+
33+
// This test forces dartdev to run implicitly and for
34+
// DDS to spawn in a separate process.
35+
test('dart --enable-vm-service smoke.dart', () async {
36+
for (int i = 1; i <= numRuns; ++i) {
37+
if (i % 5 == 0) {
38+
print('Done [$i/$numRuns]');
39+
}
40+
final result = await Process.run(
41+
Platform.executable,
42+
[
43+
'--enable-vm-service=0',
44+
script,
45+
],
46+
);
47+
expect(result.exitCode, 0);
48+
expect(result.stdout, contains('Smoke test!'));
49+
expect(result.stderr, isEmpty);
50+
}
51+
});
52+
},
53+
timeout: Timeout.none,
54+
);
55+
}

‎pkg/dartdev/test/smoke/invalid_smoke_test.dart‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'dart:io';
6+
7+
import 'package:test/test.dart';
8+
9+
const numRuns = 10;
10+
final script = Platform.script.resolve('smoke.dart').toString();
11+
12+
void main() {
13+
group(
14+
'implicit dartdev smoke -',
15+
() {
16+
test('dart invalid.dart', () async {
17+
for (int i = 1; i <= numRuns; ++i) {
18+
if (i % 5 == 0) {
19+
print('Done [$i/$numRuns]');
20+
}
21+
final result = await Process.run(
22+
Platform.executable,
23+
[
24+
'invalid.dart',
25+
],
26+
);
27+
expect(result.exitCode, 64);
28+
expect(result.stdout, isEmpty);
29+
expect(
30+
result.stderr,
31+
contains(
32+
'Error when reading \'invalid.dart\':'
33+
' No such file or directory',
34+
),
35+
);
36+
}
37+
});
38+
39+
// This test forces dartdev to run implicitly and for
40+
// DDS to spawn in a separate process..
41+
test('dart --enable-vm-service invalid.dart', () async {
42+
for (int i = 1; i <= numRuns; ++i) {
43+
if (i % 5 == 0) {
44+
print('Done [$i/$numRuns]');
45+
}
46+
final result = await Process.run(
47+
Platform.executable,
48+
[
49+
'--enable-vm-service=0',
50+
'invalid.dart',
51+
],
52+
);
53+
expect(result.exitCode, 64);
54+
expect(result.stdout, contains('Observatory listening'));
55+
expect(
56+
result.stderr,
57+
contains(
58+
'Error when reading \'invalid.dart\':'
59+
' No such file or directory',
60+
),
61+
);
62+
}
63+
});
64+
65+
test('dart run invalid.dart', () async {
66+
for (int i = 1; i <= numRuns; ++i) {
67+
if (i % 5 == 0) {
68+
print('Done [$i/$numRuns]');
69+
}
70+
final result = await Process.run(
71+
Platform.executable,
72+
[
73+
'run',
74+
'invalid.dart',
75+
],
76+
);
77+
expect(result.exitCode, 254);
78+
expect(result.stdout, isEmpty);
79+
expect(
80+
result.stderr,
81+
contains(
82+
'Error when reading \'invalid.dart\':'
83+
' No such file or directory',
84+
),
85+
);
86+
}
87+
});
88+
89+
// This test forces DDS to spawn in a separate process.
90+
test('dart run --enable-vm-service invalid.dart', () async {
91+
for (int i = 1; i <= numRuns; ++i) {
92+
if (i % 5 == 0) {
93+
print('Done [$i/$numRuns]');
94+
}
95+
final result = await Process.run(
96+
Platform.executable,
97+
[
98+
'run',
99+
'--enable-vm-service=0',
100+
'invalid.dart',
101+
],
102+
);
103+
expect(result.exitCode, 254);
104+
expect(result.stdout, contains('Observatory listening'));
105+
expect(
106+
result.stderr,
107+
contains(
108+
'Error when reading \'invalid.dart\':'
109+
' No such file or directory',
110+
),
111+
);
112+
}
113+
});
114+
},
115+
timeout: Timeout.none,
116+
// TODO(bkonyi): Fails consistently on bots, need to investigate.
117+
skip: true,
118+
);
119+
}

‎pkg/dartdev/test/smoke/smoke.dart‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
void main() {
6+
print('Smoke test!');
7+
}

‎pkg/dartdev/test/smoke/smoke_test.dart‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'dart:io';
6+
7+
import 'package:test/test.dart';
8+
9+
const numRuns = 10;
10+
final script = Platform.script.resolve('smoke.dart').toString();
11+
12+
void main() {
13+
group(
14+
'explicit dartdev smoke -',
15+
() {
16+
test('dart run smoke.dart', () async {
17+
for (int i = 1; i <= numRuns; ++i) {
18+
if (i % 5 == 0) {
19+
print('Done [$i/$numRuns]');
20+
}
21+
final result = await Process.run(
22+
Platform.executable,
23+
[
24+
'run',
25+
script,
26+
],
27+
);
28+
expect(result.exitCode, 0);
29+
expect(result.stdout, contains('Smoke test!'));
30+
expect(result.stderr, isEmpty);
31+
}
32+
});
33+
34+
// This test forces DDS to spawn in a separate process.
35+
test('dart run --enable-vm-service smoke.dart', () async {
36+
for (int i = 1; i <= numRuns; ++i) {
37+
if (i % 5 == 0) {
38+
print('Done [$i/$numRuns]');
39+
}
40+
final result = await Process.run(
41+
Platform.executable,
42+
[
43+
'run',
44+
'--enable-vm-service=0',
45+
script,
46+
],
47+
);
48+
expect(result.exitCode, 0);
49+
expect(result.stdout, contains('Smoke test!'));
50+
expect(result.stderr, isEmpty);
51+
}
52+
});
53+
},
54+
timeout: Timeout.none,
55+
);
56+
}

‎pkg/pkg.status‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ compiler/test/sourcemaps/source_mapping_test: Slow, Pass
6262
compiler/test/sourcemaps/stacktrace_test: Slow, Pass
6363
dartdev/test/commands/analyze_test: Slow, Pass
6464
dartdev/test/commands/help_test: Slow, Pass
65+
dartdev/test/smoke/*: Slow, Pass
6566
dev_compiler/test/modular/*: Slow, Pass
6667
dev_compiler/test/options/*: Skip # test needs fixes
6768
dev_compiler/test/sourcemap/*: SkipByDesign # Skip sourcemap tests

‎runtime/bin/dartdev_isolate.cc‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,28 +56,28 @@ bool DartDevIsolate::ShouldParseCommand(const char* script_uri) {
5656
(strncmp(script_uri, "google3://", 10) != 0));
5757
}
5858

59-
const char* DartDevIsolate::TryResolveDartDevSnapshotPath() {
59+
Utils::CStringUniquePtr DartDevIsolate::TryResolveDartDevSnapshotPath() {
6060
// |dir_prefix| includes the last path seperator.
6161
auto dir_prefix = EXEUtils::GetDirectoryPrefixFromExeName();
6262

6363
// First assume we're in dart-sdk/bin.
6464
char* snapshot_path =
6565
Utils::SCreate("%ssnapshots/dartdev.dart.snapshot", dir_prefix.get());
6666
if (File::Exists(nullptr, snapshot_path)) {
67-
return snapshot_path;
67+
return Utils::CreateCStringUniquePtr(snapshot_path);
6868
}
6969
free(snapshot_path);
7070

7171
// If we're not in dart-sdk/bin, we might be in one of the $SDK/out/*
7272
// directories. Try to use a snapshot rom a previously built SDK.
7373
snapshot_path = Utils::SCreate("%sdartdev.dart.snapshot", dir_prefix.get());
7474
if (File::Exists(nullptr, snapshot_path)) {
75-
return snapshot_path;
75+
return Utils::CreateCStringUniquePtr(snapshot_path);
7676
}
7777
free(snapshot_path);
7878

7979
Syslog::PrintErr("Could not find DartDev snapshot.\n");
80-
return nullptr;
80+
return Utils::CreateCStringUniquePtr(nullptr);
8181
}
8282

8383
void DartDevIsolate::DartDevRunner::Run(
@@ -176,7 +176,7 @@ void DartDevIsolate::DartDevRunner::RunCallback(uword args) {
176176

177177
// TODO(bkonyi): bring up DartDev from kernel instead of a app-jit snapshot.
178178
// See https://github.com/dart-lang/sdk/issues/42804
179-
const char* dartdev_path = DartDevIsolate::TryResolveDartDevSnapshotPath();
179+
auto dartdev_path = DartDevIsolate::TryResolveDartDevSnapshotPath();
180180
if (dartdev_path == nullptr) {
181181
ProcessError("Failed to find DartDev snapshot.", kErrorExitCode);
182182
return;
@@ -192,9 +192,8 @@ void DartDevIsolate::DartDevRunner::RunCallback(uword args) {
192192

193193
char* error;
194194
Dart_Isolate dartdev_isolate = runner->create_isolate_(
195-
dartdev_path, "dartdev", nullptr, runner->packages_file_, &flags,
195+
dartdev_path.get(), "dartdev", nullptr, runner->packages_file_, &flags,
196196
NULL /* callback_data */, const_cast<char**>(&error));
197-
free(const_cast<char*>(dartdev_path));
198197

199198
if (dartdev_isolate == nullptr) {
200199
ProcessError(error, kErrorExitCode);

‎runtime/bin/dartdev_isolate.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "include/dart_api.h"
1414
#include "include/dart_native_api.h"
1515
#include "platform/globals.h"
16+
#include "platform/utils.h"
1617

1718
namespace dart {
1819
namespace bin {
@@ -83,7 +84,7 @@ class DartDevIsolate {
8384
private:
8485
// Attempts to find the DartDev snapshot. If the snapshot cannot be found,
8586
// the VM will shutdown.
86-
static const char* TryResolveDartDevSnapshotPath();
87+
static Utils::CStringUniquePtr TryResolveDartDevSnapshotPath();
8788

8889
static DartDevRunner runner_;
8990
static bool should_run_dart_dev_;

0 commit comments

Comments
 (0)