Reland "[runtime] Use Isolate::ThrowAt with MessageLocation"

This is a reland of eb6b4ce1d8

Skip test that serializes Error which references a Script. All errors
created by ThrowAt store the current Script under the
error_script_symbol.

Original change's description:
> [runtime] Use Isolate::ThrowAt with MessageLocation
>
> Fix various missing source positions when reporting parse and compile
> errors. Namely this fixes missing source positions when having invalid
> module imports.
>
> - Use Isolate::ThrowAt with valid MessageLocation objects
> - Change public Isolate::Throw to no longer accept MessageLocation to
>   avoid misues
> - Introduce private Isolate::ThrowInternal that accepts MessageLocation
>
> Bug: v8:6513
> Change-Id: I3ee633c9fff8c9d361bddb37f56e28a50c280ec1
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2467839
> Commit-Queue: Camillo Bruni <cbruni@chromium.org>
> Reviewed-by: Marja Hölttä <marja@chromium.org>
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#70623}

Bug: v8:6513
Change-Id: Icba74f74178e28fbda0fd0c237eeb7bacbc33570
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2487123
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70741}

src/execution/execution.cc

@@ -194,10 +194,10 @@ MaybeHandle<Context> NewScriptContext(Isolate* isolate,
           // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
           // exception.
           MessageLocation location(script, 0, 1);
-          isolate->ThrowAt(isolate->factory()->NewSyntaxError(
-                               MessageTemplate::kVarRedeclaration, name),
-                           &location);
-          return MaybeHandle<Context>();
+          return isolate->ThrowAt<Context>(
+              isolate->factory()->NewSyntaxError(
+                  MessageTemplate::kVarRedeclaration, name),
+              &location);
         }
       }
     }
@@ -216,10 +216,10 @@ MaybeHandle<Context> NewScriptContext(Isolate* isolate,
         // ES#sec-globaldeclarationinstantiation 5.d:
         // If hasRestrictedGlobal is true, throw a SyntaxError exception.
         MessageLocation location(script, 0, 1);
-        isolate->ThrowAt(isolate->factory()->NewSyntaxError(
-                             MessageTemplate::kVarRedeclaration, name),
-                         &location);
-        return MaybeHandle<Context>();
+        return isolate->ThrowAt<Context>(
+            isolate->factory()->NewSyntaxError(
+                MessageTemplate::kVarRedeclaration, name),
+            &location);
       }
 
       JSGlobalObject::InvalidatePropertyCell(global_object, name);

src/execution/isolate.cc

@@ -1417,7 +1417,7 @@ Object Isolate::StackOverflow() {
       ErrorUtils::Construct(this, fun, fun, msg, SKIP_NONE, no_caller,
                             ErrorUtils::StackTraceCollection::kSimple));
 
