[wasm] response-based compile APIs

third_party/WebKit/Source/modules/fetch/Response.cpp

Index: third_party/WebKit/Source/modules/fetch/Response.cpp
diff --git a/third_party/WebKit/Source/modules/fetch/Response.cpp b/third_party/WebKit/Source/modules/fetch/Response.cpp
index d5319f453f05869de885768408521d50bf6970f7..efbe521c65ae6a839c5bbce472b8ab91a2f4b955 100644
--- a/third_party/WebKit/Source/modules/fetch/Response.cpp
+++ b/third_party/WebKit/Source/modules/fetch/Response.cpp
@@ -16,6 +16,7 @@
 #include "bindings/core/v8/V8HiddenValue.h"
 #include "bindings/core/v8/V8URLSearchParams.h"
 #include "bindings/modules/v8/ByteStringSequenceSequenceOrDictionaryOrHeaders.h"
+#include "bindings/modules/v8/V8Response.h"
 #include "core/dom/DOMArrayBuffer.h"
 #include "core/dom/DOMArrayBufferView.h"
 #include "core/dom/URLSearchParams.h"
@@ -120,8 +121,220 @@ bool isValidReasonPhrase(const String& statusText) {
   return true;
 }
 
+class FetchDataLoaderAsWasm final : public FetchDataLoader,
+                                    public BytesConsumer::Client {
+  USING_GARBAGE_COLLECTED_MIXIN(FetchDataLoaderAsWasm);
+
+ public:
+  explicit FetchDataLoaderAsWasm(v8::Isolate* isolate,

[haraken, 2017/04/04 02:39:38]

Drop explicit.

[Mircea Trofin, 2017/04/04 04:08:12]

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) {
+      const char* buffer;
+      size_t available;
+      BytesConsumer::Result result = m_consumer->beginRead(&buffer, &available);

[haraken, 2017/04/04 02:39:38]

Who allocates the buffer memory and who deallocates the memory?

[Mircea Trofin, 2017/04/04 04:08:12]

the consumer allocates, and deallocates at endRead.

I got inspiration from what we do elsewhere, e.g. FetchDataLoaderAsArrayBuffer.
I'm assuming that this pattern of allocation/deallocation by
m_consumer->beginRead/endRead is part of the contract with that object?

+
+      switch (result) {
+        case BytesConsumer::Result::ShouldWait:
+          return;
+        case BytesConsumer::Result::Ok: {
+          SendBytes(buffer, available);
+          result = m_consumer->endRead(available);
+          break;
+        }
+        case BytesConsumer::Result::Done: {
+          if (available > 0)
+            SendBytes(buffer, available);
+          v8::Isolate* isolate = m_resolver->getScriptState()->isolate();
+          ScriptState::Scope scope(m_scriptState.get());
+          v8::TryCatch trycatch(isolate);
+          v8::MaybeLocal<v8::WasmCompiledModule> maybeModule =
+              m_builder.Finish();
+          v8::Local<v8::WasmCompiledModule> module;
+          if (maybeModule.ToLocal(&module)) {

[haraken, 2017/04/04 02:39:38]

Nit: Use m_builder.ToLocal(&module). We want to avoid using MaybeLocal handles
explicitly.

[Mircea Trofin, 2017/04/04 04:08:12]

Done.

I'm curious though, what is the motivation against using MaybeLocals explicitly
(style, something else)? Thanks!

+            DCHECK(!trycatch.HasCaught());
+            ScriptValue sv(m_scriptState.get(), module);

[haraken, 2017/04/04 02:39:38]

sv => scriptValue

Blink prefers a fully qualified name.

[Mircea Trofin, 2017/04/04 04:08:12]

Done.

+            m_resolver->resolve(sv);
+          } else {
+            DCHECK(trycatch.HasCaught());
+            m_resolver->reject(trycatch.Exception());
+            trycatch.Reset();

[haraken, 2017/04/04 02:39:38]

Why do you need to Reset it?

[Mircea Trofin, 2017/04/04 04:08:12]

We don't want the exception to survive past the catch, since we're rejecting
with it.

+          }
+          m_client->didFetchDataLoadedStream();
+          return;
+        }
+        case BytesConsumer::Result::Error: {
+          // TODO(mtrofin): do we need an abort on the wasm side?
+          // m_outStream->abort();
+          m_client->didFetchDataLoadFailed();
+          return;
+        }
+      }
+    }
+  }
+
+  void cancel() override { m_consumer->cancel(); }
+
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_consumer);
+    visitor->trace(m_resolver);
+    visitor->trace(m_client);
+    FetchDataLoader::trace(visitor);
+    BytesConsumer::Client::trace(visitor);
+  }
+
+ private:
+  void SendBytes(const char* bytes, size_t size) {

[haraken, 2017/04/04 02:39:38]

sendBytes

+    char* bufferCopy = new char[size];

[haraken, 2017/04/04 02:39:38]

Use fastMalloc().

Who deletes the bufferCopy?

[Mircea Trofin, 2017/04/04 04:08:12]

Changed to using std::unique_ptr from the get-go, as per +yhirano's suggestion.

Re. destruction - the destructor of the unique_ptr it's getting handed to.
That'd be the case both before and now.

Does the fastMalloc comment still apply, now that we start from a
std::unique_ptr?

+    memcpy(bufferCopy, bytes, size);
+    // TODO(mtrofin): we want to extend OnBytesReceived to test
+    // for decoding errors or for compilation errors that
+    // happened meanwhile.
+    m_builder.OnBytesReceived(std::unique_ptr<const uint8_t[]>(
+                                  reinterpret_cast<const uint8_t*>(bufferCopy)),
+                              size);
+  }
+
+  Member<BytesConsumer> m_consumer;
+  Member<ScriptPromiseResolver> m_resolver;
+  Member<FetchDataLoader::Client> m_client;
+  v8::WasmModuleObjectBuilder m_builder;
+  const RefPtr<ScriptState> m_scriptState;
+};
+
+// TODO(mtrofin): WasmConsumer is necessary so we may provide an
+// argument to BodyStreamBuffer::startLoading, however, it fulfills
+// a very small role. Consider refactoring to avoid it.
+class WasmConsumer final : public GarbageCollectedFinalized<WasmConsumer>,

