[top-level-await] fix async evaluation with errored async parent

devsnek · V8 LUCI CQ · commit 5af53ec82c0f · 2024-08-27T08:35:50.000Z
Updates a CHECK to support EVALUATING-ASYNC modules. Updates GatherAvailableAncestors to check kErrored before calling AsyncEvaluationOrdinalCompare to prevent hitting DCHECK. Change-Id: If561751092010a36706c8c15fa977953facd5c3e Bug: 361615555 Bug: 361603701 Bug: 361611791 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5807477 Reviewed-by: Shu-yu Guo <syg@chromium.org> Commit-Queue: snek <snek@chromium.org> Cr-Commit-Position: refs/heads/main@{#95823}
diff --git a/src/d8/d8.cc b/src/d8/d8.cc
@@ -1269,34 +1269,30 @@ struct DynamicImportData {
 };
 
 namespace {
-struct ModuleResolutionData {
-  ModuleResolutionData(Isolate* isolate_, Local<Value> module_namespace_,
-                       Local<Promise::Resolver> resolver_)
-      : isolate(isolate_) {
-    module_namespace.Reset(isolate, module_namespace_);
-    resolver.Reset(isolate, resolver_);
-  }
 
-  Isolate* isolate;
-  Global<Value> module_namespace;
-  Global<Promise::Resolver> resolver;
+enum ModuleResolutionDataIndex : uint32_t {
+  kResolver = 0,
+  kNamespace = 1,
 };
 
 }  // namespace
 
 void Shell::ModuleResolutionSuccessCallback(
     const FunctionCallbackInfo<Value>& info) {
   DCHECK(i::ValidateCallbackInfo(info));
-  std::unique_ptr<ModuleResolutionData> module_resolution_data(
-      static_cast<ModuleResolutionData*>(
-          info.Data().As<v8::External>()->Value()));
-  Isolate* isolate(module_resolution_data->isolate);
+  Isolate* isolate(info.GetIsolate());
   HandleScope handle_scope(isolate);
+  Local<Array> module_resolution_data(info.Data().As<Array>());
+  Local<Context> context(isolate->GetCurrentContext());
 
   Local<Promise::Resolver> resolver(
-      module_resolution_data->resolver.Get(isolate));
+      module_resolution_data->Get(context, ModuleResolutionDataIndex::kResolver)
+          .ToLocalChecked()
+          .As<Promise::Resolver>());
   Local<Value> module_namespace(
-      module_resolution_data->module_namespace.Get(isolate));
+      module_resolution_data
+          ->Get(context, ModuleResolutionDataIndex::kNamespace)
+          .ToLocalChecked());
 
   PerIsolateData* data = PerIsolateData::Get(isolate);
   Local<Context> realm = data->realms_[data->realm_current_].Get(isolate);
@@ -1308,14 +1304,15 @@ void Shell::ModuleResolutionSuccessCallback(
 void Shell::ModuleResolutionFailureCallback(
     const FunctionCallbackInfo<Value>& info) {
   DCHECK(i::ValidateCallbackInfo(info));
-  std::unique_ptr<ModuleResolutionData> module_resolution_data(
-      static_cast<ModuleResolutionData*>(
-          info.Data().As<v8::External>()->Value()));
-  Isolate* isolate(module_resolution_data->isolate);
+  Isolate* isolate(info.GetIsolate());
   HandleScope handle_scope(isolate);
+  Local<Array> module_resolution_data(info.Data().As<Array>());
+  Local<Context> context(isolate->GetCurrentContext());
 
   Local<Promise::Resolver> resolver(
-      module_resolution_data->resolver.Get(isolate));
+      module_resolution_data->Get(context, ModuleResolutionDataIndex::kResolver)
+          .ToLocalChecked()
+          .As<Promise::Resolver>());
 
   PerIsolateData* data = PerIsolateData::Get(isolate);
   Local<Context> realm = data->realms_[data->realm_current_].Get(isolate);
@@ -1493,15 +1490,20 @@ void Shell::DoHostImportModuleDynamically(void* import_data) {
   Local<Promise> result_promise = result.As<Promise>();
 
   // Setup callbacks, and then chain them to the result promise.
-  // ModuleResolutionData will be deleted by the callbacks.
-  auto module_resolution_data =
-      new ModuleResolutionData(isolate, module_namespace, resolver);
-  Local<v8::External> edata = External::New(isolate, module_resolution_data);
+  Local<Array> module_resolution_data = v8::Array::New(isolate);
+  module_resolution_data
+      ->Set(realm, ModuleResolutionDataIndex::kResolver, resolver)
+      .ToChecked();
+  module_resolution_data
+      ->Set(realm, ModuleResolutionDataIndex::kNamespace, module_namespace)
+      .ToChecked();
   Local<Function> callback_success;
-  CHECK(Function::New(realm, ModuleResolutionSuccessCallback, edata)
+  CHECK(Function::New(realm, ModuleResolutionSuccessCallback,
+                      module_resolution_data)
             .ToLocal(&callback_success));
   Local<Function> callback_failure;
-  CHECK(Function::New(realm, ModuleResolutionFailureCallback, edata)
+  CHECK(Function::New(realm, ModuleResolutionFailureCallback,
+                      module_resolution_data)
             .ToLocal(&callback_failure));
   result_promise->Then(realm, callback_success, callback_failure)
       .ToLocalChecked();
diff --git a/src/objects/module.cc b/src/objects/module.cc
@@ -218,7 +218,7 @@ bool Module::Instantiate(Isolate* isolate, Handle<Module> module,
     return false;
   }
   DCHECK(module->status() == kLinked || module->status() == kEvaluated ||
-         module->status() == kErrored);
+         module->status() == kEvaluatingAsync || module->status() == kErrored);
   DCHECK(stack.empty());
   return true;
 }
diff --git a/src/objects/source-text-module.cc b/src/objects/source-text-module.cc
@@ -682,8 +682,8 @@ void SourceTextModule::GatherAvailableAncestors(
 
       // a. If execList does not contain m and
       //    m.[[CycleRoot]].[[EvaluationError]] is empty, then
-      if (exec_list->find(m) == exec_list->end() &&
-          m->GetCycleRoot(isolate)->status() != kErrored) {
+      if (m->GetCycleRoot(isolate)->status() != kErrored &&
+          exec_list->find(m) == exec_list->end()) {
         // i. Assert: m.[[Status]] is EVALUATING-ASYNC.
         // ii. Assert: m.[[EvaluationError]] is empty.
         DCHECK_EQ(m->status(), kEvaluatingAsync);
@@ -769,7 +769,8 @@ bool SourceTextModule::MaybeHandleEvaluationException(
 // ES#sec-moduleevaluation
 MaybeHandle<Object> SourceTextModule::Evaluate(
     Isolate* isolate, Handle<SourceTextModule> module) {
-  CHECK(module->status() == kLinked || module->status() == kEvaluated);
+  CHECK(module->status() == kLinked || module->status() == kEvaluatingAsync ||
+        module->status() == kEvaluated);
 
   // 5. Let stack be a new empty List.
   Zone zone(isolate->allocator(), ZONE_NAME);
diff --git a/test/mjsunit/harmony/modules-async-error-parent-entry.mjs b/test/mjsunit/harmony/modules-async-error-parent-entry.mjs
@@ -0,0 +1,5 @@
+// Copyright 2024 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.
+
+assertThrowsAsync(import('./modules-skip-async-error-parent.mjs'));
diff --git a/test/mjsunit/harmony/modules-skip-async-error-parent-dynamic.mjs b/test/mjsunit/harmony/modules-skip-async-error-parent-dynamic.mjs
diff --git a/test/mjsunit/harmony/modules-skip-async-error-parent-static1.mjs b/test/mjsunit/harmony/modules-skip-async-error-parent-static1.mjs
@@ -0,0 +1,6 @@
+// Copyright 2024 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.
+
+await import('./modules-skip-async-error-parent-dynamic.mjs');
+import('./modules-skip-async-error-parent.mjs').catch(() => {});
diff --git a/test/mjsunit/harmony/modules-skip-async-error-parent-static2.mjs b/test/mjsunit/harmony/modules-skip-async-error-parent-static2.mjs
@@ -0,0 +1,6 @@
+// Copyright 2024 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.
+
+import('./modules-skip-async-error-parent-static1.mjs');
+await import('./modules-skip-async-error-parent-dynamic.mjs');
diff --git a/test/mjsunit/harmony/modules-skip-async-error-parent.mjs b/test/mjsunit/harmony/modules-skip-async-error-parent.mjs
@@ -0,0 +1,10 @@
+// Copyright 2024 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.
+
+import './modules-skip-async-error-parent-static1.mjs';
+import './modules-skip-async-error-parent-static2.mjs';
+
+await {};
+
+throw new Error('aaa');

Original file line number	Diff line number	Diff line change
`@@ -218,7 +218,7 @@ bool Module::Instantiate(Isolate* isolate, Handle<Module> module,`
`218`	`218`	`return false;`
`219`	`219`	`}`
`220`	`220`	`DCHECK(module->status() == kLinked \|\| module->status() == kEvaluated \|\|`
`221`		`- module->status() == kErrored);`
	`221`	`+ module->status() == kEvaluatingAsync \|\| module->status() == kErrored);`
`222`	`222`	`DCHECK(stack.empty());`
`223`	`223`	`return true;`
`224`	`224`	`}`