[wasm] instantiate expressed in terms of compile

Today, the semantics of:

WebAssembly.instantiate

and

WebAssembly.compile().then(new WebAssemblyInstance)

are subtly different, to the point where attempting the proposed
change uncovered bugs.

In the future, it's possible that .instantiate actually have different
semantics - if we pre-specialized to the provided ffi, for example.
Right now that's not the case.

This CL:
- gets our implementation closer to what developers may write using
the compile -> new Instance alternative, in particular wrt promise
creation. By reusing code paths, we uncover more bugs, and keep
maintenance cost lower.

- it gives us the response-based WebAssembly.instantiate implicitly.
Otherwise, we'd need that same implementation on the blink side. The
negative is maintenance: imagine if the bugs I mentioned could only be
found when running in Blink.

BUG=chromium:697028

Review-Url: https://codereview.chromium.org/2806073002
Cr-Commit-Position: refs/heads/master@{#44592}

src/runtime/runtime-test.cc

@@ -111,6 +111,15 @@ bool WasmInstantiateOverride(const v8::FunctionCallbackInfo<v8::Value>& args) {
   return true;
 }
 
+bool GetWasmFromResolvedPromise(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  CHECK(args.Length() == 1);
+  v8::Local<v8::Promise> promise = v8::Local<v8::Promise>::Cast(args[0]);
+  CHECK(promise->State() == v8::Promise::PromiseState::kFulfilled);
+  args.GetReturnValue().Set(promise);
+  return true;
+}
+
 }  // namespace
 
 namespace v8 {
@@ -445,6 +454,24 @@ RUNTIME_FUNCTION(Runtime_ClearFunctionFeedback) {
   return isolate->heap()->undefined_value();
 }
 
+RUNTIME_FUNCTION(Runtime_SetWasmCompileFromPromiseOverload) {
+  v8::ExtensionCallback old = isolate->wasm_compile_callback();
+  HandleScope scope(isolate);
+  Handle<Foreign> ret =
+      isolate->factory()->NewForeign(reinterpret_cast<Address>(old));
+  isolate->set_wasm_compile_callback(GetWasmFromResolvedPromise);
+  return *ret;
+}
+
+RUNTIME_FUNCTION(Runtime_ResetWasmOverloads) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(Foreign, old, 0);
+  isolate->set_wasm_compile_callback(
+      reinterpret_cast<v8::ExtensionCallback>(old->foreign_address()));
+  return isolate->heap()->undefined_value();
+}
+
 RUNTIME_FUNCTION(Runtime_CheckWasmWrapperElision) {
   // This only supports the case where the function being exported
   // calls an intermediate function, and the intermediate function

src/runtime/runtime.h

@@ -616,6 +616,8 @@ namespace internal {
   F(ValidateWasmOrphanedInstance, 1, 1)       \
   F(SetWasmCompileControls, 2, 1)             \
   F(SetWasmInstantiateControls, 0, 1)         \
+  F(SetWasmCompileFromPromiseOverload, 0, 1)  \
+  F(ResetWasmOverloads, 1, 1)                 \
   F(HeapObjectVerify, 1, 1)                   \
   F(WasmNumInterpretedCalls, 1, 1)            \
   F(RedirectToWasmInterpreter, 2, 1)

src/wasm/wasm-js.cc

@@ -32,6 +32,18 @@ namespace v8 {
 
 namespace {
 
+#define ASSIGN(type, var, expr)      \
+  Local<type> var;                   \
+  do {                               \
+    if (!expr.ToLocal(&var)) return; \
+  } while (false)
+
+#define DO_BOOL(expr)                 \
+  do {                                \
+    bool ok;                          \
+    if (!expr.To(&ok) || !ok) return; \
+  } while (false)
+
 // 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) {
@@ -117,16 +129,15 @@ i::wasm::ModuleWireBytes GetFirstArgumentAsBytes(
   return i::wasm::ModuleWireBytes(start, start + length);
 }
 
-i::MaybeHandle<i::JSReceiver> GetSecondArgumentAsImports(
-    const v8::FunctionCallbackInfo<v8::Value>& args, ErrorThrower* thrower) {
-  if (args.Length() < 2) return {};
-  if (args[1]->IsUndefined()) return {};
+i::MaybeHandle<i::JSReceiver> GetValueAsImports(const Local<Value>& arg,
+                                                ErrorThrower* thrower) {
+  if (arg->IsUndefined()) return {};
 
-  if (!args[1]->IsObject()) {
+  if (!arg->IsObject()) {
     thrower->TypeError("Argument 1 must be an object");
     return {};
   }
-  Local<Object> obj = Local<Object>::Cast(args[1]);
+  Local<Object> obj = Local<Object>::Cast(arg);
   return i::Handle<i::JSReceiver>::cast(v8::Utils::OpenHandle(*obj));
 }
 
@@ -140,8 +151,7 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) {
   ErrorThrower thrower(i_isolate, "WebAssembly.compile()");
 
   Local<Context> context = isolate->GetCurrentContext();
-  v8::Local<v8::Promise::Resolver> resolver;
-  if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) return;
+  ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context));
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(resolver->GetPromise());
 
@@ -253,10 +263,70 @@ void WebAssemblyModuleCustomSections(
   args.GetReturnValue().Set(Utils::ToLocal(custom_sections));
 }
 
+// Entered as internal implementation detail of sync and async instantiate.
+// args[0] *must* be a WebAssembly.Module.
+void WebAssemblyInstantiateImpl(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  DCHECK_GE(args.Length(), 1);
+  Local<Value> module = args[0];
+  Local<Value> ffi = args.Data();
+
+  HandleScope scope(args.GetIsolate());
+  v8::Isolate* isolate = args.GetIsolate();
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  ErrorThrower thrower(i_isolate, "WebAssembly Instantiation");
+  i::MaybeHandle<i::JSReceiver> maybe_imports =
+      GetValueAsImports(ffi, &thrower);
+  if (thrower.error()) return;
+
+  i::Handle<i::WasmModuleObject> module_obj =
+      i::Handle<i::WasmModuleObject>::cast(
+          Utils::OpenHandle(Object::Cast(*module)));
+  i::MaybeHandle<i::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;
+  }
+  args.GetReturnValue().Set(Utils::ToLocal(instance_object.ToHandleChecked()));
+}
+
+void WebAssemblyInstantiateToPair(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  DCHECK_GE(args.Length(), 1);
+  Local<Value> module = args[0];
+  Isolate* isolate = args.GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
+
+  const uint8_t* instance_str = reinterpret_cast<const uint8_t*>("instance");
+  const uint8_t* module_str = reinterpret_cast<const uint8_t*>("module");
+  ASSIGN(Function, vanilla_instantiate,
+         Function::New(context, WebAssemblyInstantiateImpl, args.Data()));
+
+  ASSIGN(Value, instance,
+         vanilla_instantiate->Call(context, args.Holder(), 1, &module));
+  Local<Object> ret = Object::New(isolate);
+  ASSIGN(String, instance_name,
+         String::NewFromOneByte(isolate, instance_str,
+                                NewStringType::kInternalized));
+  ASSIGN(String, module_name,
+         String::NewFromOneByte(isolate, module_str,
+                                NewStringType::kInternalized));
+
+  DO_BOOL(ret->CreateDataProperty(context, instance_name, instance));
+  DO_BOOL(ret->CreateDataProperty(context, module_name, module));
+  args.GetReturnValue().Set(ret);
+}
+
 // new WebAssembly.Instance(module, imports) -> WebAssembly.Instance
 void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
   HandleScope scope(args.GetIsolate());
-  v8::Isolate* isolate = args.GetIsolate();
+  Isolate* isolate = args.GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   if (i_isolate->wasm_instance_callback()(args)) return;
 
@@ -265,14 +335,15 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
   auto maybe_module = GetFirstArgumentAsModule(args, &thrower);
   if (thrower.error()) return;
 
-  auto maybe_imports = GetSecondArgumentAsImports(args, &thrower);
-  if (thrower.error()) return;
+  // If args.Length < 2, this will be undefined - see FunctionCallbackInfo.
+  // We'll check for that in WebAssemblyInstantiateImpl.
+  Local<Value> data = args[1];
 
-  i::MaybeHandle<i::Object> instance_object = i::wasm::SyncInstantiate(
-      i_isolate, &thrower, maybe_module.ToHandleChecked(), maybe_imports,
-      i::MaybeHandle<i::JSArrayBuffer>());
-  if (instance_object.is_null()) return;
-  args.GetReturnValue().Set(Utils::ToLocal(instance_object.ToHandleChecked()));
+  ASSIGN(Function, impl,
+         Function::New(context, WebAssemblyInstantiateImpl, data));
+  Local<Value> first_param = args[0];
+  ASSIGN(Value, ret, impl->Call(context, args.Holder(), 1, &first_param));
+  args.GetReturnValue().Set(ret);
 }
 
 // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance
@@ -290,10 +361,9 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
 
-  v8::Local<v8::Promise::Resolver> resolver;
-  if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) return;
-  v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
-  return_value.Set(resolver->GetPromise());
+  ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context));
+  Local<Promise> module_promise = resolver->GetPromise();
+  args.GetReturnValue().Set(module_promise);
 
   if (args.Length() < 1) {
     thrower.TypeError(
@@ -305,7 +375,8 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
 
-  i::Handle<i::Object> first_arg = Utils::OpenHandle(*args[0]);
+  Local<Value> first_arg_value = args[0];
+  i::Handle<i::Object> first_arg = Utils::OpenHandle(*first_arg_value);
   if (!first_arg->IsJSObject()) {
     thrower.TypeError(
         "Argument 0 must be a buffer source or a WebAssembly.Module object");
@@ -315,31 +386,27 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
 
-  auto maybe_imports = GetSecondArgumentAsImports(args, &thrower);
-  if (thrower.error()) {
-    auto maybe = resolver->Reject(context, Utils::ToLocal(thrower.Reify()));
-    CHECK_IMPLIES(!maybe.FromMaybe(false),
-                  i_isolate->has_scheduled_exception());
-    return;
-  }
-  i::Handle<i::JSPromise> promise = Utils::OpenHandle(*resolver->GetPromise());
+  FunctionCallback instantiator = nullptr;
   if (HasBrand(first_arg, i::Handle<i::Symbol>(i_context->wasm_module_sym()))) {
-    // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance
-    auto module_object = GetFirstArgumentAsModule(args, &thrower);
-    i::wasm::AsyncInstantiate(i_isolate, promise,
-                              module_object.ToHandleChecked(), maybe_imports);
+    module_promise = resolver->GetPromise();
+    DO_BOOL(resolver->Resolve(context, first_arg_value));
+    instantiator = WebAssemblyInstantiateImpl;
   } else {
-    // WebAssembly.instantiate(bytes, imports) -> {module, instance}
-    auto bytes = GetFirstArgumentAsBytes(args, &thrower);
-    if (thrower.error()) {
-      auto maybe = resolver->Reject(context, Utils::ToLocal(thrower.Reify()));
-      CHECK_IMPLIES(!maybe.FromMaybe(false),
-                    i_isolate->has_scheduled_exception());
-      return;
-    }
-    i::wasm::AsyncCompileAndInstantiate(i_isolate, promise, bytes,
-                                        maybe_imports);
-  }
+    ASSIGN(Function, async_compile, Function::New(context, WebAssemblyCompile));
+    ASSIGN(Value, async_compile_retval,
+           async_compile->Call(context, args.Holder(), 1, &first_arg_value));
+    module_promise = Local<Promise>::Cast(async_compile_retval);
+    instantiator = WebAssemblyInstantiateToPair;
+  }
+  DCHECK(!module_promise.IsEmpty());
+  DCHECK_NOT_NULL(instantiator);
+  // If args.Length < 2, this will be undefined - see FunctionCallbackInfo.
+  // We'll check for that in WebAssemblyInstantiateImpl.
+  Local<Value> data = args[1];
+  ASSIGN(Function, instantiate_impl,
+         Function::New(context, instantiator, data));
+  ASSIGN(Promise, result, module_promise->Then(context, instantiate_impl));
+  args.GetReturnValue().Set(result);
 }
 
 bool GetIntegerProperty(v8::Isolate* isolate, ErrorThrower* thrower,

src/wasm/wasm-module.cc

@@ -2604,44 +2604,6 @@ void wasm::AsyncInstantiate(Isolate* isolate, Handle<JSPromise> promise,
                  instance_object.ToHandleChecked());
 }
 
-void wasm::AsyncCompileAndInstantiate(Isolate* isolate,
-                                      Handle<JSPromise> promise,
-                                      const ModuleWireBytes& bytes,
-                                      MaybeHandle<JSReceiver> imports) {
-  ErrorThrower thrower(isolate, nullptr);
-
-  // Compile the module.
-  MaybeHandle<WasmModuleObject> module_object =
-      SyncCompile(isolate, &thrower, bytes);
-  if (thrower.error()) {
-    RejectPromise(isolate, handle(isolate->context()), &thrower, promise);
-    return;
-  }
-  Handle<WasmModuleObject> module = module_object.ToHandleChecked();
-
-  // Instantiate the module.
-  MaybeHandle<WasmInstanceObject> instance_object = SyncInstantiate(
-      isolate, &thrower, module, imports, Handle<JSArrayBuffer>::null());
-  if (thrower.error()) {
-    RejectPromise(isolate, handle(isolate->context()), &thrower, promise);
-    return;
-  }
-
-  Handle<JSFunction> object_function =
-      Handle<JSFunction>(isolate->native_context()->object_function(), isolate);
-  Handle<JSObject> ret =
-      isolate->factory()->NewJSObject(object_function, TENURED);
-  Handle<String> module_property_name =
-      isolate->factory()->InternalizeUtf8String("module");
-  Handle<String> instance_property_name =
-      isolate->factory()->InternalizeUtf8String("instance");
-  JSObject::AddProperty(ret, module_property_name, module, NONE);
-  JSObject::AddProperty(ret, instance_property_name,
-                        instance_object.ToHandleChecked(), NONE);
-
-  ResolvePromise(isolate, handle(isolate->context()), promise, ret);
-}
-
 // Encapsulates all the state and steps of an asynchronous compilation.
 // An asynchronous compile job consists of a number of tasks that are executed
 // as foreground and background tasks. Any phase that touches the V8 heap or

src/wasm/wasm-module.h

@@ -472,10 +472,6 @@ V8_EXPORT_PRIVATE void AsyncInstantiate(Isolate* isolate,
                                         Handle<WasmModuleObject> module_object,
                                         MaybeHandle<JSReceiver> imports);
 
-V8_EXPORT_PRIVATE void AsyncCompileAndInstantiate(
-    Isolate* isolate, Handle<JSPromise> promise, const ModuleWireBytes& bytes,
-    MaybeHandle<JSReceiver> imports);
-
 #if V8_TARGET_ARCH_64_BIT
 const bool kGuardRegionsSupported = true;
 #else

test/mjsunit/wasm/instantiate-run-basic.js

@@ -2,19 +2,33 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-wasm
+// Flags: --allow-natives-syntax
 
 load("test/mjsunit/wasm/wasm-constants.js");
 load("test/mjsunit/wasm/wasm-module-builder.js");
 
-(function BasicTest() {
-  var kReturnValue = 15;
+const kReturnValue = 15;
+
+function getBuilder() {
   var builder = new WasmModuleBuilder();
 
   builder.addFunction("main", kSig_i_i)
     .addBody([kExprI32Const, kReturnValue])
     .exportFunc();
+  return builder;
+}
 
+(function BasicTest() {
+  var builder = getBuilder();
   var main = builder.instantiate().exports.main;
   assertEquals(kReturnValue, main());
 })();
+
+(function AsyncTest() {
+  var builder = getBuilder();
+  var buffer = builder.toBuffer();
+  assertPromiseResult(
+    WebAssembly.instantiate(buffer)
+      .then(pair => pair.instance.exports.main(), assertUnreachable)
+      .then(result => assertEquals(kReturnValue, result), assertUnreachable));
+})();

test/mjsunit/wasm/js-api.js

@@ -713,7 +713,6 @@ function assertCompileError(args, err, msg) {
   var error = null;
   assertPromiseResult(compile(...args), unexpectedSuccess, error => {
     assertTrue(error instanceof err);
-    assertTrue(Boolean(error.stack.match('js-api.js')));
     // TODO  assertTrue(Boolean(error.message.match(msg)));
   });
 }
@@ -760,7 +759,6 @@ function assertInstantiateError(args, err, msg) {
   var error = null;
   assertPromiseResult(instantiate(...args), unexpectedSuccess, error => {
     assertTrue(error instanceof err);
-    assertTrue(Boolean(error.stack.match('js-api.js')));
     // TODO assertTrue(Boolean(error.message.match(msg)));
   });
 }

test/mjsunit/wasm/wasm-api-overloading.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+load("test/mjsunit/wasm/wasm-constants.js");
+load("test/mjsunit/wasm/wasm-module-builder.js");
+
+let buffer = (() => {
+  let builder = new WasmModuleBuilder();
+  builder.addFunction("f", kSig_i_v)
+    .addBody([kExprI32Const, 42])
+    .exportAs("f");
+  return builder.toBuffer();
+})();
+
+var module = new WebAssembly.Module(buffer);
+var wrapper = Promise.resolve(module);
+
+assertPromiseResult(
+  WebAssembly.instantiate(wrapper),
+  assertUnreachable,
+  e => assertTrue(e instanceof TypeError));
+
+assertPromiseResult((
+  () => {
+    var old = %SetWasmCompileFromPromiseOverload();
+    var ret = WebAssembly.instantiate(wrapper);
+    %ResetWasmOverloads(old);
+    return ret;
+  })(),
+  pair => {
+    assertTrue(pair.instance instanceof WebAssembly.Instance);
+    assertTrue(pair.module instanceof WebAssembly.Module)
+  },
+  assertUnreachable);