Graceful handling of new versions of IndexedDB serialized data.

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

Index: third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
diff --git a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
index f2a8be4e39a38c7f30aafb55075aec6c9dd5f064..a8ef6189e2196ee21dc12aa38420120d348559fc 100644
--- a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
+++ b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
@@ -57,10 +57,6 @@ namespace blink {
 
 static v8::Local<v8::Value> deserializeIDBValueData(v8::Isolate*,
                                                     const IDBValue*);
-static v8::Local<v8::Value> deserializeIDBValue(
-    v8::Isolate*,
-    v8::Local<v8::Object> creationContext,
-    const IDBValue*);
 static v8::Local<v8::Value> deserializeIDBValueArray(
     v8::Isolate*,
     v8::Local<v8::Object> creationContext,
@@ -394,16 +390,21 @@ static v8::Local<v8::Value> deserializeIDBValueData(v8::Isolate* isolate,
 }
 
 // Deserialize the entire IDBValue (injecting key & keypath if present).
-static v8::Local<v8::Value> deserializeIDBValue(
-    v8::Isolate* isolate,
-    v8::Local<v8::Object> creationContext,
-    const IDBValue* value) {
+v8::Local<v8::Value> deserializeIDBValue(v8::Isolate* isolate,
+                                         v8::Local<v8::Object> creationContext,
+                                         const IDBValue* value) {
   ASSERT(isolate->InContext());
   if (!value || value->isNull())
     return v8::Null(isolate);
 
   v8::Local<v8::Value> v8Value = deserializeIDBValueData(isolate, value);
-  if (value->primaryKey()) {
+
+  // De-serialization failures are currently handled by returning null. This is
+  // sub-optimal, as it can cause IndexedDB to return incorrect data. The check

[jsbell, 2017/04/03 19:31:52]

Maybe note that null is a perfectly valid IDB value, for readers unfamiliar with
IDB.

[pwnall, 2017/04/04 00:55:12]

Done.

+  // below is the minimal work needed to avoid a crash. A proper solution would

[jsbell, 2017/04/03 19:31:52]

Since this comment tracks future work, rephrase as a TODO(username) and
reference a crbug ?

[pwnall, 2017/04/04 00:55:12]

I used the crbug targeted by this CL, because the bug asks for the proper
behavior. We can decide whether I'll still work on the bug past shipping this CL
and fixing the crash.

+  // require SerializedScriptValue to throw rather than returning null when it

[jsbell, 2017/04/03 19:31:52]

Return an empty handle?

I wouldn't expect SSV APIs to throw (although serialization may throw if a
getter throws)

[pwnall, 2017/04/04 00:55:12]

Done.
I switched to your suggestion of suggesting returning an empty handle :)

I initially thought that an exception would be the right thing to do in order to
produce different messages for different error conditions. "Version skew, did
you use your profile with a newer version of Chrome?" vs "Invalid tag, your disk
is dead." However, the value of these is low enough to not be worth the
architectural ugliness.

+  // encounters errors.
+  if (value->primaryKey() && !v8Value->IsNull()) {

[jsbell, 2017/04/03 19:31:52]

I wonder if we want to have more comments/DCHECKs here for context: if
primaryKey() is set then the record being retrieved is in an object store with a
key path and key generator, and the object conceptually passed an
assertPrimaryKeyValidOrInjectable() check when serialized therefore it's an
object (not a primitive value)

[pwnall, 2017/04/04 00:55:12]

I think that there is a check at a higher level -- see my note about the
different crash stack when DCHECKs are on at
https://bugs.chromium.org/p/chromium/issues/detail?id=703704#c4

If you'd like an extra DCHECK, it seems like it'd make most sense to 
DCHECK(v8Value.isObject()) right before the value.As<v8::Object>() call in
injectV8KeyIntoV8Value(). WDYT?

     v8::Local<v8::Value> key =
         ToV8(value->primaryKey(), creationContext, isolate);
     if (key.IsEmpty())