[wasm] support for recompilation if deserialization fails

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 16e3ae1e50969e8fb8c9109ed4bb4c8a7a5686d3..8de6ed568f05fa2a9a36cdd0f9bd8710f4d587b1 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
@@ -108,6 +108,22 @@ void SerializedScriptValueWriter::writeBooleanObject(bool value) {
   append(value ? TrueObjectTag : FalseObjectTag);
 }
 
+void SerializedScriptValueWriter::writeRawStringBytes(
+    v8::Local<v8::String>& string) {
+  int rawLength = string->Length();
+  string->WriteOneByte(byteAt(m_position), 0, rawLength,
+                       v8StringWriteOptions());
+  m_position += rawLength;
+}
+
+void SerializedScriptValueWriter::writeUtf8String(
+    v8::Local<v8::String>& string) {
+  int utf8Length = string->Utf8Length();
+  char* buffer = reinterpret_cast<char*>(byteAt(m_position));
+  string->WriteUtf8(buffer, utf8Length, 0, v8StringWriteOptions());
+  m_position += utf8Length;
+}
+
 void SerializedScriptValueWriter::writeOneByteString(
     v8::Local<v8::String>& string) {
   int stringLength = string->Length();
@@ -120,13 +136,10 @@ void SerializedScriptValueWriter::writeOneByteString(
 
   // ASCII fast path.
   if (stringLength == utf8Length) {
-    string->WriteOneByte(byteAt(m_position), 0, utf8Length,
-                         v8StringWriteOptions());
+    writeRawStringBytes(string);
   } else {
-    char* buffer = reinterpret_cast<char*>(byteAt(m_position));
-    string->WriteUtf8(buffer, utf8Length, 0, v8StringWriteOptions());
+    writeUtf8String(string);
   }
-  m_position += utf8Length;
 }
 
 void SerializedScriptValueWriter::writeUCharString(
@@ -1256,8 +1269,21 @@ ScriptValueSerializer::writeWasmCompiledModule(v8::Local<v8::Object> object,
   // TODO (mtrofin): explore mechanism avoiding data copying / buffer resizing.
   v8::Local<v8::WasmCompiledModule> wasmModule =
       object.As<v8::WasmCompiledModule>();
+  v8::Local<v8::String> uncompiledBytes = wasmModule->GetUncompiledBytes();

[titzer, 2016/10/12 15:09:53]

UncompiledBytes isn't a very good name. These are the original (encoded) bytes
of the module, correct? Then maybe "OriginalBytes" or "OriginalModuleBytes" is a
better name?

[Mircea Trofin, 2016/10/12 16:01:07]

I wanted to draw the compiled vs uncompiled distinction. "Original" may be
confusing because it may also mean "the object we serialized", for example. If
the application does some form of local patching, "Original" may mean "what was
downloaded" (vs what ended up being patched". I'd prefer keeping
"UncompiledBytes", I think it clarifies what his is, especially in the context
of "DeserializeOrCompile".

[titzer, 2016/10/12 16:15:30]

"Uncompiled" doesn't suggest to me that this is encoded module data that came
over the wire.

[Mircea Trofin, 2016/10/14 06:20:18]

Done.

+  DCHECK(uncompiledBytes->IsOneByte());
+
   v8::WasmCompiledModule::SerializedModule data = wasmModule->Serialize();
   m_writer.append(WasmModuleTag);
+  uint32_t uncompiledBytesLength =
+      static_cast<uint32_t>(uncompiledBytes->Length());
+  // We place a tag so we may evolve the format in which we store the
+  // uncompiled bytes. We plan to move them to a blob.
+  // We want to control how we write the string, though, so we explicitly
+  // call writeRawStringBytes.
+  m_writer.append(StringTag);

[jbroman, 2016/10/12 15:05:38]

At the moment, StringTag generally means a UTF-8 string. It might be clearer to
have a separate enum for WasmModuleStorageTag or something like that, to avoid
possible confusion.

[Mircea Trofin, 2016/10/12 16:01:07]

OK, and we're OK with later using the blob tag if that ends up being exactly how
we store, correct?

On the new tag name: the data there is raw bytes, it so happens we pull it from
a String object at serialization. We do that t avoid copying it on the v8's
Serialize API side. Then, at deserialization, we end up treating it like a byte*
anyway. How about "RawBytesTag"?

[Mircea Trofin, 2016/10/14 06:20:18]

Done.

+  m_writer.doWriteUint32(uncompiledBytesLength);
+  m_writer.ensureSpace(uncompiledBytesLength);
+  m_writer.writeRawStringBytes(uncompiledBytes);
   m_writer.doWriteUint32(static_cast<uint32_t>(data.second));
   m_writer.append(data.first.get(), static_cast<int>(data.second));
   return nullptr;
@@ -1953,24 +1979,58 @@ DOMArrayBuffer* SerializedScriptValueReader::doReadArrayBuffer() {
 bool SerializedScriptValueReader::readWasmCompiledModule(
     v8::Local<v8::Value>* value) {
   CHECK(RuntimeEnabledFeatures::webAssemblySerializationEnabled());
-  uint32_t size = 0;
-  if (!doReadUint32(&size))
+  // First, read the tag of the uncompiled bytes.
+  SerializationTag uncompiledBytesFormat = InvalidTag;
+  if (!readTag(&uncompiledBytesFormat))
     return false;
-  if (m_position + size > m_length)
+  DCHECK(uncompiledBytesFormat == StringTag);
+  // Just like when writing, we don't rely on the default string serialization
+  // mechanics for the uncompiled bytes. We don't even want a string, because
+  // that would lead to a memory copying API implementation on the V8 side.
+  uint32_t uncompiledBytesSize = 0;
+  uint32_t compiledBytesSize = 0;
+  if (!doReadUint32(&uncompiledBytesSize))
     return false;
-  const uint8_t* buf = m_buffer + m_position;
+  if (m_position + uncompiledBytesSize > m_length)
+    return false;
+  const uint8_t* uncompiledBytesStart = m_buffer + m_position;
+  m_position += uncompiledBytesSize;
+
+  if (!doReadUint32(&compiledBytesSize))
+    return false;
+  if (m_position + compiledBytesSize > m_length)
+    return false;
+  const uint8_t* compiledBytesStart = m_buffer + m_position;
+  m_position += compiledBytesSize;
+
+  v8::WasmCompiledModule::UncompiledBytes uncompiledBytes = {
+      std::unique_ptr<const uint8_t[]>(uncompiledBytesStart),

[jbroman, 2016/10/12 15:05:38]

Not new to this CL, but please don't put unowned data in an std::unique_ptr like
this.

If it doesn't take ownership of the data, it seems to me that this is an
indication that v8::WasmCompiledModule::UncompiledBytes should take a const
uint8_t* rather than a std::unique_ptr<const uint8_t[]>.

[Mircea Trofin, 2016/10/12 16:01:07]

I saw the todo here, I'll update on the v8 side and re-update this CL. Thanks
for reminding me!

[Mircea Trofin, 2016/10/14 06:20:17]

Done.

+      static_cast<size_t>(uncompiledBytesSize)};
+
   // TODO(mtrofin): simplify deserializer API. const uint8_t* + size_t should
   // be sufficient.
-  v8::WasmCompiledModule::SerializedModule data = {
-      std::unique_ptr<const uint8_t[]>(buf), static_cast<size_t>(size)};
-  v8::MaybeLocal<v8::WasmCompiledModule> retval =
-      v8::WasmCompiledModule::Deserialize(isolate(), data);
-  data.first.release();
-  m_position += size;
+  v8::WasmCompiledModule::SerializedModule compiledBytes = {
+      std::unique_ptr<const uint8_t[]>(compiledBytesStart),
+      static_cast<size_t>(compiledBytesSize)};
 
-  // TODO(mtrofin): right now, we'll return undefined if the deserialization
-  // fails, which is what may happen when v8's version changes. Update when
-  // spec settles. crbug.com/639090
+  v8::MaybeLocal<v8::WasmCompiledModule> retval =
+      v8::WasmCompiledModule::DeserializeOrCompile(isolate(), compiledBytes,
+                                                   uncompiledBytes);
+
+  compiledBytes.first.release();
+  uncompiledBytes.first.release();
+
+  // Even if deserialization fails, due to chrome upgrade, for instance,
+  // recompilation must succeed.
+  // Technically, there is a way to fail in that case, too: downgrade
+  // chrome after persisting a wasm module using new features. The wasm
+  // evolution story guarantees backwards-compatibility (Vx works in Vx+1),
+  // no guarantees are made for Vx+1 in a pre- x+1 environment. Applications
+  // are expected to feature-detect on the browser side and request from
+  // server the appropriate wasm binary version, or to patch the binary
+  // on the browser side.
+  // We fail because this is explicitly unsupported.
+  CHECK(!retval.IsEmpty());

[jochen (gone - plz use gerrit), 2016/10/12 09:34:59]

I don't think we should crash here. Downgrading is very common, e.g. ppl might
run stable and beta in parallel. If we just can't compile the blob, we should
throw an exception.

could you embed a version number in the serialized blob, so we don't even
attempt to deserialize old, incompatible blobs?

[Mircea Trofin, 2016/10/12 16:01:07]

Is it an exception, or is it just returning "false"? (I'm assuming the latter)
The serialized blob has a version number in the header. The deserialization
logic checks that first and does not go any further if the version is not a
match for the current version. This all happens on the v8 side.

[Mircea Trofin, 2016/10/14 06:20:17]

Done.

   return retval.ToLocal(value);
 }