[wasm] response-based compile APIs

third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp

+// Copyright 2015 The Chromium Authors. All rights reserved.

[haraken, 2017/04/07 04:47:46]

2017

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "modules/wasm/WasmResponseExtensions.h"
+
+#include "bindings/core/v8/ExceptionState.h"
+#include "bindings/core/v8/ScriptState.h"
+#include "bindings/modules/v8/V8Response.h"
+#include "modules/fetch/BodyStreamBuffer.h"
+#include "wtf/RefPtr.h"

[yhirano, 2017/04/07 02:39:37]

+ modules/fetch/FetchDataLoader.h
+ bindings/core/v8/ScriptPomise.h
+ bindings/core/v8/ScriptPomiseResolver.h
+ platform/heap/Handle.h
 

[Mircea Trofin, 2017/04/07 06:11:16]

done.

+
+namespace blink {
+
+namespace {
+
+class FetchDataLoaderAsWasmModule final : public FetchDataLoader,
+                                          public BytesConsumer::Client {
+  USING_GARBAGE_COLLECTED_MIXIN(FetchDataLoaderAsWasmModule);
+
+ public:
+  FetchDataLoaderAsWasmModule(v8::Isolate* isolate,

[haraken, 2017/04/07 04:47:46]

Remove the isolate parameter. You can use scriptSTate->isolate().

[Mircea Trofin, 2017/04/07 06:11:16]

Done.

+                              ScriptPromiseResolver* resolver,
+                              ScriptState* scriptState)
+      : m_resolver(resolver), m_builder(isolate), m_scriptState(scriptState) {}
+
+  void start(BytesConsumer* consumer,
+             FetchDataLoader::Client* client) override {
+    DCHECK(!m_consumer);
+    DCHECK(!m_client);
+    m_client = client;
+    m_consumer = consumer;
+    m_consumer->setClient(this);
+    onStateChange();
+  }
+
+  void onStateChange() override {
+    while (true) {
+      // buffer is allocated by beginRead and de-allocated by endRead.

[haraken, 2017/04/07 04:47:46]

beginRead just sets buffer to some address in an already-allocated buffer, and
endRead does nothing on the buffer, right?

[Mircea Trofin, 2017/04/07 06:11:17]

Yes, looking at 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/fetch/...

and also from other implementations of FetchDataLoader, it seems that beginRead
does what you mentioned. endRead invalidates the buffer. We can't take ownership
of the buffer, hence the wasm side copies.

+      const char* buffer = nullptr;
+      size_t available = 0;
+      BytesConsumer::Result result = m_consumer->beginRead(&buffer, &available);
+
+      if (result == BytesConsumer::Result::ShouldWait)
+        return;
+      if (result == BytesConsumer::Result::Ok) {
+        if (available > 0) {
+          DCHECK_NE(buffer, nullptr);
+          m_builder.OnBytesReceived(reinterpret_cast<const uint8_t*>(buffer),
+                                    available);
+        }
+        result = m_consumer->endRead(available);
+      }
+      switch (result) {
+        case BytesConsumer::Result::ShouldWait:
+          NOTREACHED();
+          return;
+        case BytesConsumer::Result::Ok: {
+          break;
+        }
+        case BytesConsumer::Result::Done: {
+          v8::Isolate* isolate = m_resolver->getScriptState()->isolate();
+          ScriptState::Scope scope(m_scriptState.get());
+          v8::TryCatch trycatch(isolate);
+          v8::Local<v8::WasmCompiledModule> module;
+          if (m_builder.Finish().ToLocal(&module)) {
+            DCHECK(!trycatch.HasCaught());
+            ScriptValue scriptValue(m_scriptState.get(), module);
+            m_resolver->resolve(scriptValue);
+          } else {
+            DCHECK(trycatch.HasCaught());
+            m_resolver->reject(trycatch.Exception());
+            trycatch.Reset();
+          }
+          m_client->didFetchDataLoadedCustomFormat();
+          return;
+        }
+        case BytesConsumer::Result::Error: {
+          // TODO(mtrofin): do we need an abort on the wasm side?
+          // Something like "m_outStream->abort()" maybe?
+          return ThrowTypeError();
+        }
+      }
+    }
+  }
+
+  void cancel() override {
+    m_consumer->cancel();
+    return ThrowTypeError();
+  }
+
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_consumer);
+    visitor->trace(m_resolver);
+    visitor->trace(m_client);
+    FetchDataLoader::trace(visitor);
+    BytesConsumer::Client::trace(visitor);
+  }
+
+ private:
+  // TODO(mtrofin): replace with spec-ed error types, once spec clarifies
+  // what they are.
+  void ThrowTypeError() {

[haraken, 2017/04/07 04:47:46]

throwTypeError

I'd prefer "rejectPromise" or something though.

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+    m_resolver->reject(V8ThrowException::createTypeError(
+        m_scriptState->isolate(), "Could not download wasm module"));
+  }
+  Member<BytesConsumer> m_consumer;
+  Member<ScriptPromiseResolver> m_resolver;
+  Member<FetchDataLoader::Client> m_client;
+  v8::WasmModuleObjectBuilder m_builder;
+  const RefPtr<ScriptState> m_scriptState;
+};
+
+// TODO(mtrofin): WasmDataLoaderClient is necessary so we may provide an
+// argument to BodyStreamBuffer::startLoading, however, it fulfills
+// a very small role. Consider refactoring to avoid it.
+class WasmDataLoaderClient final
+    : public GarbageCollectedFinalized<WasmDataLoaderClient>,
+      public FetchDataLoader::Client {
+  WTF_MAKE_NONCOPYABLE(WasmDataLoaderClient);
+  USING_GARBAGE_COLLECTED_MIXIN(WasmDataLoaderClient);
+
+ public:
+  explicit WasmDataLoaderClient(ScriptPromiseResolver* resolver)
+      : m_resolver(resolver) {}
+
+  void didFetchDataLoadedCustomFormat() override {}
+  void didFetchDataLoadFailed() override { NOTREACHED(); }
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_resolver);
+    FetchDataLoader::Client::trace(visitor);
+  }
+
+ private:
+  Member<ScriptPromiseResolver> m_resolver;

