[wasm] Refactor and fix ErrorThrower

The error thrower did allocate the exception at the moment the error was
detected. For async compilation, this meant in another step than when
it was actually thrown. Since the HandleScope of the exception already
died at that point, this would have lead to memory errors.

With this refactoring, we only store the information needed to generate
the exception in the ErrorThrower, and only generate the exception
object once it is actually needed.

With regression test.

R=ahaas@chromium.org, mtrofin@chromium.org
Also-by: ahaas@chromium.org

Change-Id: Iffcab1f8d1cf5925e3643fcf0729ba9a84c7d277
Reviewed-on: https://chromium-review.googlesource.com/490085
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45000}

src/wasm/wasm-result.cc

@@ -26,15 +26,27 @@ void VPrintFToString(std::string& str, size_t str_offset, const char* format,
   for (;; len = base::bits::RoundUpToPowerOfTwo64(len + 1)) {
     DCHECK_GE(kMaxInt, len);
     str.resize(len);
+    va_list args_copy;
+    va_copy(args_copy, args);
     int written = VSNPrintF(Vector<char>(&str.front() + str_offset,
                                          static_cast<int>(len - str_offset)),
-                            format, args);
+                            format, args_copy);
+    va_end(args_copy);
     if (written < 0) continue;  // not enough space.
     str.resize(str_offset + written);
     return;
   }
 }
 