[haraken, 2017/04/04 02:39:38]

"Finalized" wouldn't be needed.

[Mircea Trofin, 2017/04/04 04:08:12]

You mean just "GarbageCollected"? Removing the "Finalized" triggers compiler
error because FetchDataLoader::Client is GarbageCollectedMixin (if I understand
this correctly)

+                           public FetchDataLoader::Client {
+  WTF_MAKE_NONCOPYABLE(WasmConsumer);
+  USING_GARBAGE_COLLECTED_MIXIN(WasmConsumer);
+
+ public:
+  explicit WasmConsumer(ScriptPromiseResolver* resolver)
+      : m_resolver(resolver) {}
+
+  void didFetchDataLoadedStream() override {}
+  void didFetchDataLoadFailed() override {
+    ScriptState::Scope scope(m_resolver->getScriptState());
+    m_resolver->reject(V8ThrowException::createTypeError(
+        m_resolver->getScriptState()->isolate(), "Failed to fetch"));
+  }
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_resolver);
+    FetchDataLoader::Client::trace(visitor);
+  }
+
+ private:
+  Member<ScriptPromiseResolver> m_resolver;
+};
+
+// This callback may be entered as a promise is resolved, or directly
+// from the overload callback.
+void CompileFromResponseCallback(

[haraken, 2017/04/04 02:39:38]

I'm not really happy about the hand-written callbacks and the wrapper type info
overriding.

Would you help me understand why you need the hack?

[Mircea Trofin, 2017/04/04 04:08:12]

Added you to a thread. 

The very succinct explanation is that we need blink-side overloads for wasm APIs
(defined in v8), and currently there's no alternative, from what I understand.

+    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());
+    return;
+  }
+
+  if (args.Length() < 1 || !args[0]->IsObject() ||
+      !V8Response::hasInstance(args[0], args.GetIsolate())) {
+    args.GetReturnValue().Set(
+        ScriptPromise::reject(
+            scriptState, V8ThrowException::createTypeError(
+                             scriptState->isolate(),
+                             "Promise argument must produce a Response object"))
+            .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 FetchDataLoaderAsWasm(args.GetIsolate(), resolver, scriptState),
+          new WasmConsumer(resolver));
+    } else {
+      promise = ScriptPromise::reject(
+          scriptState, V8ThrowException::createTypeError(
+                           scriptState->isolate(),
+                           "Promise argument must produce a Response object"));
+    }
+  }
+  args.GetReturnValue().Set(promise.v8Value());
+}
+
+bool WasmCompileOverload(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  if (args.Length() < 1 || !args[0]->IsObject())
+    return false;
+
+  if (args[0]->IsPromise()) {
+    ScriptState* scriptState = ScriptState::forReceiverObject(args);
+    ScriptPromise sp = ScriptPromise(scriptState, args[0]);
+    ScriptPromise thenCompile = sp.then(
+        v8::Function::New(args.GetIsolate(), CompileFromResponseCallback));
+    args.GetReturnValue().Set(thenCompile.v8Value());
+    return true;
+  }
+  if (V8Response::hasInstance(args[0], args.GetIsolate())) {
+    CompileFromResponseCallback(args);
+    return true;
+  }
+  return false;
+}
+
 }  // namespace
 
+v8::Local<v8::FunctionTemplate> Response::installerOverride(
+    v8::Isolate* isolate,
+    const DOMWrapperWorld& world) {
+  isolate->SetWasmCompileCallback(WasmCompileOverload);
+  return Response::s_wrapperTypeInfo.domTemplateFunction(isolate, world);
+}
+
+const WrapperTypeInfo Response::s_wrapperTypeInfoOverride = {
+    gin::kEmbedderBlink,
+    Response::installerOverride,
+    V8Response::trace,
+    V8Response::traceWrappers,
+    nullptr,
+    "Response",
+    0,
+    WrapperTypeInfo::WrapperTypeObjectPrototype,
+    WrapperTypeInfo::ObjectClassId,
+    WrapperTypeInfo::InheritFromActiveScriptWrappable,
+    WrapperTypeInfo::Dependent};
+
 Response* Response::create(ScriptState* scriptState,
                            ExceptionState& exceptionState) {
   return create(scriptState, nullptr, String(), ResponseInit(), exceptionState);