[yhirano, 2017/04/07 02:39:37]

You don't need this member.

[Mircea Trofin, 2017/04/07 06:11:16]

Done.

+};
+
+// This callback may be entered as a promise is resolved, or directly
+// from the overload callback.
+// See https://crbug.com/708238 for tracking avoiding the hand-generated code.
+void compileFromResponseCallback(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  ExceptionState exceptionState(args.GetIsolate(),
+                                ExceptionState::ExecutionContext, "WebAssembly",
+                                "compile");
+  ExceptionToRejectPromiseScope rejectPromiseScope(args, exceptionState);
+
+  ScriptState* scriptState = ScriptState::forReceiverObject(args);
+  if (!scriptState->getExecutionContext()) {
+    args.GetReturnValue().Set(ScriptPromise().v8Value());

[yhirano, 2017/04/07 02:39:36]

ScriptPromise().v8Value() is mere an empty v8 value. Do we need this line?

[haraken, 2017/04/07 04:47:46]

Nit: v8SetReturnValue()

+    return;
+  }
+
+  if (args.Length() < 1 || !args[0]->IsObject() ||
+      !V8Response::hasInstance(args[0], args.GetIsolate())) {
+    args.GetReturnValue().Set(

[haraken, 2017/04/07 04:47:47]

Nit: v8SetReturnValue()

[Mircea Trofin, 2017/04/07 06:11:16]

I like that! Done.

+        ScriptPromise::reject(
+            scriptState, V8ThrowException::createTypeError(
+                             scriptState->isolate(),
+                             "Promise argument must produce a Response object"))

[yhirano, 2017/04/07 02:39:36]

Is "This function must be called with Promise<Response>" clearer?

[Mircea Trofin, 2017/04/07 06:11:16]

Done.

+            .v8Value());
+    return;
+  }
+
+  Response* response = V8Response::toImpl(v8::Local<v8::Object>::Cast(args[0]));
+  ScriptPromise promise;
+  if (response->isBodyLocked() || response->bodyUsed()) {
+    promise = ScriptPromise::reject(
+        scriptState,
+        V8ThrowException::createTypeError(
+            scriptState->isolate(),
+            "Cannot compile WebAssembly.Module from an already read Response"));
+  } else {
+    ScriptPromiseResolver* resolver =
+        ScriptPromiseResolver::create(scriptState);
+    if (response->bodyBuffer()) {
+      promise = resolver->promise();
+      response->bodyBuffer()->startLoading(
+          new FetchDataLoaderAsWasmModule(args.GetIsolate(), resolver,
+                                          scriptState),
+          new WasmDataLoaderClient(resolver));
+    } else {
+      promise = ScriptPromise::reject(
+          scriptState, V8ThrowException::createTypeError(
+                           scriptState->isolate(),
+                           "Promise argument must produce a Response object"));

[yhirano, 2017/04/07 02:39:37]

I think this message is confusing (from a different reason than the above). This
is the case where the response has a null body.

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+    }
+  }
+  args.GetReturnValue().Set(promise.v8Value());

[haraken, 2017/04/07 04:47:46]

Nit: v8SetReturnValue()

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+}
+
+ScriptPromise wasmCompileFromPromise(v8::Isolate* isolate,
+                                     ScriptPromise promise) {
+  v8::Local<v8::Function> compileCallback =
+      v8::Function::New(isolate, compileFromResponseCallback);
+  return promise.then(compileCallback);
+}
+
+// See https://crbug.com/708238 for tracking avoiding the hand-generated code.