-  Throw(*exception, nullptr);
+  Throw(*exception);
 
 #ifdef VERIFY_HEAP
   if (FLAG_verify_heap && FLAG_stress_compaction) {
@@ -1429,7 +1429,7 @@ Object Isolate::StackOverflow() {
   return ReadOnlyRoots(heap()).exception();
 }
 
-void Isolate::ThrowAt(Handle<JSObject> exception, MessageLocation* location) {
+Object Isolate::ThrowAt(Handle<JSObject> exception, MessageLocation* location) {
   Handle<Name> key_start_pos = factory()->error_start_pos_symbol();
   Object::SetProperty(this, exception, key_start_pos,
                       handle(Smi::FromInt(location->start_pos()), this),
@@ -1450,11 +1450,11 @@ void Isolate::ThrowAt(Handle<JSObject> exception, MessageLocation* location) {
                       Just(ShouldThrow::kThrowOnError))
       .Check();
 
-  Throw(*exception, location);
+  return ThrowInternal(*exception, location);
 }
 
 Object Isolate::TerminateExecution() {
-  return Throw(ReadOnlyRoots(this).termination_exception(), nullptr);
+  return Throw(ReadOnlyRoots(this).termination_exception());
 }
 
 void Isolate::CancelTerminateExecution() {
@@ -1584,7 +1584,7 @@ Handle<JSMessageObject> Isolate::CreateMessageOrAbort(
   return message_obj;
 }
 
-Object Isolate::Throw(Object raw_exception, MessageLocation* location) {
+Object Isolate::ThrowInternal(Object raw_exception, MessageLocation* location) {
   DCHECK(!has_pending_exception());
 
   HandleScope scope(this);

src/execution/isolate.h

@@ -819,17 +819,22 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   // Exception throwing support. The caller should use the result
   // of Throw() as its return value.
-  Object Throw(Object exception, MessageLocation* location = nullptr);
+  Object Throw(Object exception) { return ThrowInternal(exception, nullptr); }
+  Object ThrowAt(Handle<JSObject> exception, MessageLocation* location);
   Object ThrowIllegalOperation();
 
   template <typename T>
-  V8_WARN_UNUSED_RESULT MaybeHandle<T> Throw(
-      Handle<Object> exception, MessageLocation* location = nullptr) {
-    Throw(*exception, location);
+  V8_WARN_UNUSED_RESULT MaybeHandle<T> Throw(Handle<Object> exception) {
+    Throw(*exception);
     return MaybeHandle<T>();
   }
 
-  void ThrowAt(Handle<JSObject> exception, MessageLocation* location);
+  template <typename T>
+  V8_WARN_UNUSED_RESULT MaybeHandle<T> ThrowAt(Handle<JSObject> exception,
+                                               MessageLocation* location) {
+    ThrowAt(exception, location);
+    return MaybeHandle<T>();
+  }
 
   void FatalProcessOutOfHeapMemory(const char* location) {
     heap()->FatalProcessOutOfMemory(location);
@@ -1716,6 +1721,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   void AddCrashKeysForIsolateAndHeapPointers();
 
+  // Returns the Exception sentinel.
+  Object ThrowInternal(Object exception, MessageLocation* location);
+
   // This class contains a collection of data accessible from both C++ runtime
   // and compiled code (including assembly stubs, builtins, interpreter bytecode
   // handlers and optimized code).

src/execution/messages.cc

@@ -1302,13 +1302,12 @@ MessageTemplate UpdateErrorTemplate(CallPrinter::ErrorHint hint,
     case CallPrinter::ErrorHint::kNone:
       return default_id;
   }
-  return default_id;
 }
 
 }  // namespace
 
-Handle<Object> ErrorUtils::NewIteratorError(Isolate* isolate,
-                                            Handle<Object> source) {
+Handle<JSObject> ErrorUtils::NewIteratorError(Isolate* isolate,
+                                              Handle<Object> source) {
   MessageLocation location;
   CallPrinter::ErrorHint hint = CallPrinter::kNone;
   Handle<String> callsite = RenderCallSite(isolate, source, &location, &hint);
@@ -1352,13 +1351,13 @@ Object ErrorUtils::ThrowSpreadArgError(Isolate* isolate, MessageTemplate id,
     }
   }
 
-  Handle<Object> exception =
-      isolate->factory()->NewTypeError(id, callsite, object);
-  return isolate->Throw(*exception, &location);
+  isolate->ThrowAt(isolate->factory()->NewTypeError(id, callsite, object),
+                   &location);
+  return ReadOnlyRoots(isolate).exception();
 }
 
-Handle<Object> ErrorUtils::NewCalledNonCallableError(Isolate* isolate,
-                                                     Handle<Object> source) {
+Handle<JSObject> ErrorUtils::NewCalledNonCallableError(Isolate* isolate,
+                                                       Handle<Object> source) {
   MessageLocation location;
   CallPrinter::ErrorHint hint = CallPrinter::kNone;
   Handle<String> callsite = RenderCallSite(isolate, source, &location, &hint);
@@ -1367,7 +1366,7 @@ Handle<Object> ErrorUtils::NewCalledNonCallableError(Isolate* isolate,
   return isolate->factory()->NewTypeError(id, callsite);
 }
 
-Handle<Object> ErrorUtils::NewConstructedNonConstructable(
+Handle<JSObject> ErrorUtils::NewConstructedNonConstructable(
     Isolate* isolate, Handle<Object> source) {
   MessageLocation location;
   CallPrinter::ErrorHint hint = CallPrinter::kNone;
@@ -1376,10 +1375,6 @@ Handle<Object> ErrorUtils::NewConstructedNonConstructable(
   return isolate->factory()->NewTypeError(id, callsite);
 }
 
-Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
-                                                Handle<Object> object) {
-  return ThrowLoadFromNullOrUndefined(isolate, object, MaybeHandle<Object>());
-}
 Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
                                                 Handle<Object> object,
                                                 MaybeHandle<Object> key) {
@@ -1452,7 +1447,7 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
     callsite = BuildDefaultCallSite(isolate, object);
   }
 
-  Handle<Object> error;
+  Handle<JSObject> error;
   Handle<String> property_name;
   if (is_destructuring) {
     if (maybe_property_name.ToHandle(&property_name)) {
@@ -1476,7 +1471,12 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
     }
   }
 
-  return isolate->Throw(*error, location_computed ? &location : nullptr);
+  if (location_computed) {
+    isolate->ThrowAt(error, &location);
+  } else {
+    isolate->Throw(*error);
+  }
+  return ReadOnlyRoots(isolate).exception();
 }
 
 }  // namespace internal

src/execution/messages.h

@@ -297,16 +297,16 @@ class ErrorUtils : public AllStatic {
                                               Handle<JSObject> error,
                                               Handle<Object> stack_trace);
 
-  static Handle<Object> NewIteratorError(Isolate* isolate,
-                                         Handle<Object> source);
-  static Handle<Object> NewCalledNonCallableError(Isolate* isolate,
-                                                  Handle<Object> source);
-  static Handle<Object> NewConstructedNonConstructable(Isolate* isolate,
-                                                       Handle<Object> source);
+  static Handle<JSObject> NewIteratorError(Isolate* isolate,
+                                           Handle<Object> source);
+  static Handle<JSObject> NewCalledNonCallableError(Isolate* isolate,
+                                                    Handle<Object> source);
+  static Handle<JSObject> NewConstructedNonConstructable(Isolate* isolate,
+                                                         Handle<Object> source);
+  // Returns the Exception sentinel.
   static Object ThrowSpreadArgError(Isolate* isolate, MessageTemplate id,
                                     Handle<Object> object);
-  static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,
-                                             Handle<Object> object);
+  // Returns the Exception sentinel.
   static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,
                                              Handle<Object> object,
                                              MaybeHandle<Object> key);

src/json/json-parser.cc

@@ -273,8 +273,7 @@ void JsonParser<Char>::ReportUnexpectedToken(JsonToken token) {
   // separated source file.
   isolate()->debug()->OnCompileError(script);
   MessageLocation location(script, pos, pos + 1);
-  Handle<Object> error = factory->NewSyntaxError(message, arg1, arg2);
-  isolate()->Throw(*error, &location);
+  isolate()->ThrowAt(factory->NewSyntaxError(message, arg1, arg2), &location);
 
   // Move the cursor to the end so we won't be able to proceed parsing.
   cursor_ = end_;

src/objects/source-text-module.cc

@@ -189,7 +189,7 @@ MaybeHandle<Cell> SourceTextModule::ResolveExport(
     } else if (name_set->count(export_name)) {
       // Cycle detected.
       if (must_resolve) {
-        return isolate->Throw<Cell>(
+        return isolate->ThrowAt<Cell>(
             isolate->factory()->NewSyntaxError(
                 MessageTemplate::kCyclicModuleDependency, export_name,
                 module_specifier),
@@ -274,10 +274,10 @@ MaybeHandle<Cell> SourceTextModule::ResolveExportUsingStarExports(
               .ToHandle(&cell)) {
         if (unique_cell.is_null()) unique_cell = cell;
         if (*unique_cell != *cell) {
-          return isolate->Throw<Cell>(isolate->factory()->NewSyntaxError(
-                                          MessageTemplate::kAmbiguousExport,
-                                          module_specifier, export_name),
-                                      &loc);
+          return isolate->ThrowAt<Cell>(isolate->factory()->NewSyntaxError(
+                                            MessageTemplate::kAmbiguousExport,
+                                            module_specifier, export_name),
+                                        &loc);
         }
       } else if (isolate->has_pending_exception()) {
         return MaybeHandle<Cell>();
@@ -296,7 +296,7 @@ MaybeHandle<Cell> SourceTextModule::ResolveExportUsingStarExports(
 
   // Unresolvable.
   if (must_resolve) {
-    return isolate->Throw<Cell>(
+    return isolate->ThrowAt<Cell>(
         isolate->factory()->NewSyntaxError(MessageTemplate::kUnresolvableExport,
                                            module_specifier, export_name),
         &loc);

src/objects/synthetic-module.cc

@@ -59,7 +59,7 @@ MaybeHandle<Cell> SyntheticModule::ResolveExport(
 
   if (!must_resolve) return MaybeHandle<Cell>();
 
-  return isolate->Throw<Cell>(
+  return isolate->ThrowAt<Cell>(
       isolate->factory()->NewSyntaxError(MessageTemplate::kUnresolvableExport,
                                          module_specifier, export_name),
       &loc);

src/runtime/runtime-object.cc

@@ -1068,7 +1068,8 @@ RUNTIME_FUNCTION(Runtime_CopyDataPropertiesWithExcludedProperties) {
 
   // If source is undefined or null, throw a non-coercible error.
   if (source->IsNullOrUndefined(isolate)) {
-    return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, source);
+    return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, source,
+                                                    MaybeHandle<Object>());
   }
 
   ScopedVector<Handle<Object>> excluded_properties(args.length() - 1);

test/mjsunit/harmony/import-from-instantiation-errored.js

@@ -2,7 +2,7 @@
 // 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-dynamic-import
+// Flags: --allow-natives-syntax --harmony-dynamic-import --no-stress-snapshot
 
 var error1, error2;
 import('modules-skip-10.mjs').catch(e => error1 = e);