module: change return type for ModuleWrap::SyntheticModuleEvaluationStepsCallback

common.gypi

@@ -42,7 +42,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.28',
+    'v8_embedder_string': '-node.30',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/include/v8-script.h

@@ -280,7 +280,7 @@ class V8_EXPORT Module : public Data {
    *
    * If IsGraphAsync() is false, the returned Promise is settled.
    */
-  V8_WARN_UNUSED_RESULT MaybeLocal<Value> Evaluate(Local<Context> context);
+  V8_WARN_UNUSED_RESULT MaybeLocal<Promise> Evaluate(Local<Context> context);
 
   /**
    * Returns the namespace object of this module.
@@ -337,6 +337,15 @@ class V8_EXPORT Module : public Data {
    * (where an exception was thrown).
    */
   using SyntheticModuleEvaluationSteps =
+      MaybeLocal<Promise> (*)(Local<Context> context, Local<Module> module);
+
+  /*
+   * Deprecated version of SyntheticModuleEvaluationSteps: the returned value is
+   * still required to be a Promise, but that is only enforced at runtime.
+   */
+  // TODO(https://crbug.com/545375591): Remove once all embedders return a
+  // MaybeLocal<Promise>.
+  using LegacySyntheticModuleEvaluationSteps =
       MaybeLocal<Value> (*)(Local<Context> context, Local<Module> module);
 
   /**
@@ -351,6 +360,16 @@ class V8_EXPORT Module : public Data {
       const MemorySpan<const Local<String>>& export_names,
       SyntheticModuleEvaluationSteps evaluation_steps);
 
+  // TODO(https://crbug.com/545375591): Advance to V8_DEPRECATED and then remove
+  // this overload once all embedders have been migrated to the one above.
+  V8_DEPRECATE_SOON(
+      "Use the CreateSyntheticModule overload whose evaluation_steps return a "
+      "MaybeLocal<Promise>")
+  static Local<Module> CreateSyntheticModule(
+      Isolate* isolate, Local<String> module_name,
+      const MemorySpan<const Local<String>>& export_names,
+      LegacySyntheticModuleEvaluationSteps evaluation_steps);
+
   /**
    * Set this module's exported value for the name export_name to the specified
    * export_value. This method must be called only on Modules created via

deps/v8/src/api/api.cc

@@ -2398,7 +2398,7 @@ Maybe<bool> Module::InstantiateModule(Local<Context> context,
   return Just(true);
 }
 
-MaybeLocal<Value> Module::Evaluate(Local<Context> context) {
+MaybeLocal<Promise> Module::Evaluate(Local<Context> context) {
   auto i_isolate = i::Isolate::Current();
   TRACE_EVENT_CALL_STATS_SCOPED(i_isolate, "v8", "V8.Execute");
   EnterV8Scope<InternalEscapableScope> api_scope{i_isolate, context,
@@ -2436,6 +2436,33 @@ Local<Module> Module::CreateSyntheticModule(
           i_module_name, i_export_names, evaluation_steps)));
 }
 
+START_ALLOW_USE_DEPRECATED()
+Local<Module> Module::CreateSyntheticModule(
+    Isolate* v8_isolate, Local<String> module_name,
+    const MemorySpan<const Local<String>>& export_names,
+    v8::Module::LegacySyntheticModuleEvaluationSteps evaluation_steps) {
+  // TODO(https://crbug.com/545375591): Remove once
+  // LegacySyntheticModuleEvaluationSteps is gone.
+#if (__GNUC__ >= 8) || defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wcast-function-type"
+#endif
+  // Cast from 'v8::MaybeLocal<v8::Value> (*)(v8::Local<v8::Context>,
+  // v8::Local<v8::Module>)' to 'v8::MaybeLocal<v8::Promise>
+  // (*)(v8::Local<v8::Context>, v8::Local<v8::Module>)'. Both return types are
+  // pointer-sized, trivially copyable handle wrappers, so they share the same
+  // representation. SyntheticModule::Evaluate() checks at runtime that the
+  // returned value really is a Promise.
+  auto promise_returning_steps =
+      reinterpret_cast<SyntheticModuleEvaluationSteps>(evaluation_steps);
+#if (__GNUC__ >= 8) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+  return CreateSyntheticModule(v8_isolate, module_name, export_names,
+                               promise_returning_steps);
+}
+END_ALLOW_USE_DEPRECATED()
+
 Maybe<bool> Module::SetSyntheticModuleExport(Isolate* v8_isolate,
                                              Local<String> export_name,
                                              Local<v8::Value> export_value) {

deps/v8/src/d8/d8.cc

@@ -1441,7 +1441,7 @@ MaybeLocal<Module> Shell::FetchModuleTree(Local<Module> referrer,
   return result;
 }
 
-MaybeLocal<Value> Shell::JSONModuleEvaluationSteps(Local<Context> context,
+MaybeLocal<Promise> Shell::JSONModuleEvaluationSteps(Local<Context> context,
                                                    Local<Module> module) {
   Isolate* isolate = Isolate::GetCurrent();
 

deps/v8/src/d8/d8.h

@@ -884,9 +884,8 @@ class Shell : public i::AllStatic {
                                             const std::string& file_name,
                                             ModuleType module_type);
 
-  static MaybeLocal<Value> JSONModuleEvaluationSteps(Local<Context> context,
-                                                     Local<Module> module);
-
+  static MaybeLocal<Promise> JSONModuleEvaluationSteps(Local<Context> context,
+                                                       Local<Module> module);
   template <class T>
   static MaybeLocal<T> CompileString(Isolate* isolate, Local<Context> context,
                                      Local<String> source,

deps/v8/src/objects/module.cc

@@ -265,8 +265,8 @@ bool Module::FinishInstantiate(Isolate* isolate, Handle<Module> module,
   }
 }
 
-MaybeDirectHandle<Object> Module::Evaluate(Isolate* isolate,
-                                           Handle<Module> module) {
+MaybeDirectHandle<JSPromise> Module::Evaluate(Isolate* isolate,
+                                              Handle<Module> module) {
 #ifdef DEBUG
   PrintStatusMessage(*module, "Evaluating module ");
 #endif  // DEBUG
@@ -492,16 +492,13 @@ void JSDeferredModuleNamespace::EvaluateModuleSync(
     return;
   }
 
-  MaybeDirectHandle<Object> maybe_result = Module::Evaluate(isolate, module);
-  DirectHandle<Object> result;
-  if (!maybe_result.ToHandle(&result)) {
+  MaybeDirectHandle<JSPromise> maybe_result = Module::Evaluate(isolate, module);
+  DirectHandle<JSPromise> promise;
+  if (!maybe_result.ToHandle(&promise)) {
     return;
   }
 
-  // If there's a result, it needs to be a promise with either Reject or
-  // Fulfilled status.
-  DCHECK(IsJSPromise(*result));
-  DirectHandle<JSPromise> promise = Cast<JSPromise>(result);
+  // The result is always the module's top-level capability promise.
   // 5. If promise.[[PromiseState]] is rejected, then
   if (promise->status() == Promise::kRejected) {
     // a. If promise.[[PromiseIsHandled]] is false, perform

deps/v8/src/objects/module.h

@@ -74,7 +74,7 @@ class Module : public TorqueGeneratedModule<Module, HeapObject> {
       const UserResolveCallbacks& callbacks);
 
   // Implementation of spec operation ModuleEvaluation.
-  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<Object> Evaluate(
+  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<JSPromise> Evaluate(
       Isolate* isolate, Handle<Module> module);
 
   // Get the namespace object for [module].  If it doesn't exist yet, it is

deps/v8/src/objects/source-text-module.cc

@@ -886,8 +886,8 @@ bool SourceTextModule::MaybeHandleEvaluationException(
   return false;
 }
 
-// ES#sec-moduleevaluation
-MaybeDirectHandle<Object> SourceTextModule::Evaluate(
+// https://tc39.es/ecma262/#sec-moduleevaluation
+MaybeDirectHandle<JSPromise> SourceTextModule::Evaluate(
     Isolate* isolate, Handle<SourceTextModule> module) {
   CHECK(module->status() == kLinked || module->status() == kEvaluatingAsync ||
         module->status() == kEvaluated);

deps/v8/src/objects/source-text-module.h

@@ -192,7 +192,7 @@ class SourceTextModule
                                        AvailableAncestorsSet* exec_list);
 
   // Implementation of spec concrete method Evaluate.
-  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<Object> Evaluate(
+  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<JSPromise> Evaluate(
       Isolate* isolate, Handle<SourceTextModule> module);
 
   // Implementation of spec abstract operation InnerModuleEvaluation.

deps/v8/src/objects/synthetic-module.cc

@@ -5,6 +5,7 @@
 #include "src/objects/synthetic-module.h"
 
 #include "src/api/api-inl.h"
+#include "src/base/macros.h"
 #include "src/builtins/accessors.h"
 #include "src/objects/js-generator-inl.h"
 #include "src/objects/module-inl.h"
@@ -105,41 +106,39 @@ bool SyntheticModule::FinishInstantiate(Isolate* isolate,
 
 // Implements Synthetic Module Record's Evaluate concrete method:
 // https://heycam.github.io/webidl/#smr-evaluate
-MaybeDirectHandle<Object> SyntheticModule::Evaluate(
+// The callback may have been created through the deprecated
+// v8::Module::LegacySyntheticModuleEvaluationSteps overload, in which case it
+// actually returns a v8::MaybeLocal<v8::Value> and is called here through a
+// mismatching signature. Both return types are pointer-sized, trivially
+// copyable handle wrappers, so this is safe in practice, but it does trip
+// CFI's and UBSan's indirect call checks.
+// TODO(https://crbug.com/545375591): Remove DISABLE_CFI_ICALL once the
+// deprecated overload is gone.
+DISABLE_CFI_ICALL
+MaybeDirectHandle<JSPromise> SyntheticModule::Evaluate(
     Isolate* isolate, DirectHandle<SyntheticModule> module) {
   module->SetStatus(kEvaluating);
 
   v8::Module::SyntheticModuleEvaluationSteps evaluation_steps =
       FUNCTION_CAST<v8::Module::SyntheticModuleEvaluationSteps>(
           module->evaluation_steps()->foreign_address<kSyntheticModuleTag>());
+  // Deliberately received as a v8::Local<v8::Value>: the deprecated callback
+  // signature only promises a Promise, it doesn't guarantee one.
   v8::Local<v8::Value> result;
   if (!evaluation_steps(Utils::ToLocal(isolate->native_context()),
                         Utils::ToLocal(Cast<Module>(module)))
            .ToLocal(&result)) {
     module->RecordError(isolate, isolate->exception());
-    return MaybeDirectHandle<Object>();
+    return MaybeDirectHandle<JSPromise>();
   }
 
   module->SetStatus(kEvaluated);
 
   DirectHandle<Object> result_from_callback = Utils::OpenDirectHandle(*result);
-
-  DirectHandle<JSPromise> capability;
-  if (IsJSPromise(*result_from_callback)) {
-    capability = Cast<JSPromise>(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();
-  }
-
+  CHECK(IsJSPromise(*result_from_callback));
+  DirectHandle<JSPromise> capability = Cast<JSPromise>(result_from_callback);
   module->set_top_level_capability(*capability);
-
-  return result_from_callback;
+  return capability;
 }
 
 }  // namespace internal

deps/v8/src/objects/synthetic-module.h

@@ -60,7 +60,7 @@ class SyntheticModule
   static V8_WARN_UNUSED_RESULT bool FinishInstantiate(
       Isolate* isolate, DirectHandle<SyntheticModule> module);
 
-  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<Object> Evaluate(
+  static V8_WARN_UNUSED_RESULT MaybeDirectHandle<JSPromise> Evaluate(
       Isolate* isolate, DirectHandle<SyntheticModule> module);
 
   TQ_OBJECT_CONSTRUCTORS(SyntheticModule)

deps/v8/test/cctest/test-api.cc

@@ -24701,33 +24701,52 @@ TEST(CodeCache) {
   isolate2->Dispose();
 }
 
-v8::MaybeLocal<Value> UnexpectedSyntheticModuleEvaluationStepsCallback(
+v8::MaybeLocal<Promise> UnexpectedSyntheticModuleEvaluationStepsCallback(
     Local<Context> context, Local<Module> module) {
   CHECK_WITH_MSG(false, "Unexpected call to synthetic module re callback");
 }
 
 static int synthetic_module_callback_count;
 
-v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallback(
+v8::MaybeLocal<Promise> SyntheticModuleEvaluationStepsCallback(
     Local<Context> context, Local<Module> module) {
   synthetic_module_callback_count++;
-  return v8::Undefined(reinterpret_cast<v8::Isolate*>(CcTest::isolate()));
+  Local<v8::Promise::Resolver> resolver =
+      v8::Promise::Resolver::New(context).ToLocalChecked();
+  resolver->Resolve(context, v8::Undefined(CcTest::isolate())).Check();
+  return resolver->GetPromise();
 }
 
-v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackFail(
+v8::MaybeLocal<Promise> SyntheticModuleEvaluationStepsCallbackFail(
     Local<Context> context, Local<Module> module) {
   synthetic_module_callback_count++;
   CcTest::isolate()->ThrowException(
       v8_str("SyntheticModuleEvaluationStepsCallbackFail exception"));
-  return v8::MaybeLocal<Value>();
+  return v8::MaybeLocal<Promise>();
 }
 
-v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackSetExport(
+// Deprecated version of the evaluation steps, returning a MaybeLocal<Value>
+// that holds a Promise.
+// TODO(https://crbug.com/545375591): Remove together with
+// v8::Module::LegacySyntheticModuleEvaluationSteps.
+v8::MaybeLocal<Value> LegacySyntheticModuleEvaluationStepsCallback(
+    Local<Context> context, Local<Module> module) {
+  synthetic_module_callback_count++;
+  Local<v8::Promise::Resolver> resolver =
+      v8::Promise::Resolver::New(context).ToLocalChecked();
+  resolver->Resolve(context, v8::Undefined(CcTest::isolate())).Check();
+  return resolver->GetPromise();
+}
+
+v8::MaybeLocal<Promise> SyntheticModuleEvaluationStepsCallbackSetExport(
     Local<Context> context, Local<Module> module) {
   Maybe<bool> set_export_result = module->SetSyntheticModuleExport(
       CcTest::isolate(), v8_str("test_export"), v8_num(42));
   CHECK(set_export_result.FromJust());
-  return v8::Undefined(reinterpret_cast<v8::Isolate*>(CcTest::isolate()));
+  Local<v8::Promise::Resolver> resolver =
+      v8::Promise::Resolver::New(context).ToLocalChecked();
+  resolver->Resolve(context, v8::Undefined(CcTest::isolate())).Check();
+  return resolver->GetPromise();
 }
 
 namespace {
@@ -25058,7 +25077,44 @@ TEST(SyntheticModuleEvaluationStepsNoThrow) {
       context, export_names, SyntheticModuleEvaluationStepsCallback);
   CHECK_EQ(synthetic_module_callback_count, 0);
   Local<Value> completion_value = module->Evaluate(context).ToLocalChecked();
-  CHECK(completion_value->IsUndefined());
+  CHECK(completion_value->IsPromise());
+  Local<v8::Promise> promise(Local<v8::Promise>::Cast(completion_value));
+  CHECK_EQ(promise->State(), v8::Promise::kFulfilled);
+  CHECK(promise->Result()->IsUndefined());
+  CHECK_EQ(synthetic_module_callback_count, 1);
+  CHECK_EQ(module->GetStatus(), Module::kEvaluated);
+}
+
+// Covers the deprecated evaluation steps version, where the returned Promise is
+// only checked at runtime.
+// TODO(https://crbug.com/545375591): Remove together with
+// v8::Module::LegacySyntheticModuleEvaluationSteps.
+TEST(SyntheticModuleEvaluationStepsLegacyCallback) {
+  synthetic_module_callback_count = 0;
+  LocalContext env;
+  v8::Isolate* isolate = env.isolate();
+  v8::Isolate::Scope iscope(isolate);
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Context> context = v8::Context::New(isolate);
+  v8::Context::Scope cscope(context);
+
+  auto export_names = std::to_array<Local<v8::String>>({v8_str("default")});
+
+  START_ALLOW_USE_DEPRECATED()
+  Local<Module> module = v8::Module::CreateSyntheticModule(
+      isolate,
+      v8_str("SyntheticModuleEvaluationStepsLegacyCallback-"
+             "TestSyntheticModule"),
+      export_names, LegacySyntheticModuleEvaluationStepsCallback);
+  END_ALLOW_USE_DEPRECATED()
+  module->InstantiateModule(context, UnexpectedModuleResolveCallback)
+      .ToChecked();
+
+  CHECK_EQ(synthetic_module_callback_count, 0);
+  Local<Value> completion_value = module->Evaluate(context).ToLocalChecked();
+  CHECK(completion_value->IsPromise());
+  Local<v8::Promise> promise(Local<v8::Promise>::Cast(completion_value));
+  CHECK_EQ(promise->State(), v8::Promise::kFulfilled);
   CHECK_EQ(synthetic_module_callback_count, 1);
   CHECK_EQ(module->GetStatus(), Module::kEvaluated);
 }
@@ -25116,7 +25172,10 @@ TEST(SyntheticModuleEvaluationStepsSetExport) {
   CHECK(IsUndefined(test_export_cell->value()));
 
   Local<Value> completion_value = module->Evaluate(context).ToLocalChecked();
-  CHECK(completion_value->IsUndefined());
+  CHECK(completion_value->IsPromise());
+  Local<v8::Promise> promise(Local<v8::Promise>::Cast(completion_value));
+  CHECK_EQ(promise->State(), v8::Promise::kFulfilled);
+  CHECK(promise->Result()->IsUndefined());
   CHECK_EQ(42, i::Object::NumberValue(test_export_cell->value()));
   CHECK_EQ(module->GetStatus(), Module::kEvaluated);
 }
@@ -27084,7 +27143,7 @@ MaybeLocal<Module> CheckResolveModuleWithImportSource(
 
   return v8::Module::CreateSyntheticModule(
       isolate, v8_str("my-mod"), {},
-      [](Local<Context> context, Local<Module> module) -> MaybeLocal<Value> {
+      [](Local<Context> context, Local<Module> module) -> MaybeLocal<Promise> {
         // Do nothing.
         Local<v8::Promise::Resolver> resolver =
             v8::Promise::Resolver::New(context).ToLocalChecked();