[haraken, 2017/04/07 04:47:46]

Add a comment about what spec this method is implementing.

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+bool wasmCompileOverload(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  if (args.Length() < 1 || !args[0]->IsObject())
+    return false;
+
+  v8::Isolate* isolate = args.GetIsolate();
+  ScriptState* scriptState = ScriptState::forReceiverObject(args);
+
+  if (args[0]->IsPromise()) {
+    ScriptPromise scriptPromise = ScriptPromise(scriptState, args[0]);
+    args.GetReturnValue().Set(

[haraken, 2017/04/07 04:47:47]

Nit: v8SetReturnValue()

[Mircea Trofin, 2017/04/07 06:11:16]

Done.

+        wasmCompileFromPromise(isolate, scriptPromise).v8Value());
+    return true;
+  }
+  if (V8Response::hasInstance(args[0], args.GetIsolate())) {
+    ScriptPromiseResolver* scriptPromiseResolver =
+        ScriptPromiseResolver::create(scriptState);
+    ScriptValue wrappedResponse = ScriptValue::from(scriptState, args[0]);

[haraken, 2017/04/07 04:47:46]

ScriptValue wrappedResponse(scriptState, args[0]) will work?

[Mircea Trofin, 2017/04/07 06:11:17]

Ya, but I moved that line in the resolve() call below, I think it reads 
closer to the "Promise.resolve(parameter)" js equivalent. So I'm still using the
::from there, to avoid a temp variable.

+    args.GetReturnValue().Set(

[haraken, 2017/04/07 04:47:46]

Nit: v8SetReturnValue()

[Mircea Trofin, 2017/04/07 06:11:17]

Done.

+        (wasmCompileFromPromise(isolate, scriptPromiseResolver->promise()))
+            .v8Value());
+    scriptPromiseResolver->resolve(wrappedResponse);
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
+void WasmResponseExtensions::initialize(v8::Isolate* isolate) {
+  isolate->SetWasmCompileCallback(wasmCompileOverload);
+}
+
+}  // namespace blink