Fix top-level await crash from synthetic module being evaluated twice

With top-level await, when Evaluate is performed on an already-evaluated
synthetic module, Module::InnerEvaluate returns undefined.  This breaks
top-level await's assumption that the returned value is always a
promise.

In order to make SyntheticModule's behavior consistent with
SourceTextModule, the top_level_capability field is moved up to Module
and SyntheticModule::Evaluate places the promise returned from the
host's evaluation steps in that field.  Now SourceTextModule and
SyntheticModule can share the same code to handle the case where the
module is either kErrored or kEvaluated, so the code for this
is moved up to Module.

Thus, SyntheticModule is now guaranteed to return the
promise from the evaluation steps even on subsequent Evaluate() calls.

Unfortunately Node hasn't yet updated their EvaluationStepsCallback
to return a Promise, so we can't yet assume that the returned value
is a Promise without breaking Node.  So, this change also adds a clause
to check for this condition and create a new resolved Promise if one
was not provided by the callback steps.  This could eventually be
removed once Node's callback steps are updated for top-level await.

Change-Id: I2d6ae918abfeba9e3a757838502d4df92946edaa
Bug: v8:11398
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2673794Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#72629}

include/v8.h

@@ -1724,7 +1724,7 @@ class V8_EXPORT Module : public Data {
   /*
    * Callback defined in the embedder.  This is responsible for setting
    * the module's exported values with calls to SetSyntheticModuleExport().
-   * The callback must return a Value to indicate success (where no
+   * The callback must return a resolved Promise to indicate success (where no
    * exception was thrown) and return an empy MaybeLocal to indicate falure
    * (where an exception was thrown).
    */

src/diagnostics/objects-debug.cc

@@ -1422,6 +1422,11 @@ void Module::ModuleVerify(Isolate* isolate) {
     CHECK_EQ(JSModuleNamespace::cast(module_namespace()).module(), *this);
   }
 
+  if (!(status() == kErrored || status() == kEvaluating ||
+        status() == kEvaluated)) {
+    CHECK(top_level_capability().IsUndefined());
+  }
+
   CHECK_NE(hash(), 0);
 }
 
@@ -1455,7 +1460,6 @@ void SourceTextModule::SourceTextModuleVerify(Isolate* isolate) {
     } else if (status() == kUninstantiated) {
       CHECK(code().IsSharedFunctionInfo());
     }
-    CHECK(top_level_capability().IsUndefined());
     CHECK(!AsyncParentModuleCount());
     CHECK(!pending_async_dependencies());
     CHECK(!async_evaluating());

src/heap/factory.cc

