Graceful handling of new versions of IndexedDB serialized data.

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

Index: third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp
diff --git a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp
index 9c23c90eb51dbfbc854a10b7428d6eebea9945c9..90da9f94e1514438be637e6454408fcfbe522f1d 100644
--- a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp
+++ b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp
@@ -25,13 +25,24 @@
 
 #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 {
@@ -116,6 +127,52 @@ void checkKeyPathNumberValue(v8::Isolate* isolate,
   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();
@@ -238,4 +295,44 @@ TEST(InjectIDBKeyTest, SubProperty) {
                  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