[modules] Handle termination exceptions better

- Force RecordError in case of a TerminationException
- Remove Module::RecordErrorUsingPendingException
- Use more raw objects and instance methods if possible

Bug: v8:12379
Change-Id: Ia7e73715c3cdfe59d3fa324be3ce4213e454ff26
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3829087Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82561}

src/objects/module.cc

@@ -74,31 +74,24 @@ void Module::SetStatus(Status new_status) {
   SetStatusInternal(*this, new_status);
 }
 
-// static
-void Module::RecordErrorUsingPendingException(Isolate* isolate,
-                                              Handle<Module> module) {
-  Handle<Object> the_exception(isolate->pending_exception(), isolate);
-  RecordError(isolate, module, the_exception);
-}
-
-// static
-void Module::RecordError(Isolate* isolate, Handle<Module> module,
-                         Handle<Object> error) {
+void Module::RecordError(Isolate* isolate, Object error) {
   DisallowGarbageCollection no_gc;
-  DCHECK(module->exception().IsTheHole(isolate));
-  DCHECK(!error->IsTheHole(isolate));
-  if (module->IsSourceTextModule()) {
+  // Allow overriding exceptions with termination exceptions.
+  DCHECK_IMPLIES(isolate->is_catchable_by_javascript(error),
+                 exception().IsTheHole(isolate));
+  DCHECK(!error.IsTheHole(isolate));
+  if (IsSourceTextModule()) {
     // Revert to minmal SFI in case we have already been instantiating or
     // evaluating.
-    auto self = SourceTextModule::cast(*module);
+    auto self = SourceTextModule::cast(*this);
     self.set_code(self.GetSharedFunctionInfo());
   }
-  SetStatusInternal(*module, Module::kErrored);
-  if (isolate->is_catchable_by_javascript(*error)) {
-    module->set_exception(*error);
+  SetStatusInternal(*this, Module::kErrored);
+  if (isolate->is_catchable_by_javascript(error)) {
+    set_exception(error);
   } else {
     // v8::TryCatch uses `null` for termination exceptions.
-    module->set_exception(*isolate->factory()->null_value());
+    set_exception(ReadOnlyRoots(isolate).null_value());
   }
 }
 
@@ -246,10 +239,10 @@ MaybeHandle<Object> Module::Evaluate(Isolate* isolate, Handle<Module> module) {
 #ifdef DEBUG
   PrintStatusMessage(*module, "Evaluating module ");
 #endif  // DEBUG
-  STACK_CHECK(isolate, MaybeHandle<Object>());
+  int module_status = module->status();
 
   // In the event of errored evaluation, return a rejected promise.
-  if (module->status() == kErrored) {
+  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.
@@ -267,12 +260,12 @@ MaybeHandle<Object> Module::Evaluate(Isolate* isolate, Handle<Module> module) {
 
   // Start of Evaluate () Concrete Method
   // 2. Assert: module.[[Status]] is "linked" or "evaluated".
-  CHECK(module->status() == kLinked || module->status() == kEvaluated);
+  CHECK(module_status == kLinked || 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()) {
+  if (module_status == kEvaluated && module->IsSourceTextModule()) {
     module = Handle<SourceTextModule>::cast(module)->GetCycleRoot(isolate);
   }
 

src/objects/module.h

@@ -121,13 +121,9 @@ class Module : public TorqueGeneratedModule<Module, HeapObject> {
   static void Reset(Isolate* isolate, Handle<Module> module);
   static void ResetGraph(Isolate* isolate, Handle<Module> module);
 
-  // To set status to kErrored, RecordError or RecordErrorUsingPendingException
-  // should be used.
+  // To set status to kErrored, RecordError should be used.
   void SetStatus(Status status);
-  static void RecordErrorUsingPendingException(Isolate* isolate,
-                                               Handle<Module>);
-  static void RecordError(Isolate* isolate, Handle<Module> module,
-                          Handle<Object> error);
+  void RecordError(Isolate* isolate, Object error);
 
   TQ_OBJECT_CONSTRUCTORS(Module)
 };

src/objects/source-text-module.cc

@@ -683,6 +683,34 @@ MaybeHandle<JSObject> SourceTextModule::GetImportMeta(
   return Handle<JSObject>::cast(import_meta);
 }
 
+bool SourceTextModule::MaybeHandleEvaluationException(
+    Isolate* isolate, ZoneForwardList<Handle<SourceTextModule>>* stack) {
+  DisallowGarbageCollection no_gc;
+  Object pending_exception = isolate->pending_exception();
+  if (isolate->is_catchable_by_javascript(pending_exception)) {
+    //  a. For each Cyclic Module Record m in stack, do
+    for (Handle<SourceTextModule>& descendant : *stack) {
+      //   i. Assert: m.[[Status]] is "evaluating".
+      CHECK_EQ(descendant->status(), kEvaluating);
+      //  ii. Set m.[[Status]] to "evaluated".
+      // iii. Set m.[[EvaluationError]] to result.
+      descendant->RecordError(isolate, pending_exception);
+    }
+    return true;
+  }
+  // If the exception was a termination exception, rejecting the promise
+  // would resume execution, and our API contract is to return an empty
+  // handle. The module's status should be set to kErrored and the
+  // exception field should be set to `null`.
+  RecordError(isolate, pending_exception);
+  for (Handle<SourceTextModule>& descendant : *stack) {
+    descendant->RecordError(isolate, pending_exception);
+  }
+  CHECK_EQ(status(), kErrored);
+  CHECK_EQ(exception(), *isolate->factory()->null_value());
+  return false;
+}
+
 MaybeHandle<Object> SourceTextModule::Evaluate(
     Isolate* isolate, Handle<SourceTextModule> module) {
   CHECK(module->status() == kLinked || module->status() == kEvaluated);
@@ -704,26 +732,8 @@ MaybeHandle<Object> SourceTextModule::Evaluate(
   Handle<Object> unused_result;
   if (!InnerModuleEvaluation(isolate, module, &stack, &dfs_index)
            .ToHandle(&unused_result)) {
-    //  a. For each Cyclic Module Record m in stack, do
-    for (auto& descendant : stack) {
-      //   i. Assert: m.[[Status]] is "evaluating".
-      CHECK_EQ(descendant->status(), kEvaluating);
-      //  ii. Set m.[[Status]] to "evaluated".
-      // iii. Set m.[[EvaluationError]] to result.
-      Module::RecordErrorUsingPendingException(isolate, descendant);
-    }
-
-    // If the exception was a termination exception, rejecting the promise
-    // would resume execution, and our API contract is to return an empty
-    // handle. The module's status should be set to kErrored and the
-    // exception field should be set to `null`.
-    if (!isolate->is_catchable_by_javascript(isolate->pending_exception())) {
-      CHECK_EQ(module->status(), kErrored);
-      CHECK_EQ(module->exception(), *isolate->factory()->null_value());
-      return {};
-    }
+    if (!module->MaybeHandleEvaluationException(isolate, &stack)) return {};
     CHECK_EQ(module->exception(), isolate->pending_exception());
-
     //  d. Perform ! Call(capability.[[Reject]], undefined,
     //                    «result.[[Value]]»).
     isolate->clear_pending_exception();
@@ -875,7 +885,7 @@ void SourceTextModule::AsyncModuleExecutionRejected(
   }
 
   // 5. Set module.[[EvaluationError]] to ThrowCompletion(error).
-  Module::RecordError(isolate, module, exception);
+  module->RecordError(isolate, *exception);
 
   // 6. Set module.[[AsyncEvaluating]] to false.
   isolate->DidFinishModuleAsyncEvaluation(module->async_evaluating_ordinal());
@@ -1022,49 +1032,56 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
     Isolate* isolate, Handle<SourceTextModule> module,
     ZoneForwardList<Handle<SourceTextModule>>* stack, unsigned* dfs_index) {
   STACK_CHECK(isolate, MaybeHandle<Object>());
-
+  int module_status = module->status();
   // InnerModuleEvaluation(module, stack, index)
   // 2. If module.[[Status]] is "evaluated", then
   //    a. If module.[[EvaluationError]] is undefined, return index.
   //       (We return undefined instead)
-  if (module->status() == kEvaluated || module->status() == kEvaluating) {
+  if (module_status == kEvaluated || module_status == kEvaluating) {
     return isolate->factory()->undefined_value();
   }
 
   //    b. Otherwise return module.[[EvaluationError]].
   //       (We throw on isolate and return a MaybeHandle<Object>
   //        instead)
-  if (module->status() == kErrored) {
+  if (module_status == kErrored) {
     isolate->Throw(module->exception());
     return MaybeHandle<Object>();
   }
 
   // 4. Assert: module.[[Status]] is "linked".
-  CHECK_EQ(module->status(), kLinked);
+  CHECK_EQ(module_status, kLinked);
 
-  // 5. Set module.[[Status]] to "evaluating".
-  module->SetStatus(kEvaluating);
+  Handle<FixedArray> requested_modules;
 
-  // 6. Set module.[[DFSIndex]] to index.
-  module->set_dfs_index(*dfs_index);
+  {
+    DisallowGarbageCollection no_gc;
+    SourceTextModule raw_module = *module;
+    // 5. Set module.[[Status]] to "evaluating".
+    raw_module.SetStatus(kEvaluating);
 
-  // 7. Set module.[[DFSAncestorIndex]] to index.
-  module->set_dfs_ancestor_index(*dfs_index);
+    // 6. Set module.[[DFSIndex]] to index.
+    raw_module.set_dfs_index(*dfs_index);
 
-  // 8. Set module.[[PendingAsyncDependencies]] to 0.
-  DCHECK(!module->HasPendingAsyncDependencies());
+    // 7. Set module.[[DFSAncestorIndex]] to index.
+    raw_module.set_dfs_ancestor_index(*dfs_index);
 
-  // 9. Set module.[[AsyncParentModules]] to a new empty List.
-  module->set_async_parent_modules(ReadOnlyRoots(isolate).empty_array_list());
+    // 8. Set module.[[PendingAsyncDependencies]] to 0.
+    DCHECK(!raw_module.HasPendingAsyncDependencies());
 
-  // 10. Set index to index + 1.
-  (*dfs_index)++;
+    // 9. Set module.[[AsyncParentModules]] to a new empty List.
+    raw_module.set_async_parent_modules(
+        ReadOnlyRoots(isolate).empty_array_list());
 
-  // 11. Append module to stack.
-  stack->push_front(module);
+    // 10. Set index to index + 1.
+    (*dfs_index)++;
 
-  // Recursion.
-  Handle<FixedArray> requested_modules(module->requested_modules(), isolate);
+    // 11. Append module to stack.
+    stack->push_front(module);
+
+    // Recursion.
+    requested_modules = handle(raw_module.requested_modules(), isolate);
+  }
 
   // 12. For each String required that is an element of
   //     module.[[RequestedModules]], do
@@ -1079,13 +1096,14 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
           isolate,
           InnerModuleEvaluation(isolate, required_module, stack, dfs_index),
           Object);
+      int required_module_status = required_module->status();
 
       //    i. Assert: requiredModule.[[Status]] is either "evaluating" or
       //       "evaluated".
       //       (We also assert the module cannot be errored, because if it was
       //        we would have already returned from InnerModuleEvaluation)
-      CHECK_GE(required_module->status(), kEvaluating);
-      CHECK_NE(required_module->status(), kErrored);
+      CHECK_GE(required_module_status, kEvaluating);
+      CHECK_NE(required_module_status, kErrored);
 
       //   ii.  Assert: requiredModule.[[Status]] is "evaluating" if and
       //        only if requiredModule is in stack.
@@ -1096,7 +1114,7 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
           }));
 
       //  iii.  If requiredModule.[[Status]] is "evaluating", then
-      if (required_module->status() == kEvaluating) {
+      if (required_module_status == kEvaluating) {
         //      1. Set module.[[DFSAncestorIndex]] to
         //         min(
         //           module.[[DFSAncestorIndex]],
@@ -1108,9 +1126,10 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
         //   iv. Otherwise,
         //      1. Set requiredModule to requiredModule.[[CycleRoot]].
         required_module = required_module->GetCycleRoot(isolate);
+        required_module_status = required_module->status();
 
         //      2. Assert: requiredModule.[[Status]] is "evaluated".
-        CHECK_GE(required_module->status(), kEvaluated);
+        CHECK_GE(required_module_status, kEvaluated);
 
         //      3. If requiredModule.[[EvaluationError]] is not undefined,
         //         return module.[[EvaluationError]].
@@ -1119,7 +1138,7 @@ MaybeHandle<Object> SourceTextModule::InnerModuleEvaluation(
         //          where the AsyncCycleRoot has an error. Instead of returning
         //          the exception, we throw on isolate and return a
         //          MaybeHandle<Object>)
-        if (required_module->status() == kErrored) {
+        if (required_module_status == kErrored) {
           isolate->Throw(required_module->exception());
           return MaybeHandle<Object>();
         }

src/objects/source-text-module.h

@@ -198,6 +198,11 @@ class SourceTextModule
       Isolate* isolate, Handle<SourceTextModule> module,
       ZoneForwardList<Handle<SourceTextModule>>* stack, unsigned* dfs_index);
 
+  // Returns true if the evaluation exception was catchable by js, and false
+  // for termination exceptions.
+  bool MaybeHandleEvaluationException(
+      Isolate* isolate, ZoneForwardList<Handle<SourceTextModule>>* stack);
+
   static V8_WARN_UNUSED_RESULT bool MaybeTransitionComponent(
       Isolate* isolate, Handle<SourceTextModule> module,
       ZoneForwardList<Handle<SourceTextModule>>* stack, Status new_status);

src/objects/synthetic-module.cc

@@ -111,7 +111,7 @@ MaybeHandle<Object> SyntheticModule::Evaluate(Isolate* isolate,
            Utils::ToLocal(Handle<Module>::cast(module)))
            .ToLocal(&result)) {
     isolate->PromoteScheduledException();
-    Module::RecordErrorUsingPendingException(isolate, module);
+    module->RecordError(isolate, isolate->pending_exception());
     return MaybeHandle<Object>();
   }