@@ -2335,10 +2335,10 @@ Handle<SourceTextModule> Factory::NewSourceTextModule(
   module->set_requested_modules(*requested_modules);
   module->set_status(Module::kUninstantiated);
   module->set_exception(roots.the_hole_value());
+  module->set_top_level_capability(roots.undefined_value());
   module->set_import_meta(roots.the_hole_value());
   module->set_dfs_index(-1);
   module->set_dfs_ancestor_index(-1);
-  module->set_top_level_capability(roots.undefined_value());
   module->set_flags(0);
   module->set_async(IsAsyncModule(sfi->kind()));
   module->set_async_evaluating(false);
@@ -2365,6 +2365,7 @@ Handle<SyntheticModule> Factory::NewSyntheticModule(
   module->set_module_namespace(roots.undefined_value());
   module->set_status(Module::kUninstantiated);
   module->set_exception(roots.the_hole_value());
+  module->set_top_level_capability(roots.undefined_value());
   module->set_name(*module_name);
   module->set_export_names(*export_names);
   module->set_exports(*exports);

src/objects/module-inl.h

@@ -33,6 +33,7 @@ CAST_ACCESSOR(Module)
 ACCESSORS(Module, exports, ObjectHashTable, kExportsOffset)
 ACCESSORS(Module, module_namespace, HeapObject, kModuleNamespaceOffset)
 ACCESSORS(Module, exception, Object, kExceptionOffset)
+ACCESSORS(Module, top_level_capability, HeapObject, kTopLevelCapabilityOffset)
 SMI_ACCESSORS(Module, status, kStatusOffset)
 SMI_ACCESSORS(Module, hash, kHashOffset)
 
@@ -41,8 +42,6 @@ BOOL_ACCESSORS(SourceTextModule, flags, async_evaluating,
                AsyncEvaluatingBit::kShift)
 ACCESSORS(SourceTextModule, async_parent_modules, ArrayList,
           kAsyncParentModulesOffset)
-ACCESSORS(SourceTextModule, top_level_capability, HeapObject,
-          kTopLevelCapabilityOffset)
 
 struct Module::Hash {
   V8_INLINE size_t operator()(Module const& module) const {

src/objects/module.cc

@@ -245,11 +245,56 @@ MaybeHandle<Object> Module::Evaluate(Isolate* isolate, Handle<Module> module) {
   PrintStatusMessage(*module, "Evaluating module ");
 #endif  // DEBUG
   STACK_CHECK(isolate, MaybeHandle<Object>());
-  if (FLAG_harmony_top_level_await && module->IsSourceTextModule()) {
+  if (FLAG_harmony_top_level_await) {
+    return Module::EvaluateMaybeAsync(isolate, module);
+  } else {
+    return Module::InnerEvaluate(isolate, module);
+  }
+}
+
+MaybeHandle<Object> Module::EvaluateMaybeAsync(Isolate* isolate,
+                                               Handle<Module> module) {
+  // In the event of errored evaluation, return a rejected promise.
+  if (module->status() == kErrored) {
+    // If we have a top level capability we assume it has already been
+    // rejected, and return it here. Otherwise create a new promise and
+    // reject it with the module's exception.
+    if (module->top_level_capability().IsJSPromise()) {
+      Handle<JSPromise> top_level_capability(
+          JSPromise::cast(module->top_level_capability()), isolate);
+      DCHECK(top_level_capability->status() == Promise::kRejected &&
+             top_level_capability->result() == module->exception());
+      return top_level_capability;
+    }
+    Handle<JSPromise> capability = isolate->factory()->NewJSPromise();
+    JSPromise::Reject(capability, handle(module->exception(), isolate));
+    return capability;
+  }
+
+  // Start of Evaluate () Concrete Method
+  // 2. Assert: module.[[Status]] is "linked" or "evaluated".
+  CHECK(module->status() == kInstantiated || module->status() == kEvaluated);
+
+  // 3. If module.[[Status]] is "evaluated", set module to
+  //    module.[[CycleRoot]].
+  // A Synthetic Module has no children so it is its own cycle root.
+  if (module->status() == kEvaluated && module->IsSourceTextModule()) {
+    module = Handle<SourceTextModule>::cast(module)->GetCycleRoot(isolate);
+  }
+
+  // 4. If module.[[TopLevelCapability]] is not undefined, then
+  //    a. Return module.[[TopLevelCapability]].[[Promise]].
+  if (module->top_level_capability().IsJSPromise()) {
+    return handle(JSPromise::cast(module->top_level_capability()), isolate);
+  }
+  DCHECK(module->top_level_capability().IsUndefined());
+
+  if (module->IsSourceTextModule()) {
     return SourceTextModule::EvaluateMaybeAsync(
         isolate, Handle<SourceTextModule>::cast(module));
   } else {
-    return Module::InnerEvaluate(isolate, module);
+    return SyntheticModule::Evaluate(isolate,
+                                     Handle<SyntheticModule>::cast(module));
   }
 }
 

src/objects/module.h

@@ -65,6 +65,10 @@ class Module : public HeapObject {
   Object GetException();
   DECL_ACCESSORS(exception, Object)
 
+  // The top level promise capability of this module. Will only be defined
+  // for cycle roots.
+  DECL_ACCESSORS(top_level_capability, HeapObject)
+
   // Returns if this module or any transitively requested module is [[Async]],
   // i.e. has a top-level await.
   V8_WARN_UNUSED_RESULT bool IsGraphAsync(Isolate* isolate) const;
@@ -132,6 +136,9 @@ class Module : public HeapObject {
       ZoneForwardList<Handle<SourceTextModule>>* stack, unsigned* dfs_index,
       Zone* zone);
 
+  static V8_WARN_UNUSED_RESULT MaybeHandle<Object> EvaluateMaybeAsync(
+      Isolate* isolate, Handle<Module> module);
+
   static V8_WARN_UNUSED_RESULT MaybeHandle<Object> InnerEvaluate(
       Isolate* isolate, Handle<Module> module);
 

src/objects/module.tq

@@ -9,6 +9,7 @@ extern class Module extends HeapObject {
   status: Smi;
   module_namespace: JSModuleNamespace|Undefined;
   exception: Object;
+  top_level_capability: JSPromise|Undefined;
 }
 
 @generateCppClass

src/objects/source-text-module.cc

@@ -624,40 +624,6 @@ MaybeHandle<JSObject> SourceTextModule::GetImportMeta(
 
 MaybeHandle<Object> SourceTextModule::EvaluateMaybeAsync(
     Isolate* isolate, Handle<SourceTextModule> module) {
-  // In the event of errored evaluation, return a rejected promise.
-  if (module->status() == kErrored) {
-    // If we have a top level capability we assume it has already been
-    // rejected, and return it here. Otherwise create a new promise and
-    // reject it with the module's exception.
-    if (module->top_level_capability().IsJSPromise()) {
-      Handle<JSPromise> top_level_capability(
-          JSPromise::cast(module->top_level_capability()), isolate);
-      DCHECK(top_level_capability->status() == Promise::kRejected &&
-             top_level_capability->result() == module->exception());
-      return top_level_capability;
-    }
-    Handle<JSPromise> capability = isolate->factory()->NewJSPromise();
-    JSPromise::Reject(capability, handle(module->exception(), isolate));
-    return capability;
-  }
-
-  // Start of Evaluate () Concrete Method
-  // 2. Assert: module.[[Status]] is "linked" or "evaluated".
-  CHECK(module->status() == kInstantiated || module->status() == kEvaluated);
-
-  // 3. If module.[[Status]] is "evaluated", set module to
-  //    module.[[CycleRoot]].
-  if (module->status() == kEvaluated) {
-    module = module->GetCycleRoot(isolate);
-  }
-
-  // 4. If module.[[TopLevelCapability]] is not undefined, then
-  //    a. Return module.[[TopLevelCapability]].[[Promise]].
-  if (module->top_level_capability().IsJSPromise()) {
-    return handle(JSPromise::cast(module->top_level_capability()), isolate);
-  }
-  DCHECK(module->top_level_capability().IsUndefined());
-
   // 6. Let capability be ! NewPromiseCapability(%Promise%).
   Handle<JSPromise> capability = isolate->factory()->NewJSPromise();
 

src/objects/source-text-module.h

@@ -109,10 +109,6 @@ class SourceTextModule
   // an async child.
   DECL_BOOLEAN_ACCESSORS(async_evaluating)
 
-  // The top level promise capability of this module. Will only be defined
-  // for cycle roots.
-  DECL_ACCESSORS(top_level_capability, HeapObject)
-
   // The parent modules of a given async dependency, use async_parent_modules()
   // to retrieve the ArrayList representation.
   DECL_ACCESSORS(async_parent_modules, ArrayList)

src/objects/source-text-module.tq

@@ -37,7 +37,6 @@ extern class SourceTextModule extends Module {
   cycle_root: SourceTextModule|TheHole;
 
   async_parent_modules: ArrayList;
-  top_level_capability: JSPromise|Undefined;
 
   // TODO(neis): Don't store those in the module object?
   dfs_index: Smi;

src/objects/synthetic-module.cc

@@ -116,7 +116,27 @@ MaybeHandle<Object> SyntheticModule::Evaluate(Isolate* isolate,
   }
 
   module->SetStatus(kEvaluated);
-  return Utils::OpenHandle(*result);
+
+  Handle<Object> result_from_callback = Utils::OpenHandle(*result);
+
+  if (FLAG_harmony_top_level_await) {
+    Handle<JSPromise> capability;
+    if (result_from_callback->IsJSPromise()) {
+      capability = Handle<JSPromise>::cast(result_from_callback);
+    } else {
+      // The host's evaluation steps should have returned a resolved Promise,
+      // but as an allowance to hosts that have not yet finished the migration
+      // to top-level await, create a Promise if the callback result didn't give
+      // us one.
+      capability = isolate->factory()->NewJSPromise();
+      JSPromise::Resolve(capability, isolate->factory()->undefined_value())
+          .ToHandleChecked();
+    }
+
+    module->set_top_level_capability(*capability);
+  }
+
+  return result_from_callback;
 }
 
 }  // namespace internal

test/mjsunit/harmony/modules-import-assertions-dynamic-11.mjs

+// 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: --allow-natives-syntax --harmony-import-assertions --harmony-top-level-await
+
+var life1;
+var life2;
+import('modules-skip-1.json', { assert: { type: 'json' } }).then(
+    namespace => life1 = namespace.default.life);
+
+// Try loading the same module a second time.
+import('modules-skip-1.json', { assert: { type: 'json' } }).then(
+    namespace => life2 = namespace.default.life);
+
+%PerformMicrotaskCheckpoint();
+
+assertEquals(42, life1);
+assertEquals(42, life2);