Revert of [wasm] instantiate expressed in terms of compile (patchset #6...

Revert of [wasm] instantiate expressed in terms of compile (patchset #6 id:140001 of https://codereview.chromium.org/2806073002/ )

Reason for revert:
Roll blocker: https://bugs.chromium.org/p/chromium/issues/detail?id=710824

Original issue's description:
> [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}
> Committed: https://chromium.googlesource.com/v8/v8/+/7829af3275ff4644a2d0a1270abe1a1e4415e9fb

TBR=bradnelson@chromium.org,ahaas@chromium.org,adamk@chromium.org,mtrofin@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:697028

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

src/runtime/runtime-test.cc

@@ -111,15 +111,6 @@ 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 {
@@ -454,24 +445,6 @@ 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,8 +616,6 @@ 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,18 +32,6 @@ 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) {
@@ -129,15 +117,16 @@ i::wasm::ModuleWireBytes GetFirstArgumentAsBytes(
   return i::wasm::ModuleWireBytes(start, start + length);
 }
 
-i::MaybeHandle<i::JSReceiver> GetValueAsImports(const Local<Value>& arg,
-                                                ErrorThrower* thrower) {
-  if (arg->IsUndefined()) return {};
+i::MaybeHandle<i::JSReceiver> GetSecondArgumentAsImports(
+    const v8::FunctionCallbackInfo<v8::Value>& args, ErrorThrower* thrower) {
+  if (args.Length() < 2) return {};
+  if (args[1]->IsUndefined()) return {};
 
-  if (!arg->IsObject()) {
+  if (!args[1]->IsObject()) {
     thrower->TypeError("Argument 1 must be an object");
     return {};
   }
-  Local<Object> obj = Local<Object>::Cast(arg);
+  Local<Object> obj = Local<Object>::Cast(args[1]);
   return i::Handle<i::JSReceiver>::cast(v8::Utils::OpenHandle(*obj));
 }
 
@@ -151,7 +140,8 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) {
   ErrorThrower thrower(i_isolate, "WebAssembly.compile()");
 
   Local<Context> context = isolate->GetCurrentContext();
-  ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(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());
 
@@ -263,70 +253,10 @@ 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());
-  Isolate* isolate = args.GetIsolate();
-  Local<Context> context = isolate->GetCurrentContext();
+  v8::Isolate* isolate = args.GetIsolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   if (i_isolate->wasm_instance_callback()(args)) return;
 
@@ -335,15 +265,14 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
   auto maybe_module = GetFirstArgumentAsModule(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];
+  auto maybe_imports = GetSecondArgumentAsImports(args, &thrower);
+  if (thrower.error()) return;
 
-  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);
+  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()));
 }
 
 // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance
@@ -361,9 +290,10 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
 
-  ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context));
-  Local<Promise> module_promise = resolver->GetPromise();
-  args.GetReturnValue().Set(module_promise);
+  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());
 
   if (args.Length() < 1) {
     thrower.TypeError(
@@ -375,8 +305,7 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
 
-  Local<Value> first_arg_value = args[0];
-  i::Handle<i::Object> first_arg = Utils::OpenHandle(*first_arg_value);
+  i::Handle<i::Object> first_arg = Utils::OpenHandle(*args[0]);
   if (!first_arg->IsJSObject()) {
     thrower.TypeError(
         "Argument 0 must be a buffer source or a WebAssembly.Module object");
@@ -386,27 +315,31 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
 
-  FunctionCallback instantiator = nullptr;
+  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());
   if (HasBrand(first_arg, i::Handle<i::Symbol>(i_context->wasm_module_sym()))) {
-    module_promise = resolver->GetPromise();
-    DO_BOOL(resolver->Resolve(context, first_arg_value));
-    instantiator = WebAssemblyInstantiateImpl;
+    // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance
+    auto module_object = GetFirstArgumentAsModule(args, &thrower);
+    i::wasm::AsyncInstantiate(i_isolate, promise,
+                              module_object.ToHandleChecked(), maybe_imports);
   } else {
-    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);
+    // 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);
+  }
 }
 
 bool GetIntegerProperty(v8::Isolate* isolate, ErrorThrower* thrower,

src/wasm/wasm-module.cc

@@ -2603,6 +2603,44 @@ 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,6 +472,10 @@ 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,33 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax
+// Flags: --expose-wasm
 
 load("test/mjsunit/wasm/wasm-constants.js");
 load("test/mjsunit/wasm/wasm-module-builder.js");
 
-const kReturnValue = 15;
-
-function getBuilder() {
+(function BasicTest() {
+  var kReturnValue = 15;
   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,6 +713,7 @@ 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)));
   });
 }
@@ -759,6 +760,7 @@ 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);