Graceful handling of new versions of IndexedDB serialized data.

third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.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
@@ skipping 39 matching lines @@
 #include "modules/indexeddb/IDBValue.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/SharedBuffer.h"
 #include "wtf/MathExtras.h"
 #include "wtf/Vector.h"
 
 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,
     const Vector<RefPtr<IDBValue>>*);
 
 v8::Local<v8::Value> ToV8(const IDBKeyPath& value,
                           v8::Local<v8::Object> creationContext,
                           v8::Isolate* isolate) {
   switch (value.getType()) {
     case IDBKeyPath::NullType:
@@ skipping 313 matching lines @@
 
   RefPtr<SerializedScriptValue> serializedValue =
       value->createSerializedValue();
   SerializedScriptValue::DeserializeOptions options;
   options.blobInfo = value->blobInfo();
   options.readWasmFromStream = true;
   return serializedValue->deserialize(isolate, options);
 }
 
 // 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())
       return v8::Local<v8::Value>();
     bool injected =
         injectV8KeyIntoV8Value(isolate, key, v8Value, value->keyPath());
     DCHECK(injected);
   }
 
   return v8Value;
@@ skipping 192 matching lines @@
   if (expectedKey && expectedKey->isEqual(value->primaryKey()))
     return;
 
   bool injected = injectV8KeyIntoV8Value(
       isolate, keyValue.v8Value(), scriptValue.v8Value(), value->keyPath());
   DCHECK(injected);
 }
 #endif
 
 }  // namespace blink