[wasm] Support wasm module structured cloning.

third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
index cbfc21cf08d5c6261d2f582d973edadf8e236678..4a881ba0d02d51c132f72cf79d2f4e51575a96da 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
@@ -868,6 +868,9 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::doSerializeObject(v8::L
         return writeTransferredSharedArrayBuffer(object, index, next);
     }
 
+    if (object->IsWebAssemblyCompiledModule()) {
+        return writeWasmCompiledModule(object, next);
+    }

[jsbell, 2016/08/18 16:48:02]

Nit: no {} necessary for single line ifs

[Mircea Trofin, 2016/08/18 17:34:08]

Acknowledged.

[Mircea Trofin, 2016/08/18 23:02:59]

Done.

     // Transferable only objects
     if (V8MessagePort::hasInstance(object, isolate())) {
         uint32_t index;
@@ -1175,6 +1178,16 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::writeAndGreyArrayBuffer
     return nullptr;
 }
 
+ScriptValueSerializer::StateBase* ScriptValueSerializer::writeWasmCompiledModule(v8::Local<v8::Object> object, StateBase* next)
+{
+    v8::Local<v8::WasmCompiledModule> wasmModule = object.As<v8::WasmCompiledModule>();
+    v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();

[jsbell, 2016/08/18 16:48:02]

Is this a copy, or does SerializedModule point into the existing bytes? If it's
a copy, can we know the size before we call Serialize(), and change Serialize()
to take a writer?

(that could be follow-up work to avoid the copy if necessary and possible)

[Mircea Trofin, 2016/08/18 17:34:08]

SerializedModule points to existing bytes, at the moment.

I am overall concerned with the need to copy bytes. I would prefer what you
suggest - taking some form of writer, and, in addition, avoiding the need to
allocate contiguous memory for the data. Wasm compiled modules may be assumed to
be in the order of tens of megs, at least, so I'm concerned both with copying
and with the memory pressure the contiguous memory requirement creates. On that,
is segmented writing something currently supported?

I was planning to look into those details as part of next steps. My first goal
was to ensure the end to end story holds up. Any pushback to staging it this
way?

[jsbell, 2016/08/18 18:13:14]

Staging is great! (I'm a fan of leaving TODOs in the code in that case)

[Mircea Trofin, 2016/08/18 23:02:59]

Done.

+    m_writer.append(WasmModuleTag);
+    m_writer.doWriteUint32(static_cast<uint32_t>(data.second));
+    m_writer.append(data.first.get(), static_cast<int>(data.second));
+    return nullptr;
+}
+
 ScriptValueSerializer::StateBase* ScriptValueSerializer::writeAndGreyArrayBuffer(v8::Local<v8::Object> object, StateBase* next)
 {
     DOMArrayBuffer* arrayBuffer = V8ArrayBuffer::toImpl(object);
@@ -1475,6 +1488,12 @@ bool SerializedScriptValueReader::readWithTag(SerializationTag tag, v8::Local<v8
         deserializer.pushObjectReference(*value);
         break;
     }
+    case WasmModuleTag: {
+        if (!readWasmCompiledModule(value))
+            return false;
+        deserializer.pushObjectReference(*value);
+        break;
+    }
     case ArrayBufferTag: {
         if (!m_version)
             return false;
@@ -1827,6 +1846,25 @@ DOMArrayBuffer* SerializedScriptValueReader::doReadArrayBuffer()
     return DOMArrayBuffer::create(bufferStart, byteLength);
 }
 
+bool SerializedScriptValueReader::readWasmCompiledModule(v8::Local<v8::Value>* value)
+{
+    uint32_t size = 0;
+    if (!doReadUint32(&size))
+        return false;
+    if (m_position + size > m_length)
+        return false;
+    const uint8_t* buf = m_buffer + m_position;
+    v8::WasmCompiledModule::SerializedModule data = {
+        std::unique_ptr<const uint8_t[]>(buf),

[jsbell, 2016/08/18 16:48:02]

Uh... it seems like the SerializedModule constructor should not take a
unique_ptr if this is safe. If it's not claiming ownership of the data (i.e.
doing a std::move on the ptr) why is it taking a unique_ptr in the first place?

[Mircea Trofin, 2016/08/18 17:34:08]

SerializedModule is just a pair used by both serializer and deserializer. The
idea was to enforce the contract that 
1) the deserializer doesn't own the data, and shouldn't delete it. Passing const
uint8_t* would't be enough, one can still delete that. A const std::unique_ptr&,
however, makes that trickier.
2) the serializer relinquishes the control of the data to the caller (hence the
unique_ptr)

But maybe that's too overboard. We could just pass to the deserializer the const
uint8_t* and the length and document the ownership aspect. The serializer and
deserializer wouldn't have symmetric APIs, but that's not a strong argument,
imho, especially when I doubt any symmetry would be left once we have a
non-copying, segmented design.

...and maybe there's another option?

[jsbell, 2016/08/18 18:13:14]

I'd start with this, pending any better ideas.

[Mircea Trofin, 2016/08/18 23:02:59]

Added a TODO, and will follow with the v8 CL + chrome CL subsequently

+        static_cast<size_t>(size)
+    };
+    v8::MaybeLocal<v8::WasmCompiledModule> retval =
+        v8::WasmCompiledModule::Deserialize(isolate(), data);

[jbroman, 2016/08/18 16:59:09]

If I read this correctly, every time the V8 version changes every compiled
module stored in IndexedDB will become invalid, and any attempt to read a record
containing a module will fail (IIRC this appears as the object being
deserialized as null).

https://github.com/WebAssembly/design/blob/master/JS.md#structured-clone-of-a...
doesn't mention this behaviour.

[Mircea Trofin, 2016/08/18 17:34:08]

That's correct, the intent is to not support deserialization across v8 versions.
Returning undefined is one option, throwing is another one (there may be more).
The spec leaves this detail unspecified (I'll follow up on that), and we still
need to figure out, for IndexedDB, what would happen with the stored bits in
this case. So there are a few loose ends still left to be tied, but the goal of
this CL is to be a first step and make sure the basic end to end plumbing works
as expected.

Does this staging approach for enabling the feature make sense?

[jbroman, 2016/08/18 17:55:50]

Seems fine, as long as this is addressed before shipping.

[jsbell, 2016/08/18 18:13:14]

I think what we settled on (in email conversations) is that we'd yield a Module
that was "neutered" in the same way as a closed Blob - an object makes it out of
the database but produces errors if you try and use it. But that may not have
made it into the spec.
+1 to staging. (and sprinkles of TODOs that link to tracking crbugs would be
awesome!)

[Mircea Trofin, 2016/08/18 23:02:59]

Done.

+    data.first.release();
+    m_position += size;
+    return retval.ToLocal(value);
+}
+
 bool SerializedScriptValueReader::readArrayBuffer(v8::Local<v8::Value>* value)
 {
     DOMArrayBuffer* arrayBuffer = doReadArrayBuffer();