[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}

src/diagnostics/objects-printer.cc

@@ -1621,6 +1621,7 @@ void SourceTextModule::SourceTextModulePrint(std::ostream& os) {  // NOLINT
   os << "\n - requested_modules: " << Brief(requested_modules());
   os << "\n - script: " << Brief(script());
   os << "\n - import_meta: " << Brief(import_meta());
+  os << "\n - cycle_root: " << Brief(cycle_root());
   os << "\n";
 }
 

src/heap/factory.cc

@@ -2485,6 +2485,7 @@ Handle<SourceTextModule> Factory::NewSourceTextModule(
   module->set_flags(0);
   module->set_async(IsAsyncModule(code->kind()));
   module->set_async_evaluating(false);
+  module->set_cycle_root(roots.the_hole_value());
   module->set_async_parent_modules(*async_parent_modules);
   module->set_pending_async_dependencies(0);
   return module;

src/objects/module-inl.h

@@ -111,6 +111,14 @@ class UnorderedModuleSet
             ZoneAllocator<Handle<Module>>(zone)) {}
 };
 
+Handle<SourceTextModule> SourceTextModule::GetCycleRoot(
+    Isolate* isolate) const {
+  CHECK_GE(status(), kEvaluated);
+  DCHECK(!cycle_root().IsTheHole(isolate));
+  Handle<SourceTextModule> root(SourceTextModule::cast(cycle_root()), isolate);
+  return root;
+}
+
 void SourceTextModule::AddAsyncParentModule(Isolate* isolate,
                                             Handle<SourceTextModule> module,
                                             Handle<SourceTextModule> parent) {

src/objects/source-text-module.cc

@@ -398,6 +398,7 @@ bool SourceTextModule::MaybeTransitionComponent(
   DCHECK_LE(module->dfs_ancestor_index(), module->dfs_index());
   if (module->dfs_ancestor_index() == module->dfs_index()) {
     // This is the root of its strongly connected component.
+    Handle<SourceTextModule> cycle_root = module;
     Handle<SourceTextModule> ancestor;
     do {
       ancestor = stack->front();
@@ -407,6 +408,9 @@ bool SourceTextModule::MaybeTransitionComponent(
       if (new_status == kInstantiated) {
         if (!SourceTextModule::RunInitializationCode(isolate, ancestor))
           return false;
+      } else if (new_status == kEvaluated) {
+        DCHECK(ancestor->cycle_root().IsTheHole(isolate));
+        ancestor->set_cycle_root(*cycle_root);
       }
       ancestor->SetStatus(new_status);
     } while (*ancestor != *module);
@@ -617,9 +621,9 @@ MaybeHandle<Object> SourceTextModule::EvaluateMaybeAsync(
   CHECK(module->status() == kInstantiated || module->status() == kEvaluated);
 
   // 3. If module.[[Status]] is "evaluated", set module to
-  //    GetAsyncCycleRoot(module).
+  //    module.[[CycleRoot]].
   if (module->status() == kEvaluated) {
-    module = GetAsyncCycleRoot(isolate, module);
+    module = module->GetCycleRoot(isolate);
   }
 
   // 4. If module.[[TopLevelCapability]] is not undefined, then
@@ -734,37 +738,27 @@ void SourceTextModule::AsyncModuleExecutionFulfilled(
   for (int i = 0; i < module->AsyncParentModuleCount(); i++) {
     Handle<SourceTextModule> m = module->GetAsyncParentModule(isolate, i);
 
-    //  a. If module.[[DFSIndex]] is not equal to module.[[DFSAncestorIndex]],
-    //     then
-    if (module->dfs_index() != module->dfs_ancestor_index()) {
-      //   i. Assert: m.[[DFSAncestorIndex]] is equal to
-      //      module.[[DFSAncestorIndex]].
-      DCHECK_LE(m->dfs_ancestor_index(), module->dfs_ancestor_index());
-    }
-    //  b. Decrement m.[[PendingAsyncDependencies]] by 1.
+    //  a. Decrement m.[[PendingAsyncDependencies]] by 1.
     m->DecrementPendingAsyncDependencies();
 
-    //  c. If m.[[PendingAsyncDependencies]] is 0 and m.[[EvaluationError]] is
+    //  b. If m.[[PendingAsyncDependencies]] is 0 and m.[[EvaluationError]] is
     //     undefined, then
     if (!m->HasPendingAsyncDependencies() && m->status() == kEvaluated) {
       //   i. Assert: m.[[AsyncEvaluating]] is true.
       DCHECK(m->async_evaluating());
 
-      //  ii. Let cycleRoot be ! GetAsyncCycleRoot(m).
-      auto cycle_root = GetAsyncCycleRoot(isolate, m);
-
-      // iii. If cycleRoot.[[EvaluationError]] is not undefined,
+      // ii. If m.[[CycleRoot]].[[EvaluationError]] is not undefined,
       //      return undefined.
-      if (cycle_root->status() == kErrored) {
+      if (m->GetCycleRoot(isolate)->status() == kErrored) {
         return;
       }
 
-      //  iv. If m.[[Async]] is true, then
+      //  iii. If m.[[Async]] is true, then
       if (m->async()) {
         //    1. Perform ! ExecuteAsyncModule(m).
         ExecuteAsyncModule(isolate, m);
       } else {
-        // v. Otherwise,
+        // iv. Otherwise,
         //    1. Let result be m.ExecuteModule().
         //    2. If result is a normal completion,
         Handle<Object> unused_result;
@@ -1044,8 +1038,8 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
                      required_module->dfs_ancestor_index()));
       } else {
         //   iv. Otherwise,
-        //      1. Set requiredModule to GetAsyncCycleRoot(requiredModule).
-        required_module = GetAsyncCycleRoot(isolate, required_module);
+        //      1. Set requiredModule to requiredModule.[[CycleRoot]].
+        required_module = required_module->GetCycleRoot(isolate);
 
         //      2. Assert: requiredModule.[[Status]] is "evaluated".
         CHECK_GE(required_module->status(), kEvaluated);
@@ -1103,43 +1097,6 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
   return result;
 }
 
-Handle<SourceTextModule> SourceTextModule::GetAsyncCycleRoot(
-    Isolate* isolate, Handle<SourceTextModule> module) {
-  // 1. Assert: module.[[Status]] is "evaluated".
-  CHECK_GE(module->status(), kEvaluated);
-
-  // 2. If module.[[AsyncParentModules]] is an empty List, return module.
-  if (module->AsyncParentModuleCount() == 0) {
-    return module;
-  }
-
-  // 3. Repeat, while module.[[DFSIndex]] is greater than
-  //    module.[[DFSAncestorIndex]],
-  while (module->dfs_index() > module->dfs_ancestor_index()) {
-    //  a. Assert: module.[[AsyncParentModules]] is a non-empty List.
-    DCHECK_GT(module->AsyncParentModuleCount(), 0);
-
-    //  b. Let nextCycleModule be the first element of
-    //     module.[[AsyncParentModules]].
-    Handle<SourceTextModule> next_cycle_module =
-        module->GetAsyncParentModule(isolate, 0);
-
-    //  c. Assert: nextCycleModule.[[DFSAncestorIndex]] is less than or equal
-    //     to module.[[DFSAncestorIndex]].
-    DCHECK_LE(next_cycle_module->dfs_ancestor_index(),
-              module->dfs_ancestor_index());
-
-    //  d. Set module to nextCycleModule
-    module = next_cycle_module;
-  }
-
-  // 4. Assert: module.[[DFSIndex]] is equal to module.[[DFSAncestorIndex]].
-  DCHECK_EQ(module->dfs_index(), module->dfs_ancestor_index());
-
-  // 5. Return module.
-  return module;
-}
-
 void SourceTextModule::Reset(Isolate* isolate,
                              Handle<SourceTextModule> module) {
   Factory* factory = isolate->factory();

src/objects/source-text-module.h

@@ -78,6 +78,9 @@ class SourceTextModule
                                           Handle<SourceTextModule> module,
                                           Handle<SourceTextModule> parent);
 
+  // Get the non-hole cycle root. Only valid when status >= kEvaluated.
+  inline Handle<SourceTextModule> GetCycleRoot(Isolate* isolate) const;
+
   // Returns a SourceTextModule, the
   // ith parent in depth first traversal order of a given async child.
   inline Handle<SourceTextModule> GetAsyncParentModule(Isolate* isolate,
@@ -163,10 +166,6 @@ class SourceTextModule
       Isolate* isolate, Handle<SourceTextModule> module,
       ZoneForwardList<Handle<SourceTextModule>>* stack, Status new_status);
 
-  // Implementation of spec GetAsyncCycleRoot.
-  static V8_WARN_UNUSED_RESULT Handle<SourceTextModule> GetAsyncCycleRoot(
-      Isolate* isolate, Handle<SourceTextModule> module);
-
   // Implementation of spec ExecuteModule is broken up into
   // InnerExecuteAsyncModule for asynchronous modules and ExecuteModule
   // for synchronous modules.

src/objects/source-text-module.tq

@@ -29,6 +29,11 @@ extern class SourceTextModule extends Module {
   // a JSObject afterwards.
   import_meta: TheHole|JSObject;
 
+  // The first visited module of a cycle. For modules not in a cycle, this is
+  // the module itself. It's the hole before the module state transitions to
+  // kEvaluated.
+  cycle_root: SourceTextModule|TheHole;
+
   async_parent_modules: ArrayList;
   top_level_capability: JSPromise|Undefined;
 

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

@@ -0,0 +1,12 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+import "modules-skip-async-cycle-start.mjs"
+
+assertEquals(globalThis.test_order, [
+  '2', 'async before', 'async after', '1',
+  '3', 'start',
+]);

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

@@ -0,0 +1,14 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+import "modules-skip-async-cycle-2.mjs";
+import "modules-skip-async-cycle-leaf.mjs";
+
+if (globalThis.test_order === undefined) {
+  globalThis.test_order = [];
+}
+globalThis.test_order.push('1');
+

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

@@ -0,0 +1,12 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+import "modules-skip-async-cycle-1.mjs";
+
+if (globalThis.test_order === undefined) {
+  globalThis.test_order = [];
+}
+globalThis.test_order.push('2');

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

@@ -0,0 +1,12 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+import "modules-skip-async-cycle-2.mjs";
+
+if (globalThis.test_order === undefined) {
+  globalThis.test_order = [];
+}
+globalThis.test_order.push('3');

test/mjsunit/harmony/modules-skip-async-cycle-leaf.mjs

@@ -0,0 +1,13 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+if (globalThis.test_order === undefined) {
+  globalThis.test_order = [];
+}
+
+globalThis.test_order.push('async before');
+await 0;
+globalThis.test_order.push('async after');

test/mjsunit/harmony/modules-skip-async-cycle-start.mjs

@@ -0,0 +1,13 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-top-level-await
+
+import "modules-skip-async-cycle-1.mjs";
+import "modules-skip-async-cycle-3.mjs";
+
+if (globalThis.test_order === undefined) {
+  globalThis.test_order = [];
+}
+globalThis.test_order.push('start');