[wasm] Use pending exceptions consistently

In our internal code, we should only use pending exceptions. They will
be converted to scheduled exceptions on the API boundary.
Hence, the ErrorThrower just sets a pending exception; it should never
have to think about scheduled exceptions. The new
ScheduledErrorThrower inherits from ErrorThrower and reschedules any
pending exceptions in its destructor (turning them into scheduled
exceptions).
In some situations, there might already be a scheduled exception, e.g.
when calling other API methods (v8::Value::Get). In this case, the
ErrorThrower should also not set another pending exception. For the
reasons mentioned above, this can only be handled in the
ScheduledErrorThrower, which is used the API methods.

This fixes one DCHECK failure and one TODO about scheduled exceptions
if no instance can be created, because the start function throws.

R=mtrofin@chromium.org, mstarzinger@chromium.org
BUG=v8:6232,chromium:736256

Change-Id: I4905be04c565df9495de18fb26adbb5c05d193d2
Reviewed-on: https://chromium-review.googlesource.com/548641
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46314}

src/wasm/module-compiler.cc

@@ -1121,7 +1121,6 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
 
     if (retval.is_null()) {
       DCHECK(isolate_->has_pending_exception());
-      isolate_->OptionalRescheduleException(false);
       // It's unfortunate that the new instance is already linked in the
       // chain. However, we need to set up everything before executing the
       // start function, such that stack trace information can be generated

src/wasm/wasm-js.cc

@@ -40,6 +40,35 @@ namespace {
     }                                                \
   } while (false)
 
+// Like an ErrorThrower, but turns all pending exceptions into scheduled
+// exceptions when going out of scope. Use this in API methods.
+// Note that pending exceptions are not necessarily created by the ErrorThrower,
+// but e.g. by the wasm start function. There might also be a scheduled
+// exception, created by another API call (e.g. v8::Object::Get). But there
+// should never be both pending and scheduled exceptions.
+class ScheduledErrorThrower : public ErrorThrower {
+ public:
+  ScheduledErrorThrower(v8::Isolate* isolate, const char* context)
+      : ScheduledErrorThrower(reinterpret_cast<i::Isolate*>(isolate), context) {
+  }
+  ScheduledErrorThrower(i::Isolate* isolate, const char* context)
+      : ErrorThrower(isolate, context) {}
+  ~ScheduledErrorThrower() {
+    // There should never be both a pending and a scheduled exception.
+    DCHECK(!isolate()->has_scheduled_exception() ||
+           !isolate()->has_pending_exception());
+    // Don't throw another error if there is already a scheduled error.
+    if (isolate()->has_scheduled_exception()) {
+      Reset();
+    } else if (isolate()->has_pending_exception()) {
+      Reset();
+      isolate()->OptionalRescheduleException(false);
+    } else if (error()) {
+      isolate()->ScheduleThrow(*Reify());
+    }
+  }
+};
+
 // TODO(wasm): move brand check to the respective types, and don't throw
 // in it, rather, use a provided ErrorThrower, or let caller handle it.
 static bool HasBrand(i::Handle<i::Object> value, i::Handle<i::Symbol> sym) {
@@ -152,7 +181,7 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) {
   MicrotasksScope runs_microtasks(isolate, MicrotasksScope::kRunMicrotasks);
 
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.compile()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.compile()");
 
   Local<Context> context = isolate->GetCurrentContext();
   ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context));
@@ -175,7 +204,7 @@ void WebAssemblyValidate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.validate()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.validate()");
 
   auto bytes = GetFirstArgumentAsBytes(args, &thrower);
 
