Skip to content

Commit 9689b17

Browse files
sygCommit Bot
syg
authored and
Commit Bot
committed
[top-level-await] Implement spec fix for cycle root detection
There is a bug in the top-level await spec draft such that async strongly connected components are not always evaluated before their depending modules. See tc39/proposal-top-level-await#161 for full discussion and spec fix. Bug: v8:11376 Change-Id: I88bf06afb2e9a5d8d0b757de8276f1d1242a875e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2667772 Reviewed-by: Adam Klein <adamk@chromium.org> Reviewed-by: Camillo Bruni <cbruni@chromium.org> Commit-Queue: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/master@{#72508}
1 parent 64471ba commit 9689b17

12 files changed

Lines changed: 108 additions & 61 deletions

‎src/diagnostics/objects-printer.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1663,6 +1663,7 @@ void SourceTextModule::SourceTextModulePrint(std::ostream& os) { // NOLINT
16631663
os << "\n - origin: " << Brief(script.GetNameOrSourceURL());
16641664
os << "\n - requested_modules: " << Brief(requested_modules());
16651665
os << "\n - import_meta: " << Brief(import_meta());
1666+
os << "\n - cycle_root: " << Brief(cycle_root());
16661667
os << "\n";
16671668
}
16681669

‎src/heap/factory.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2342,6 +2342,7 @@ Handle<SourceTextModule> Factory::NewSourceTextModule(
23422342
module->set_flags(0);
23432343
module->set_async(IsAsyncModule(sfi->kind()));
23442344
module->set_async_evaluating(false);
2345+
module->set_cycle_root(roots.the_hole_value());
23452346
module->set_async_parent_modules(*async_parent_modules);
23462347
module->set_pending_async_dependencies(0);
23472348
return module;

‎src/objects/module-inl.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,14 @@ class UnorderedModuleSet
111111
ZoneAllocator<Handle<Module>>(zone)) {}
112112
};
113113

