IndexedDB: Refactor value wrapping code.

This is extracted from the WASM + IndexedDB code into a separate CL, for
better reviewability. Serializing WASM modules will add a format to the
value-wrapping logic, pushing the total complexity to the point where
the benefits of having clearly separated lifecycle steps for
IDBValueWrapper exceed the benefits of having a terse API.

Therefore, this CL introduces a clear separation between the stages of
an IDBValueWrapper's lifecycle, which are:
1) Cloning (a structured clone may be needed to compute index values)
2) Wrapping
3) Extracting information for the backend -- the data bytes and the Blob list

The CL also introduces logic for including byte vectors in wrappers,
which will be needed for the V8 snapshot version -- details in
https://docs.google.com/document/d/1IgUeRLza0WC3uAzXxla5rH1bs5jUHKbLjsfU--zdYag

Last, this CL makes it possible to call
IDBValueWrapper::WrapIfBiggerThan() multiple times, which will come in
handy for the new wrapping format -- details in
https://crrev.com/c/709596/17/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp


Bug: 719007
Change-Id: Ie74ddfd4ec849d4777ab9a3af5dc254fc5b14837
Reviewed-on: https://chromium-review.googlesource.com/804969
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522369}

third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp

@@ -529,13 +529,13 @@ IDBRequest* IDBObjectStore::put(ScriptState* script_state,
   IDBRequest* request = IDBRequest::Create(
       script_state, source, transaction_.Get(), std::move(metrics));
 
+  value_wrapper.DoneCloning();
   value_wrapper.WrapIfBiggerThan(IDBValueWrapper::kWrapThreshold);
-  value_wrapper.ExtractBlobDataHandles(request->transit_blob_handles());
 
+  request->transit_blob_handles() = value_wrapper.TakeBlobDataHandles();
   BackendDB()->Put(
-      transaction_->Id(), Id(), WebData(value_wrapper.ExtractWireBytes()),
-      value_wrapper.WrappedBlobInfo(), key,
-      static_cast<WebIDBPutMode>(put_mode),
+      transaction_->Id(), Id(), WebData(value_wrapper.TakeWireBytes()),
+      value_wrapper.TakeBlobInfo(), key, static_cast<WebIDBPutMode>(put_mode),
       request->CreateWebCallbacks().release(), index_ids, index_keys);
 
   return request;

third_party/WebKit/Source/modules/indexeddb/IDBRequest.h

@@ -296,8 +296,8 @@ class MODULES_EXPORT IDBRequest : public EventTargetWithInlineData,
   // (which hangs onto the BlobDataHandle) may get garbage-collected. IDBRequest
   // needs to hang onto the BlobDataHandle as well, to avoid having the
   // browser-side Blob get destroyed before the IndexedDB request is processed.
-  inline Vector<scoped_refptr<BlobDataHandle>>* transit_blob_handles() {
-    return &transit_blob_handles_;
+  inline Vector<scoped_refptr<BlobDataHandle>>& transit_blob_handles() {
+    return transit_blob_handles_;
   }
 
 #if DCHECK_IS_ON()

third_party/WebKit/Source/modules/indexeddb/IDBTestHelper.cpp

@@ -28,19 +28,20 @@ scoped_refptr<IDBValue> CreateIDBValueForTesting(v8::Isolate* isolate,
   IDBValueWrapper wrapper(isolate, v8_array,
                           SerializedScriptValue::SerializeOptions::kSerialize,
                           non_throwable_exception_state);
+  wrapper.DoneCloning();
   wrapper.WrapIfBiggerThan(create_wrapped_value ? 0 : 1024 * element_count);
 
-  std::unique_ptr<Vector<scoped_refptr<BlobDataHandle>>> blob_data_handles =
-      std::make_unique<Vector<scoped_refptr<BlobDataHandle>>>();
-  wrapper.ExtractBlobDataHandles(blob_data_handles.get());
-  Vector<WebBlobInfo>& blob_infos = wrapper.WrappedBlobInfo();
-  scoped_refptr<SharedBuffer> wrapped_marker_buffer =
-      wrapper.ExtractWireBytes();
+  Vector<scoped_refptr<BlobDataHandle>> blob_data_handles =
+      wrapper.TakeBlobDataHandles();
+  Vector<WebBlobInfo> blob_infos = wrapper.TakeBlobInfo();
+  scoped_refptr<SharedBuffer> wrapped_marker_buffer = wrapper.TakeWireBytes();
   IDBKey* key = IDBKey::CreateNumber(42.0);
   IDBKeyPath key_path(String("primaryKey"));
 
   scoped_refptr<IDBValue> idb_value = IDBValue::Create(
-      std::move(wrapped_marker_buffer), std::move(blob_data_handles),
+      std::move(wrapped_marker_buffer),
+      std::make_unique<Vector<scoped_refptr<BlobDataHandle>>>(
+          blob_data_handles),
       std::make_unique<Vector<WebBlobInfo>>(blob_infos), key, key_path);
 
   DCHECK_EQ(create_wrapped_value,

third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp

@@ -13,7 +13,6 @@
 #include "modules/indexeddb/IDBRequest.h"
 #include "modules/indexeddb/IDBValue.h"
 #include "platform/blob/BlobData.h"
-#include "platform/wtf/text/StringView.h"
 #include "platform/wtf/text/WTFString.h"
 
 namespace blink {
@@ -42,16 +41,15 @@ namespace {
 // The SSV format version whose encoding hole is (ab)used for wrapping.
 const static uint8_t kRequiresProcessingSSVPseudoVersion = 17;
 
-// Identifies IndexedDB values that were wrapped in Blobs. The wrapper has the
-// following format:
+// SSV processing command replacing the SSV data bytes with a Blob's contents.
 //
 // 1) 0xFF - kVersionTag
 // 2) 0x11 - kRequiresProcessingSSVPseudoVersion
-// 3) 0x01 - kBlobWrappedValue
+// 3) 0x01 - kReplaceWithBlob
 // 4) varint - Blob size
 // 5) varint - the offset of the SSV-wrapping Blob in the IDBValue list of Blobs
 //             (should always be the last Blob)
-const static uint8_t kBlobWrappedValue = 1;
+const static uint8_t kReplaceWithBlob = 1;
 
 }  // namespace
 
@@ -68,8 +66,9 @@ IDBValueWrapper::IDBValueWrapper(
 
   serialized_value_ = SerializedScriptValue::Serialize(isolate, value, options,
                                                        exception_state);
-  if (serialized_value_)
+  if (serialized_value_) {
     original_data_length_ = serialized_value_->DataLengthInBytes();
+  }
 #if DCHECK_IS_ON()
   if (exception_state.HadException())
     had_exception_ = true;
@@ -82,9 +81,9 @@ IDBValueWrapper::~IDBValueWrapper() {}
 
 void IDBValueWrapper::Clone(ScriptState* script_state, ScriptValue* clone) {
 #if DCHECK_IS_ON()
-  DCHECK(!had_exception_) << __FUNCTION__
+  DCHECK(!had_exception_) << __func__
                           << " called on wrapper with serialization exception";
-  DCHECK(!wrap_called_) << "Clone() called after WrapIfBiggerThan()";
+  DCHECK(!done_cloning_) << __func__ << " called after DoneCloning()";
 #endif  // DCHECK_IS_ON()
 
   bool read_wasm_from_stream = true;
@@ -95,7 +94,8 @@ void IDBValueWrapper::Clone(ScriptState* script_state, ScriptValue* clone) {
                                   &blob_info_, read_wasm_from_stream);
 }
 
-void IDBValueWrapper::WriteVarint(unsigned value, Vector<char>& output) {
+// static
+void IDBValueWrapper::WriteVarInt(unsigned value, Vector<char>& output) {
   // Writes an unsigned integer as a base-128 varint.
   // The number is written, 7 bits at a time, from the least significant to
   // the most significant 7 bits. Each byte, except the last, has the MSB set.
@@ -107,22 +107,39 @@ void IDBValueWrapper::WriteVarint(unsigned value, Vector<char>& output) {
   output.back() &= 0x7F;
 }
 
-bool IDBValueWrapper::WrapIfBiggerThan(unsigned max_bytes) {
+// static
+void IDBValueWrapper::WriteBytes(const Vector<uint8_t>& bytes,
+                                 Vector<char>& output) {
+  IDBValueWrapper::WriteVarInt(bytes.size(), output);
+  output.Append(bytes.data(), bytes.size());
+}
+
+void IDBValueWrapper::DoneCloning() {
 #if DCHECK_IS_ON()
-  DCHECK(!had_exception_) << __FUNCTION__
+  DCHECK(!had_exception_) << __func__
                           << " called on wrapper with serialization exception";
-  DCHECK(!wrap_called_) << __FUNCTION__ << " called twice on the same wrapper";
-  wrap_called_ = true;
+  DCHECK(!done_cloning_) << __func__ << " called twice";
+  done_cloning_ = true;
 #endif  // DCHECK_IS_ON()
 
-  StringView wire_data = serialized_value_->GetWireData();
-  DCHECK(wire_data.Is8Bit());
-  unsigned wire_data_size = wire_data.length();
-  if (wire_data_size <= max_bytes) {
-    wire_bytes_.ReserveInitialCapacity(wire_data_size);
-    wire_bytes_.Append(wire_data.Characters8(), wire_data_size);
+  wire_data_ = serialized_value_->GetWireData();
+  DCHECK(wire_data_.Is8Bit());
+  for (const auto& kvp : serialized_value_->BlobDataHandles())
+    blob_handles_.push_back(std::move(kvp.value));
+}
+
+bool IDBValueWrapper::WrapIfBiggerThan(unsigned max_bytes) {
+#if DCHECK_IS_ON()
+  DCHECK(done_cloning_) << __func__ << " called before DoneCloning()";
+  DCHECK(owns_blob_handles_)
+      << __func__ << " called after TakeBlobDataHandles()";
+  DCHECK(owns_blob_info_) << __func__ << " called after TakeBlobInfo()";
+  DCHECK(owns_wire_bytes_) << __func__ << " called after TakeWireBytes()";
+#endif  // DCHECK_IS_ON()
+
+  unsigned wire_data_size = wire_data_.length();
+  if (wire_data_size <= max_bytes)
     return false;
-  }
 
   // TODO(pwnall): The MIME type should probably be an atomic string.
   String mime_type(kWrapMimeType);
@@ -130,37 +147,48 @@ bool IDBValueWrapper::WrapIfBiggerThan(unsigned max_bytes) {
   //                         Blob::Create to avoid a buffer copy.
   std::unique_ptr<BlobData> wrapper_blob_data = BlobData::Create();
   wrapper_blob_data->SetContentType(String(kWrapMimeType));
-  wrapper_blob_data->AppendBytes(wire_data.Characters8(), wire_data_size);
-  wrapper_handle_ =
+  wrapper_blob_data->AppendBytes(wire_data_.Characters8(), wire_data_size);
+  scoped_refptr<BlobDataHandle> wrapper_handle =
       BlobDataHandle::Create(std::move(wrapper_blob_data), wire_data_size);
-  blob_info_.emplace_back(wrapper_handle_->Uuid(), wrapper_handle_->GetType(),
+  blob_info_.emplace_back(wrapper_handle->Uuid(), wrapper_handle->GetType(),
                           wire_data_size);
-
-  wire_bytes_.clear();
-  wire_bytes_.push_back(kVersionTag);
-  wire_bytes_.push_back(kRequiresProcessingSSVPseudoVersion);
-  wire_bytes_.push_back(kBlobWrappedValue);
-  IDBValueWrapper::WriteVarint(wrapper_handle_->size(), wire_bytes_);
-  IDBValueWrapper::WriteVarint(serialized_value_->BlobDataHandles().size(),
-                               wire_bytes_);
+  blob_handles_.push_back(std::move(wrapper_handle));
+
+  wire_data_buffer_.clear();
+  wire_data_buffer_.push_back(kVersionTag);
+  wire_data_buffer_.push_back(kRequiresProcessingSSVPseudoVersion);
+  wire_data_buffer_.push_back(kReplaceWithBlob);
+  IDBValueWrapper::WriteVarInt(wire_data_size, wire_data_buffer_);
+  IDBValueWrapper::WriteVarInt(serialized_value_->BlobDataHandles().size(),
+                               wire_data_buffer_);
+
+  wire_data_ = StringView(wire_data_buffer_.data(), wire_data_buffer_.size());
+  DCHECK(!wire_data_buffer_.IsEmpty());
+  DCHECK(wire_data_.Is8Bit());
   return true;
 }
 
-void IDBValueWrapper::ExtractBlobDataHandles(
-    Vector<scoped_refptr<BlobDataHandle>>* blob_data_handles) {
-  for (const auto& kvp : serialized_value_->BlobDataHandles())
-    blob_data_handles->push_back(kvp.value);
-  if (wrapper_handle_)
-    blob_data_handles->push_back(std::move(wrapper_handle_));
-}
-
-scoped_refptr<SharedBuffer> IDBValueWrapper::ExtractWireBytes() {
+scoped_refptr<SharedBuffer> IDBValueWrapper::TakeWireBytes() {
 #if DCHECK_IS_ON()
-  DCHECK(!had_exception_) << __FUNCTION__
-                          << " called on wrapper with serialization exception";
+  DCHECK(done_cloning_) << __func__ << " called before DoneCloning()";
+  DCHECK(owns_wire_bytes_) << __func__ << " called twice";
+  owns_wire_bytes_ = false;
 #endif  // DCHECK_IS_ON()
 
-  return SharedBuffer::AdoptVector(wire_bytes_);
+  if (wire_data_buffer_.IsEmpty()) {
+    // The wire bytes are coming directly from the SSV's GetWireData() call.
+    DCHECK_EQ(wire_data_.Characters8(),
+              serialized_value_->GetWireData().Characters8());
+    DCHECK_EQ(wire_data_.length(), serialized_value_->GetWireData().length());
+    return SharedBuffer::Create(wire_data_.Characters8(),
+                                static_cast<size_t>(wire_data_.length()));
+  }
+
+  // The wire bytes are coming from wire_data_buffer_, so we can avoid a copy.
+  DCHECK_EQ(wire_data_buffer_.data(),
+            reinterpret_cast<const char*>(wire_data_.Characters8()));
+  DCHECK_EQ(wire_data_buffer_.size(), wire_data_.length());
+  return SharedBuffer::AdoptVector(wire_data_buffer_);
 }
 
 IDBValueUnwrapper::IDBValueUnwrapper() {
@@ -176,7 +204,7 @@ bool IDBValueUnwrapper::IsWrapped(IDBValue* value) {
 
   return header[0] == kVersionTag &&
          header[1] == kRequiresProcessingSSVPseudoVersion &&
-         header[2] == kBlobWrappedValue;
+         header[2] == kReplaceWithBlob;
 }
 
 bool IDBValueUnwrapper::IsWrapped(
@@ -225,11 +253,11 @@ bool IDBValueUnwrapper::Parse(IDBValue* value) {
   end_ = data + value->data_->size();
   current_ = data + 3;
 
-  if (!ReadVarint(blob_size_))
+  if (!ReadVarInt(blob_size_))
     return Reset();
 
   unsigned blob_offset;
-  if (!ReadVarint(blob_offset))
+  if (!ReadVarInt(blob_offset))
     return Reset();
 
   size_t value_blob_count = value->blob_data_->size();
@@ -249,7 +277,7 @@ scoped_refptr<BlobDataHandle> IDBValueUnwrapper::WrapperBlobHandle() {
   return std::move(blob_handle_);
 }
 
-bool IDBValueUnwrapper::ReadVarint(unsigned& value) {
+bool IDBValueUnwrapper::ReadVarInt(unsigned& value) {
   value = 0;
   unsigned shift = 0;
   bool has_another_byte;
@@ -269,6 +297,22 @@ bool IDBValueUnwrapper::ReadVarint(unsigned& value) {
   return true;
 }
 
+bool IDBValueUnwrapper::ReadBytes(Vector<uint8_t>& value) {
+  unsigned length;
+  if (!ReadVarInt(length))
+    return false;
+
+  DCHECK_LE(current_, end_);
+  if (end_ - current_ < static_cast<ptrdiff_t>(length))
+    return false;
+  Vector<uint8_t> result;
+  result.ReserveInitialCapacity(length);
+  result.Append(current_, length);
+  value = std::move(result);
+  current_ += length;
+  return true;
+}
+
 bool IDBValueUnwrapper::Reset() {
 #if DCHECK_IS_ON()
   blob_handle_ = nullptr;

third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h

@@ -12,6 +12,7 @@
 #include "platform/SharedBuffer.h"
 #include "platform/wtf/Allocator.h"
 #include "platform/wtf/Vector.h"
+#include "platform/wtf/text/StringView.h"
 #include "public/platform/WebBlobInfo.h"
 #include "v8/include/v8.h"
 
@@ -28,6 +29,29 @@ class SharedBuffer;
 
 // Logic for serializing V8 values for storage in IndexedDB.
 //
+// An IDBValueWrapper instance drives the serialization of a single V8 value to
+// IndexedDB. An instance's lifecycle goes through the following stages:
+// 1) Cloning - Right after an instance is constructed, its internal
+//    representation is optimized for structured cloning via the Clone() method.
+//    This may be necessary when extracting the primary key and/or index keys
+//    for the serialized value.
+// 2) Wrapping - DoneCloning() transitions the instance to an internal
+//    reprensetation optimized for wrapping via WrapIfBiggerThan().
+// 3) Reading results - After any desired wrapping is performed, the Take*()
+//    methods yield the serialized value components passed to the backing store.
+//    To avoid unnecessary copies, the Take*() methods move out parts of the
+//    internal representation, so each Take*() method can be called at most
+//    once.
+//
+// Example usage:
+//     auto wrapper = new IDBValueWrapper();
+//     wrapper.Clone(...);  // Structured clone used to extract keys.
+//     wrapper.DoneCloning();
+//     wrapper.WrapIfBiggerThan(kWrapThreshold);
+//     wrapper.TakeWireBytes();
+//     wrapper.TakeBlobDataHandles();
+//     wrapper.TakeBlobInfo();
+//
 // V8 values are stored on disk using the format implemented in
 // SerializedScriptValue (SSV), which is essentialy a byte array plus an array
 // of attached Blobs. For "normal" (not too large) V8 values, the SSV output's
@@ -62,6 +86,10 @@ class MODULES_EXPORT IDBValueWrapper {
   //
   // The serialization process can throw an exception. The caller is responsible
   // for checking exception_state.
+  //
+  // The wrapper's internal representation is optimized for cloning the
+  // serialized value. DoneCloning() must be called to transition to an internal
+  // representation optimized for writing.
   IDBValueWrapper(
       v8::Isolate*,
       v8::Local<v8::Value>,
@@ -75,41 +103,55 @@ class MODULES_EXPORT IDBValueWrapper {
   // a value's key and index keys are extracted from a structured clone of the
   // value, which avoids the issue of side-effects in custom getters.
   //
-  // This method cannot be called after WrapIfBiggerThan().
+  // This method cannot be called after DoneCloning().
   void Clone(ScriptState*, ScriptValue* clone);
 
+  // Optimizes the serialized value's internal representation for writing to
+  // disk.
+  //
+  // This must be called before Take*() methods can be called. After this method
+  // is called, Clone() cannot be called anymore.
+  void DoneCloning();
+
   // Conditionally wraps the serialized value's byte array into a Blob.
   //
   // The byte array is wrapped if its size exceeds max_bytes. In production, the
   // max_bytes threshold is currently always kWrapThreshold.
   //
-  // This method must be called before ExtractWireBytes() and cannot be called
-  // after ExtractWireBytes().
+  // This method must be called before the Take*() methods are called.
   bool WrapIfBiggerThan(unsigned max_bytes);
 
-  // Obtains the BlobDataHandles from the serialized value's Blob array.
+  // Obtains the byte array for the serialized value.
   //
   // This method must be called at most once, and must be called after
   // WrapIfBiggerThan().
-  void ExtractBlobDataHandles(
-      Vector<scoped_refptr<BlobDataHandle>>* blob_data_handles);
+  scoped_refptr<SharedBuffer> TakeWireBytes();
 
-  // Obtains the byte array for the serialized value.
+  // Obtains the BlobDataHandles from the serialized value's Blob array.
   //
   // This method must be called at most once, and must be called after
-  // WrapIfBiggerThan().
-  scoped_refptr<SharedBuffer> ExtractWireBytes();
+  // DoneCloning().
+  Vector<scoped_refptr<BlobDataHandle>> TakeBlobDataHandles() {
+#if DCHECK_IS_ON()
+    DCHECK(done_cloning_) << __func__ << " called before DoneCloning()";
+    DCHECK(owns_blob_handles_) << __func__ << " called twice";
+    owns_blob_handles_ = false;
+#endif  // DCHECK_IS_ON()
+
+    return std::move(blob_handles_);
+  }
 
   // Obtains WebBlobInfos for the serialized value's Blob array.
   //
   // This method must be called at most once, and must be called after
-  // WrapIfBiggerThan().
-  inline Vector<WebBlobInfo>& WrappedBlobInfo() {
+  // DoneCloning().
+  inline Vector<WebBlobInfo> TakeBlobInfo() {
 #if DCHECK_IS_ON()
-    DCHECK(!had_exception_)
-        << "WrapBlobInfo() called on wrapper with serialization exception";
+    DCHECK(done_cloning_) << __func__ << " called before DoneCloning()";
+    DCHECK(owns_blob_info_) << __func__ << " called twice";
+    owns_blob_info_ = false;
 #endif  // DCHECK_IS_ON()
-    return blob_info_;
+    return std::move(blob_info_);
   }
 
   size_t DataLengthBeforeWrapInBytes() { return original_data_length_; }
@@ -128,17 +170,32 @@ class MODULES_EXPORT IDBValueWrapper {
       "application/vnd.blink-idb-value-wrapper";
 
   // Used to serialize the wrapped value. Exposed for testing.
-  static void WriteVarint(unsigned value, Vector<char>& output);
+  static void WriteVarInt(unsigned value, Vector<char>& output);
+  static void WriteBytes(const Vector<uint8_t>& bytes, Vector<char>& output);
 
  private:
+  // V8 value serialization state.
   scoped_refptr<SerializedScriptValue> serialized_value_;
-  scoped_refptr<BlobDataHandle> wrapper_handle_;
+  Vector<scoped_refptr<BlobDataHandle>> blob_handles_;
   Vector<WebBlobInfo> blob_info_;
-  Vector<char> wire_bytes_;
+
+  // Buffer for wire data that is not stored in SerializedScriptValue.
+  //
+  // This buffer ends up storing metadata generated by wrapping operations.
+  Vector<char> wire_data_buffer_;
+
+  // Points into SerializedScriptValue's data buffer, or into wire_data_buffer_.
+  StringView wire_data_;
+
   size_t original_data_length_ = 0;
+
 #if DCHECK_IS_ON()
+  // Accounting for lifecycle stages.
   bool had_exception_ = false;
-  bool wrap_called_ = false;
+  bool done_cloning_ = false;
+  bool owns_blob_handles_ = true;
+  bool owns_blob_info_ = true;
+  bool owns_wire_bytes_ = true;
 #endif  // DCHECK_IS_ON()
 };
 
@@ -189,10 +246,11 @@ class MODULES_EXPORT IDBValueUnwrapper {
 
  private:
   // Only present in tests.
-  friend class IDBValueUnwrapperReadVarintTestHelper;
+  friend class IDBValueUnwrapperReadTestHelper;
 
   // Used to deserialize the wrapped value.
-  bool ReadVarint(unsigned& value);
+  bool ReadVarInt(unsigned&);
+  bool ReadBytes(Vector<uint8_t>&);
 
   // Resets the parsing state.
   bool Reset();