@@ -196,7 +225,7 @@ void WebAssemblyModule(const v8::FunctionCallbackInfo<v8::Value>& args) {
   if (i_isolate->wasm_module_callback()(args)) return;
 
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Module()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Module()");
 
   auto bytes = GetFirstArgumentAsBytes(args, &thrower);
 
@@ -216,7 +245,7 @@ void WebAssemblyModuleImports(const v8::FunctionCallbackInfo<v8::Value>& args) {
   HandleScope scope(args.GetIsolate());
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Module.imports()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Module.imports()");
 
   auto maybe_module = GetFirstArgumentAsModule(args, &thrower);
   if (thrower.error()) return;
@@ -229,7 +258,7 @@ void WebAssemblyModuleExports(const v8::FunctionCallbackInfo<v8::Value>& args) {
   HandleScope scope(args.GetIsolate());
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Module.exports()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Module.exports()");
 
   auto maybe_module = GetFirstArgumentAsModule(args, &thrower);
   if (thrower.error()) return;
@@ -243,7 +272,8 @@ void WebAssemblyModuleCustomSections(
   HandleScope scope(args.GetIsolate());
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Module.customSections()");
+  ScheduledErrorThrower thrower(i_isolate,
+                                "WebAssembly.Module.customSections()");
 
   auto maybe_module = GetFirstArgumentAsModule(args, &thrower);
   if (thrower.error()) return;
@@ -271,7 +301,9 @@ MaybeLocal<Value> WebAssemblyInstantiateImpl(Isolate* isolate,
                                              Local<Value> ffi) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
 
-  ErrorThrower thrower(i_isolate, "WebAssembly Instantiation");
+  i::MaybeHandle<i::Object> instance_object;
+  {
+    ScheduledErrorThrower thrower(i_isolate, "WebAssembly Instantiation");
     i::MaybeHandle<i::JSReceiver> maybe_imports =
         GetValueAsImports(ffi, &thrower);
     if (thrower.error()) return {};
@@ -279,16 +311,13 @@ MaybeLocal<Value> WebAssemblyInstantiateImpl(Isolate* isolate,
     i::Handle<i::WasmModuleObject> module_obj =
         i::Handle<i::WasmModuleObject>::cast(
             Utils::OpenHandle(Object::Cast(*module)));
-  i::MaybeHandle<i::Object> instance_object =
+    instance_object =
         i::wasm::SyncInstantiate(i_isolate, &thrower, module_obj, maybe_imports,
                                  i::MaybeHandle<i::JSArrayBuffer>());
-
-  if (instance_object.is_null()) {
-    // TODO(wasm): this *should* mean there's an error to throw, but
-    // we exit sometimes the instantiation pipeline without throwing.
-    // v8:6232.
-    return {};
   }
+
+  DCHECK_EQ(instance_object.is_null(), i_isolate->has_scheduled_exception());
+  if (instance_object.is_null()) return {};
   return Utils::ToLocal(instance_object.ToHandleChecked());
 }
 
@@ -354,7 +383,7 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   if (i_isolate->wasm_instance_callback()(args)) return;
 
-  ErrorThrower thrower(i_isolate, "WebAssembly.Instance()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Instance()");
 
   GetFirstArgumentAsModule(args, &thrower);
   if (thrower.error()) return;
@@ -404,7 +433,7 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   MicrotasksScope runs_microtasks(isolate, MicrotasksScope::kRunMicrotasks);
 
-  ErrorThrower thrower(i_isolate, "WebAssembly.instantiate()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.instantiate()");
 
   HandleScope scope(isolate);
 
@@ -491,7 +520,7 @@ void WebAssemblyTable(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Module()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Module()");
   if (args.Length() < 1 || !args[0]->IsObject()) {
     thrower.TypeError("Argument 0 must be a table descriptor");
     return;
@@ -544,7 +573,7 @@ void WebAssemblyMemory(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Memory()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Memory()");
   if (args.Length() < 1 || !args[0]->IsObject()) {
     thrower.TypeError("Argument 0 must be a memory descriptor");
     return;
@@ -588,7 +617,7 @@ void WebAssemblyTableGetLength(
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Table.length()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Table.length()");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),
@@ -607,7 +636,7 @@ void WebAssemblyTableGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Table.grow()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Table.grow()");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),
@@ -660,7 +689,7 @@ void WebAssemblyTableGet(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Table.get()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Table.get()");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),
@@ -689,7 +718,7 @@ void WebAssemblyTableSet(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Table.set()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Table.set()");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),
@@ -731,7 +760,7 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Memory.grow()");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Memory.grow()");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),
@@ -778,7 +807,7 @@ void WebAssemblyMemoryGetBuffer(
   v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.Memory.buffer");
+  ScheduledErrorThrower thrower(i_isolate, "WebAssembly.Memory.buffer");
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(Utils::OpenHandle(*args.This()),

src/wasm/wasm-module.cc

@@ -808,11 +808,9 @@ MaybeHandle<WasmInstanceObject> wasm::SyncCompileAndInstantiate(
   DCHECK_EQ(thrower->error(), module.is_null());
   if (module.is_null()) return {};
 
-  MaybeHandle<WasmInstanceObject> instance = wasm::SyncInstantiate(
-      isolate, thrower, module.ToHandleChecked(), Handle<JSReceiver>::null(),
+  return wasm::SyncInstantiate(isolate, thrower, module.ToHandleChecked(),
+                               Handle<JSReceiver>::null(),
                                Handle<JSArrayBuffer>::null());
-  DCHECK_EQ(thrower->error(), instance.is_null());
-  return instance;
 }
 
 namespace {

src/wasm/wasm-result.cc

@@ -136,10 +136,8 @@ Handle<Object> ErrorThrower::Reify() {
   Vector<const char> msg_vec(error_msg_.data(), error_msg_.size());
   Handle<String> message =
       isolate_->factory()->NewStringFromUtf8(msg_vec).ToHandleChecked();
-  error_type_ = kNone;  // Reset.
-  Handle<Object> exception =
-      isolate_->factory()->NewError(constructor, message);
-  return exception;
+  Reset();
+  return isolate_->factory()->NewError(constructor, message);
 }
 
 void ErrorThrower::Reset() {
@@ -157,7 +155,10 @@ ErrorThrower::ErrorThrower(ErrorThrower&& other)
 
 ErrorThrower::~ErrorThrower() {
   if (error() && !isolate_->has_pending_exception()) {
-    isolate_->ScheduleThrow(*Reify());
+    // We don't want to mix pending exceptions and scheduled exceptions, hence
+    // an existing exception should be pending, never scheduled.
+    DCHECK(!isolate_->has_scheduled_exception());
+    isolate_->Throw(*Reify());
   }
 }
 

src/wasm/wasm-result.h

@@ -123,6 +123,8 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   bool error() const { return error_type_ != kNone; }
   bool wasm_error() { return error_type_ >= kFirstWasmError; }
 
+  Isolate* isolate() const { return isolate_; }
+
  private:
   enum ErrorType {
     kNone,
@@ -146,6 +148,9 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   std::string error_msg_;
 
   DISALLOW_COPY_AND_ASSIGN(ErrorThrower);
+  // ErrorThrower should always be stack-allocated, since it constitutes a scope
+  // (things happen in the destructor).
+  DISALLOW_NEW_AND_DELETE();
 };
 
 }  // namespace wasm

test/common/wasm/wasm-module-runner.cc

@@ -134,6 +134,8 @@ int32_t CallWasmFunctionForTesting(Isolate* isolate, Handle<JSObject> instance,
 
   // The result should be a number.
   if (retval.is_null()) {
+    DCHECK(isolate->has_pending_exception());
+    isolate->clear_pending_exception();
     thrower->RuntimeError("Calling exported wasm function failed.");
     return -1;
   }

test/common/wasm/wasm-module-runner.h

@@ -27,6 +27,9 @@ std::unique_ptr<WasmModule> DecodeWasmModuleForTesting(
     Isolate* isolate, ErrorThrower* thrower, const byte* module_start,
     const byte* module_end, ModuleOrigin origin, bool verify_functions = false);
 
+// Call an exported wasm function by name. Returns -1 if the export does not
+// exist or throws an error. Errors are cleared from the isolate before
+// returning.
 int32_t CallWasmFunctionForTesting(Isolate* isolate, Handle<JSObject> instance,
                                    ErrorThrower* thrower, const char* name,
                                    int argc, Handle<Object> argv[]);

test/mjsunit/wasm/start-function.js

@@ -29,7 +29,7 @@ function assertVerifies(sig, body) {
 
 assertVerifies(kSig_v_v, [kExprNop]);
 
-// Arguments aren't allow to start functions.
+// Arguments aren't allowed to start functions.
 assertThrows(() => {instantiate(kSig_i_i, [kExprGetLocal, 0]);});
 assertThrows(() => {instantiate(kSig_i_ii, [kExprGetLocal, 0]);});
 assertThrows(() => {instantiate(kSig_i_dd, [kExprGetLocal, 0]);});
@@ -122,3 +122,27 @@ assertThrows(() => {instantiate(kSig_i_v, [kExprI32Const, 0]);});
   var module = builder.instantiate(ffi);
   assertTrue(ranned);
 })();
+
+(function testStartFunctionThrowsExplicitly() {
+  print('testStartFunctionThrowsExplicitly');
+  let error = new Error('my explicit error');
+  function throw_fn() {
+    throw error;
+  }
+  let builder = new WasmModuleBuilder();
+  builder.addImport('foo', 'bar', kSig_v_v);
+  let func = builder.addFunction('', kSig_v_v).addBody([kExprCallFunction, 0]);
+  builder.addStart(func.index);
+
+  assertThrowsEquals(() => builder.instantiate(ffi), error);
+})();
+
+(function testStartFunctionThrowsImplicitly() {
+  print("testStartFunctionThrowsImplicitly");
+  let builder = new WasmModuleBuilder();
+  let func = builder.addFunction('', kSig_v_v).addBody([kExprUnreachable]);
+  builder.addStart(func.index);
+
+  assertThrows(
+      () => builder.instantiate(), WebAssembly.RuntimeError, /unreachable/);
+})();