Graceful handling of new versions of IndexedDB serialized data.

third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *     notice, this list of conditions and the following disclaimer in the
  *     documentation and/or other materials provided with the distribution.
  *
  * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
  * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
  * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
  * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
  * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
  * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
  * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
  * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "bindings/modules/v8/V8BindingForModules.h"
 
+#include "bindings/core/v8/SerializationTag.h"
+#include "bindings/core/v8/SerializedScriptValue.h"
 #include "bindings/core/v8/ToV8.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8BindingForTesting.h"
 #include "bindings/core/v8/V8PerIsolateData.h"
 #include "bindings/modules/v8/ToV8ForModules.h"
+#include "modules/indexeddb/IDBAny.h"
 #include "modules/indexeddb/IDBKey.h"
 #include "modules/indexeddb/IDBKeyPath.h"
+#include "modules/indexeddb/IDBValue.h"
+#include "platform/SharedBuffer.h"
+#include "public/platform/WebBlobInfo.h"
+#include "public/platform/WebData.h"
+#include "public/platform/WebString.h"
+#include "public/platform/modules/indexeddb/WebIDBKey.h"
+#include "public/platform/modules/indexeddb/WebIDBKeyPath.h"
+#include "public/platform/modules/indexeddb/WebIDBValue.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace blink {
 
 namespace {
 
 IDBKey* checkKeyFromValueAndKeyPathInternal(v8::Isolate* isolate,
                                             const ScriptValue& value,
                                             const String& keyPath) {
   IDBKeyPath idbKeyPath(keyPath);
@@ skipping 64 matching lines @@
 void checkKeyPathNumberValue(v8::Isolate* isolate,
                              const ScriptValue& value,
                              const String& keyPath,
                              int expected) {
   IDBKey* idbKey = checkKeyFromValueAndKeyPathInternal(isolate, value, keyPath);
   ASSERT_TRUE(idbKey);
   ASSERT_EQ(IDBKey::NumberType, idbKey->getType());
   ASSERT_TRUE(expected == idbKey->number());
 }
 
+void serializeEmptyObject(V8TestingScope* scope, Vector<char>* wireBytes) {
+  v8::Isolate* isolate = scope->isolate();

[jbroman, 2017/03/31 18:42:59]

nit: Just pass the isolate as an argument, rather than the scope?

[pwnall, 2017/03/31 22:50:24]

Done.
Thank you for catching this! I forgot to clean up after debugging.

+
+  v8::Local<v8::Object> object = v8::Object::New(isolate);
+  ASSERT_TRUE(object->IsObject());
+  NonThrowableExceptionState nonThrowableExceptionState;
+
+  SerializedScriptValue::SerializeOptions options;
+  RefPtr<SerializedScriptValue> serializedValue =
+      SerializedScriptValue::serialize(isolate, object, options,
+                                       nonThrowableExceptionState);
+  serializedValue->toWireBytes(*wireBytes);
+
+  // Sanity check that the serialization header has not changed, as the tests
+  // that use this method rely on the header format.
+  //
+  // The cast from char* to unsigned char* is necessary to avoid VS2015 warning
+  // C4309 (truncation of constant value). This happens because VersionTag is
+  // 0xFF.
+  const unsigned char* wireData =
+      reinterpret_cast<unsigned char*>(wireBytes->data());
+  ASSERT_EQ(
+      static_cast<unsigned char>(SerializedScriptValue::wireFormatVersion),
+      wireData[0]);
+  ASSERT_EQ(static_cast<unsigned char>(VersionTag), wireData[1]);
+
+  // 13 is v8::internal::kLatestVersion in v8/src/value-serializer.cc
+  ASSERT_EQ(static_cast<unsigned char>(13), wireData[2]);

[jbroman, 2017/03/31 18:42:59]

This will make the test fail whenever the v8 version bumps, which will make such
changes difficult to roll into Chromium. Could a weaker expectation work?

(Ideally we wouldn't be hard-coding the format of this header at all, though I'm
not sure I have a fantastic workaround.)

In the near future, V8 may expose this version in an API, but in the meantime it
would probably suffice to just assert that it's at least 13.

[pwnall, 2017/03/31 22:50:24]

Done.
Thank you for the suggestion! This is a very reasonable way to handle the
situation.

+  ASSERT_EQ(static_cast<unsigned char>(VersionTag), wireData[3]);
+}
+
+PassRefPtr<IDBValue> createIDBValue(v8::Isolate* isolate,
+                                    Vector<char>& wireBytes,
+                                    double primaryKey,
+                                    const WebString& keyPath) {
+  DCHECK_GE(wireBytes.size(), 5UL);
+  WebData webData(SharedBuffer::adoptVector(wireBytes));
+  DCHECK_GE(webData.size(), 5UL);
+  Vector<WebBlobInfo> webBlobInfo;
+  WebIDBKey webIdbKey = WebIDBKey::createNumber(42.0);
+  WebIDBKeyPath webIdbKeyPath(WebString("foo"));
+  WebIDBValue webIdbValue(webData, webBlobInfo, webIdbKey, webIdbKeyPath);
+  DCHECK_GE(webIdbValue.data.size(), 5UL);
+  return IDBValue::create(webIdbValue, isolate);
+}
+
 TEST(IDBKeyFromValueAndKeyPathTest, TopLevelPropertyStringValue) {
   V8TestingScope scope;
   v8::Isolate* isolate = scope.isolate();
 
   // object = { foo: "zoo" }
   v8::Local<v8::Object> object = v8::Object::New(isolate);
   ASSERT_TRUE(
       v8CallBoolean(object->Set(scope.context(), v8AtomicString(isolate, "foo"),
                                 v8AtomicString(isolate, "zoo"))));
 
@@ skipping 102 matching lines @@
                  IDBKey::createArray(IDBKey::KeyArray()), scriptObject,
                  "foo.baz");
   checkInjection(scope.getScriptState(),
                  IDBKey::createArray(IDBKey::KeyArray()), scriptObject, "bar");
 
   checkInjectionDisallowed(scope.getScriptState(), scriptObject, "foo.bar.baz");
   checkInjection(scope.getScriptState(), IDBKey::createString("zoo"),
                  scriptObject, "foo.xyz.foo");
 }
 
+TEST(DeserializeIDBValueTest, CurrentVersions) {
+  V8TestingScope scope;
+  v8::Isolate* isolate = scope.isolate();
+
+  Vector<char> objectBytes;
+  serializeEmptyObject(&scope, &objectBytes);
+  RefPtr<IDBValue> idbValue = createIDBValue(isolate, objectBytes, 42.0, "foo");
+
+  v8::Local<v8::Value> v8Value =
+      deserializeIDBValue(isolate, scope.context()->Global(), idbValue.get());
+  EXPECT_TRUE(!scope.getExceptionState().hadException());
+
+  ASSERT_TRUE(v8Value->IsObject());
+  v8::Local<v8::Object> v8ValueObject = v8Value.As<v8::Object>();
+  v8::Local<v8::Value> v8NumberValue =
+      v8ValueObject->Get(scope.context(), v8AtomicString(isolate, "foo"))
+          .ToLocalChecked();
+  ASSERT_TRUE(v8NumberValue->IsNumber());
+  v8::Local<v8::Number> v8Number = v8NumberValue.As<v8::Number>();
+  EXPECT_EQ(v8Number->Value(), 42.0);
+}
+
+TEST(DeserializeIDBValueTest, FutureV8Version) {
+  V8TestingScope scope;
+  v8::Isolate* isolate = scope.isolate();
+
+  // These steps reproduce the circumstances of http://crbug.com/703704
+  Vector<char> objectBytes;
+  serializeEmptyObject(&scope, &objectBytes);
+  objectBytes[2] += 1;
+  RefPtr<IDBValue> idbValue = createIDBValue(isolate, objectBytes, 42.0, "foo");
+
+  v8::Local<v8::Value> v8Value =
+      deserializeIDBValue(isolate, scope.context()->Global(), idbValue.get());
+  // The exception check below will need to be changed when we throw exceptions
+  // instead of returning null on deserialization errors.

[jbroman, 2017/03/31 18:42:59]

V8 does throw, but Blink throwing is a more complicated question, because there
are a variety of APIs that can deserialize, and many do so lazily (e.g.
deserialization of postMessage body happens during the MessageEvent data getter,
which might be a surprising place for an exception to emerge) -- and would
probably have interop implications.

[pwnall, 2017/03/31 22:50:24]

I made the comment here be less assertive.

Thank you very much for the heads up regarding the complexity in changing
deserialization. I haven't looked at this code for too long, so I have very
little clue what's going on. (best situation to plan improvements? :D)

IIUC, if serialization fails currently, we'll have a spurious message event
whose data is null? Ideally, we'd not invoke the app's event handler if
deserialization fails. Based on the little I know so far, that'd discard the
scope, so the thrown exception would be ignored? (sorry if this is completely
wrong, i'm still figuring things out)

I didn't know that the postMessage spec covers situations where deserialization
fails. I'll pester jsbell for details. Thank you for opening my eyes to all
this!

+  EXPECT_TRUE(!scope.getExceptionState().hadException());
+  EXPECT_TRUE(v8Value->IsNull());
+}
+
 }  // namespace blink