114+
Handle<SourceTextModule> SourceTextModule::GetCycleRoot(
115+
Isolate* isolate) const {
116+
CHECK_GE(status(), kEvaluated);
117+
DCHECK(!cycle_root().IsTheHole(isolate));
118+
Handle<SourceTextModule> root(SourceTextModule::cast(cycle_root()), isolate);
119+
return root;
120+
}
121+
114122
void SourceTextModule::AddAsyncParentModule(Isolate* isolate,
115123
Handle<SourceTextModule> module,
116124
Handle<SourceTextModule> parent) {

‎src/objects/source-text-module.cc‎

Lines changed: 14 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,7 @@ bool SourceTextModule::MaybeTransitionComponent(
421421
DCHECK_LE(module->dfs_ancestor_index(), module->dfs_index());
422422
if (module->dfs_ancestor_index() == module->dfs_index()) {
423423
// This is the root of its strongly connected component.
424+
Handle<SourceTextModule> cycle_root = module;
424425
Handle<SourceTextModule> ancestor;
425426
do {
426427
ancestor = stack->front();
@@ -430,6 +431,9 @@ bool SourceTextModule::MaybeTransitionComponent(
430431
if (new_status == kInstantiated) {
431432
if (!SourceTextModule::RunInitializationCode(isolate, ancestor))
432433
return false;
434+
} else if (new_status == kEvaluated) {
435+
DCHECK(ancestor->cycle_root().IsTheHole(isolate));
436+
ancestor->set_cycle_root(*cycle_root);
433437
}
434438
ancestor->SetStatus(new_status);
435439
} while (*ancestor != *module);
@@ -642,9 +646,9 @@ MaybeHandle<Object> SourceTextModule::EvaluateMaybeAsync(
642646
CHECK(module->status() == kInstantiated || module->status() == kEvaluated);
643647

644648
// 3. If module.[[Status]] is "evaluated", set module to
645-
// GetAsyncCycleRoot(module).
649+
// module.[[CycleRoot]].
646650
if (module->status() == kEvaluated) {
647-
module = GetAsyncCycleRoot(isolate, module);
651+
module = module->GetCycleRoot(isolate);
648652
}
649653

650654
// 4. If module.[[TopLevelCapability]] is not undefined, then
@@ -759,37 +763,27 @@ void SourceTextModule::AsyncModuleExecutionFulfilled(
759763
for (int i = 0; i < module->AsyncParentModuleCount(); i++) {
760764
Handle<SourceTextModule> m = module->GetAsyncParentModule(isolate, i);
761765

762-
// a. If module.[[DFSIndex]] is not equal to module.[[DFSAncestorIndex]],
763-
// then
764-
if (module->dfs_index() != module->dfs_ancestor_index()) {
765-
// i. Assert: m.[[DFSAncestorIndex]] is equal to
766-
// module.[[DFSAncestorIndex]].
767-
DCHECK_LE(m->dfs_ancestor_index(), module->dfs_ancestor_index());
768-
}
769-
// b. Decrement m.[[PendingAsyncDependencies]] by 1.
766+
// a. Decrement m.[[PendingAsyncDependencies]] by 1.
770767
m->DecrementPendingAsyncDependencies();
771768

772-
// c. If m.[[PendingAsyncDependencies]] is 0 and m.[[EvaluationError]] is
769+
// b. If m.[[PendingAsyncDependencies]] is 0 and m.[[EvaluationError]] is
773770
// undefined, then
774771
if (!m->HasPendingAsyncDependencies() && m->status() == kEvaluated) {
775772
// i. Assert: m.[[AsyncEvaluating]] is true.
776773
DCHECK(m->async_evaluating());
777774

778-
// ii. Let cycleRoot be ! GetAsyncCycleRoot(m).
779-
auto cycle_root = GetAsyncCycleRoot(isolate, m);
780-
781-
// iii. If cycleRoot.[[EvaluationError]] is not undefined,
775+
// ii. If m.[[CycleRoot]].[[EvaluationError]] is not undefined,
782776
// return undefined.
783-
if (cycle_root->status() == kErrored) {
777+
if (m->GetCycleRoot(isolate)->status() == kErrored) {
784778
return;
785779
}
786780

787-
// iv. If m.[[Async]] is true, then
781+
// iii. If m.[[Async]] is true, then
788782
if (m->async()) {
789783
// 1. Perform ! ExecuteAsyncModule(m).
790784
ExecuteAsyncModule(isolate, m);
791785
} else {
792-
// v. Otherwise,
786+
// iv. Otherwise,
793787
// 1. Let result be m.ExecuteModule().
794788
// 2. If result is a normal completion,
795789
Handle<Object> unused_result;
@@ -1069,8 +1063,8 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
10691063
required_module->dfs_ancestor_index()));
10701064
} else {
10711065
// iv. Otherwise,
1072-
// 1. Set requiredModule to GetAsyncCycleRoot(requiredModule).
1073-
required_module = GetAsyncCycleRoot(isolate, required_module);
1066+
// 1. Set requiredModule to requiredModule.[[CycleRoot]].
1067+
required_module = required_module->GetCycleRoot(isolate);
10741068

10751069
// 2. Assert: requiredModule.[[Status]] is "evaluated".
10761070
CHECK_GE(required_module->status(), kEvaluated);
@@ -1128,43 +1122,6 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
11281122
return result;
11291123
}
11301124

1131-
Handle<SourceTextModule> SourceTextModule::GetAsyncCycleRoot(
1132-
Isolate* isolate, Handle<SourceTextModule> module) {
1133-
// 1. Assert: module.[[Status]] is "evaluated".
1134-
CHECK_GE(module->status(), kEvaluated);
1135-
1136-
// 2. If module.[[AsyncParentModules]] is an empty List, return module.
1137-
if (module->AsyncParentModuleCount() == 0) {
1138-
return module;
1139-
}
1140-
1141-
// 3. Repeat, while module.[[DFSIndex]] is greater than
1142-
// module.[[DFSAncestorIndex]],
1143-
while (module->dfs_index() > module->dfs_ancestor_index()) {
1144-
// a. Assert: module.[[AsyncParentModules]] is a non-empty List.
1145-
DCHECK_GT(module->AsyncParentModuleCount(), 0);
1146-
1147-
// b. Let nextCycleModule be the first element of
1148-
// module.[[AsyncParentModules]].
1149-
Handle<SourceTextModule> next_cycle_module =
1150-
module->GetAsyncParentModule(isolate, 0);
1151-
1152-
// c. Assert: nextCycleModule.[[DFSAncestorIndex]] is less than or equal
1153-
// to module.[[DFSAncestorIndex]].
1154-
DCHECK_LE(next_cycle_module->dfs_ancestor_index(),
1155-
module->dfs_ancestor_index());
1156-
1157-
// d. Set module to nextCycleModule
1158-
module = next_cycle_module;
1159-
}
1160-
1161-
// 4. Assert: module.[[DFSIndex]] is equal to module.[[DFSAncestorIndex]].
1162-
DCHECK_EQ(module->dfs_index(), module->dfs_ancestor_index());
1163-
1164-
// 5. Return module.
1165-
return module;
1166-
}
1167-
11681125
void SourceTextModule::Reset(Isolate* isolate,
11691126
Handle<SourceTextModule> module) {
11701127
Factory* factory = isolate->factory();

‎src/objects/source-text-module.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ class SourceTextModule
8383
Handle<SourceTextModule> module,
8484
Handle<SourceTextModule> parent);
8585

86+
// Get the non-hole cycle root. Only valid when status >= kEvaluated.
87+
inline Handle<SourceTextModule> GetCycleRoot(Isolate* isolate) const;
88+
8689
// Returns a SourceTextModule, the
8790
// ith parent in depth first traversal order of a given async child.
8891
inline Handle<SourceTextModule> GetAsyncParentModule(Isolate* isolate,
@@ -169,10 +172,6 @@ class SourceTextModule
169172
Isolate* isolate, Handle<SourceTextModule> module,
170173
ZoneForwardList<Handle<SourceTextModule>>* stack, Status new_status);
171174

172-
// Implementation of spec GetAsyncCycleRoot.
173-
static V8_WARN_UNUSED_RESULT Handle<SourceTextModule> GetAsyncCycleRoot(
174-
Isolate* isolate, Handle<SourceTextModule> module);
175-
176175
// Implementation of spec ExecuteModule is broken up into
177176
// InnerExecuteAsyncModule for asynchronous modules and ExecuteModule
178177
// for synchronous modules.

‎src/objects/source-text-module.tq‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@ extern class SourceTextModule extends Module {
3131
// a JSObject afterwards.
3232
import_meta: TheHole|JSObject;
3333

34+
// The first visited module of a cycle. For modules not in a cycle, this is
35+
// the module itself. It's the hole before the module state transitions to
36+
// kEvaluated.
37+
cycle_root: SourceTextModule|TheHole;
38+
3439
async_parent_modules: ArrayList;
3540
top_level_capability: JSPromise|Undefined;
3641

‎test/mjsunit/harmony/modules-import-rqstd-order-async-cycle.mjs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-top-level-await
6+
7+
import "modules-skip-async-cycle-start.mjs"
8+
9+
assertEquals(globalThis.test_order, [
10+
'2', 'async before', 'async after', '1',
11+
'3', 'start',
12+
]);

‎test/mjsunit/harmony/modules-skip-async-cycle-1.mjs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-top-level-await
6+
7+
import "modules-skip-async-cycle-2.mjs";
8+
import "modules-skip-async-cycle-leaf.mjs";
9+
10+
if (globalThis.test_order === undefined) {
11+
globalThis.test_order = [];
12+
}
13+
globalThis.test_order.push('1');
14+

‎test/mjsunit/harmony/modules-skip-async-cycle-2.mjs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-top-level-await
6+
7+
import "modules-skip-async-cycle-1.mjs";
8+
9+
if (globalThis.test_order === undefined) {
10+
globalThis.test_order = [];
11+
}
12+
globalThis.test_order.push('2');

‎test/mjsunit/harmony/modules-skip-async-cycle-3.mjs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-top-level-await
6+
7+
import "modules-skip-async-cycle-2.mjs";
8+
9+
if (globalThis.test_order === undefined) {
10+
globalThis.test_order = [];
11+
}
12+
globalThis.test_order.push('3');

0 commit comments

Comments
 (0)