+PRINTF_FORMAT(3, 4)
+void PrintFToString(std::string& str, size_t str_offset, const char* format,
+                    ...) {
+  va_list args;
+  va_start(args, format);
+  VPrintFToString(str, str_offset, format, args);
+  va_end(args);
+}
+
 }  // namespace
 
 void ResultBase::error(uint32_t offset, std::string error_msg) {
@@ -51,80 +63,93 @@ void ResultBase::verror(const char* format, va_list args) {
   if (error_msg_.empty() == 0) error_msg_.assign("Error");
 }
 
-void ErrorThrower::Format(i::Handle<i::JSFunction> constructor,
-                          const char* format, va_list args) {
+void ErrorThrower::Format(ErrorType type, const char* format, va_list args) {
+  DCHECK_NE(kNone, type);
   // Only report the first error.
   if (error()) return;
 
-  constexpr int kMaxErrorMessageLength = 256;
-  EmbeddedVector<char, kMaxErrorMessageLength> buffer;
-
-  int context_len = 0;
+  size_t context_len = 0;
   if (context_) {
-    context_len = SNPrintF(buffer, "%s: ", context_);
-    CHECK_LE(0, context_len);  // check for overflow.
+    PrintFToString(error_msg_, 0, "%s: ", context_);
+    context_len = error_msg_.size();
   }
-
-  int message_len =
-      VSNPrintF(buffer.SubVector(context_len, buffer.length()), format, args);
-  CHECK_LE(0, message_len);  // check for overflow.
-
-  Vector<char> whole_message = buffer.SubVector(0, context_len + message_len);
-  i::Handle<i::String> message =
-      isolate_->factory()
-          ->NewStringFromOneByte(Vector<uint8_t>::cast(whole_message))
-          .ToHandleChecked();
-  exception_ = isolate_->factory()->NewError(constructor, message);
+  VPrintFToString(error_msg_, context_len, format, args);
+  error_type_ = type;
 }
 
 void ErrorThrower::TypeError(const char* format, ...) {
-  if (error()) return;
   va_list arguments;
   va_start(arguments, format);
-  Format(isolate_->type_error_function(), format, arguments);
+  Format(kTypeError, format, arguments);
   va_end(arguments);
 }
 
 void ErrorThrower::RangeError(const char* format, ...) {
-  if (error()) return;
   va_list arguments;
   va_start(arguments, format);
-  Format(isolate_->range_error_function(), format, arguments);
+  Format(kRangeError, format, arguments);
   va_end(arguments);
 }
 
 void ErrorThrower::CompileError(const char* format, ...) {
-  if (error()) return;
-  wasm_error_ = true;
   va_list arguments;
   va_start(arguments, format);
-  Format(isolate_->wasm_compile_error_function(), format, arguments);
+  Format(kCompileError, format, arguments);
   va_end(arguments);
 }
 
 void ErrorThrower::LinkError(const char* format, ...) {
-  if (error()) return;
-  wasm_error_ = true;
   va_list arguments;
   va_start(arguments, format);
-  Format(isolate_->wasm_link_error_function(), format, arguments);
+  Format(kLinkError, format, arguments);
   va_end(arguments);
 }
 
 void ErrorThrower::RuntimeError(const char* format, ...) {
-  if (error()) return;
-  wasm_error_ = true;
   va_list arguments;
   va_start(arguments, format);
-  Format(isolate_->wasm_runtime_error_function(), format, arguments);
+  Format(kRuntimeError, format, arguments);
   va_end(arguments);
 }
 
+Handle<Object> ErrorThrower::Reify() {
+  Handle<JSFunction> constructor;
+  switch (error_type_) {
+    case kNone:
+      UNREACHABLE();
+    case kTypeError:
+      constructor = isolate_->type_error_function();
+      break;
+    case kRangeError:
+      constructor = isolate_->range_error_function();
+      break;
+    case kCompileError:
+      constructor = isolate_->wasm_compile_error_function();
+      break;
+    case kLinkError:
+      constructor = isolate_->wasm_link_error_function();
+      break;
+    case kRuntimeError:
+      constructor = isolate_->wasm_runtime_error_function();
+      break;
+  }
+  Vector<const uint8_t> msg_vec(
+      reinterpret_cast<const uint8_t*>(error_msg_.data()),
+      static_cast<int>(error_msg_.size()));
+  Handle<String> message =
+      isolate_->factory()->NewStringFromOneByte(msg_vec).ToHandleChecked();
+  error_type_ = kNone;  // Reset.
+  Handle<Object> exception =
+      isolate_->factory()->NewError(constructor, message);
+  return exception;
+}
+
 ErrorThrower::~ErrorThrower() {
   if (error() && !isolate_->has_pending_exception()) {
-    isolate_->ScheduleThrow(*exception_);
+    isolate_->ScheduleThrow(*Reify());
   }
 }
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/wasm-result.h

@@ -95,7 +95,7 @@ class Result : public ResultBase {
 // A helper for generating error messages that bubble up to JS exceptions.
 class V8_EXPORT_PRIVATE ErrorThrower {
  public:
-  ErrorThrower(i::Isolate* isolate, const char* context)
+  ErrorThrower(Isolate* isolate, const char* context)
       : isolate_(isolate), context_(context) {}
   ~ErrorThrower();
 
@@ -112,22 +112,32 @@ class V8_EXPORT_PRIVATE ErrorThrower {
                  result.error_offset());
   }
 
-  i::Handle<i::Object> Reify() {
-    i::Handle<i::Object> result = exception_;
-    exception_ = i::Handle<i::Object>::null();
-    return result;
-  }
+  Handle<Object> Reify();
 
-  bool error() const { return !exception_.is_null(); }
-  bool wasm_error() { return wasm_error_; }
+  bool error() const { return error_type_ != kNone; }
+  bool wasm_error() { return error_type_ >= kFirstWasmError; }
 
  private:
-  void Format(i::Handle<i::JSFunction> constructor, const char* fmt, va_list);
-
-  i::Isolate* isolate_;
+  enum ErrorType {
+    kNone,
+    // General errors.
+    kTypeError,
+    kRangeError,
+    // Wasm errors.
+    kCompileError,
+    kLinkError,
+    kRuntimeError,
+
+    // Marker.
+    kFirstWasmError = kCompileError
+  };
+
+  void Format(ErrorType error_type_, const char* fmt, va_list);
+
+  Isolate* isolate_;
   const char* context_;
-  i::Handle<i::Object> exception_;
-  bool wasm_error_ = false;
+  ErrorType error_type_ = kNone;
+  std::string error_msg_;
 };
 
 }  // namespace wasm

test/mjsunit/mjsunit.js

@@ -504,7 +504,7 @@ var failWithMessage;
       fail = result => failWithMessage("assertPromiseResult failed: " + result);
 
     eval("%IncrementWaitCount()");
-    promise.then(
+    return promise.then(
       result => {
         eval("%DecrementWaitCount()");
         success(result);

test/mjsunit/wasm/async-compile.js

@@ -7,43 +7,73 @@
 load("test/mjsunit/wasm/wasm-constants.js");
 load("test/mjsunit/wasm/wasm-module-builder.js");
 
-let ok_buffer = (() => {
-  var builder = new WasmModuleBuilder();
-  builder.addFunction("f", kSig_i_v)
-    .addBody([kExprI32Const, 42])
-    .exportAs("f");
-  return builder.toBuffer();
-})();
-
-// The OK buffer validates and can be made into a module.
-assertTrue(WebAssembly.validate(ok_buffer));
-let ok_module = new WebAssembly.Module(ok_buffer);
-assertTrue(ok_module instanceof WebAssembly.Module);
-
-// The bad buffer does not validate and cannot be made into a module.
-let bad_buffer = new ArrayBuffer(0);
-assertFalse(WebAssembly.validate(bad_buffer));
-assertThrows(() => new WebAssembly.Module(bad_buffer), WebAssembly.CompileError);
-
-function checkModule(module) {
-  assertTrue(module instanceof WebAssembly.Module);
+function assertCompiles(buffer) {
+  return assertPromiseResult(
+      WebAssembly.compile(buffer),
+      module => assertTrue(module instanceof WebAssembly.Module),
+      ex => assertUnreachable);
 }
 
-function checkCompileError(ex) {
-  assertTrue(ex instanceof WebAssembly.CompileError);
+function assertCompileError(buffer) {
+  return assertPromiseResult(
+      WebAssembly.compile(buffer), module => assertUnreachable,
+      ex => assertTrue(ex instanceof WebAssembly.CompileError));
 }
 
-let kNumCompiles = 3;
+// These tests execute asynchronously. In order to avoid executing several tests
+// concurrently (which makes debugging much harder), build a promise chain to
+// start the next task only after the previous one ended.
 
-// Three compilations of the OK module should succeed.
-for (var i = 0; i < kNumCompiles; i++) {
-  assertPromiseResult(WebAssembly.compile(ok_buffer), checkModule,
-              (ex) => assertUnreachable);
-}
+let testChain = Promise.resolve();
+let addTest = fun => testChain = testChain.then(() => fun());
 
-// Three compilations of the bad module should fail.
-for (var i = 0; i < kNumCompiles; i++) {
-  assertPromiseResult(WebAssembly.compile(bad_buffer),
-              (module) => assertUnreachable,
-              checkCompileError);
-}
+addTest(async function basicCompile() {
+  let ok_buffer = (() => {
+    var builder = new WasmModuleBuilder();
+    builder.addFunction('f', kSig_i_v)
+        .addBody([kExprI32Const, 42])
+        .exportAs('f');
+    return builder.toBuffer();
+  })();
+
+  // The OK buffer validates and can be made into a module.
+  assertTrue(WebAssembly.validate(ok_buffer));
+  let ok_module = new WebAssembly.Module(ok_buffer);
+  assertTrue(ok_module instanceof WebAssembly.Module);
+
+  // The bad buffer does not validate and cannot be made into a module.
+  let bad_buffer = new ArrayBuffer(0);
+  assertFalse(WebAssembly.validate(bad_buffer));
+  assertThrows(
+      () => new WebAssembly.Module(bad_buffer), WebAssembly.CompileError);
+
+  let kNumCompiles = 3;
+
+  // Three compilations of the OK module should succeed.
+  for (var i = 0; i < kNumCompiles; i++) {
+    await assertCompiles(ok_buffer);
+  }
+
+  // Three compilations of the bad module should fail.
+  for (var i = 0; i < kNumCompiles; i++) {
+    await assertCompileError(bad_buffer);
+  }
+});
+
+addTest(async function badFunctionInTheMiddle() {
+  // We had an error where an exception was generated by a background task and
+  // later thrown in a foreground task. The handle to the exception died
+  // inbetween, since the HandleScope was left.
+  // This test reproduced that error.
+  let builder = new WasmModuleBuilder();
+  let sig = builder.addType(kSig_i_v);
+  for (var i = 0; i < 10; ++i) {
+    builder.addFunction('a' + i, sig).addBody([kExprI32Const, 42]);
+  }
+  builder.addFunction('bad', sig).addBody([]);
+  for (var i = 0; i < 10; ++i) {
+    builder.addFunction('b' + i, sig).addBody([kExprI32Const, 42]);
+  }
+  let buffer = builder.toBuffer();
+  await assertCompileError(buffer);
+});