Indexed DB: Fix compound index keys vs. autoincrement stores

Stores with (1) a keyPath (a.k.a. inline keys) and (2) autoIncrement
(a.k.a. a key generator) have primary keys generated by the browser
and inserted into objects lazily, since the renderer doesn't know what
the primary keys will be when the object is serialized.

Indexes might have a keyPath that references the same spot in the
object. This is handled by checking if the keypaths match. If so,
the browser synthesizes the index key (same as the primary key). But
Chrome was not handling the case where the index had a compound
key - a keypath that's an array, plucking multiple values out of
the object. An object with unresolved keypaths would normally just
not be indexed, per spec. But since the primary keys should be injected
before the indexing occurs, these should be indexed.

Fix this by sending the index keys from the renderer to the browser
as an array with "holes" that need to be filled in.

This is covered by an existing Web Platform Test, which we now pass.

Bug: 701972
Change-Id: I14940b23cfcbb7f8b673143b402f574220184fd7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1728058
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692313}

content/browser/indexed_db/indexed_db_index_writer.cc

@@ -13,6 +13,7 @@
 #include "content/browser/indexed_db/indexed_db_tracing.h"
 #include "content/browser/indexed_db/indexed_db_transaction.h"
 #include "third_party/blink/public/common/indexeddb/indexeddb_metadata.h"
+#include "third_party/blink/public/mojom/indexeddb/indexeddb.mojom.h"
 
 using base::ASCIIToUTF16;
 using blink::IndexedDBIndexKeys;
@@ -131,13 +132,27 @@ bool MakeIndexWriters(IndexedDBTransaction* transaction,
     // A copy is made because additional keys may be added.
     std::vector<IndexedDBKey> keys = it.keys;
 
-    // If the object_store is using auto_increment, then any indexes with an
-    // identical key_path need to also use the primary (generated) key as a key.
-    if (key_was_generated && (index.key_path == object_store.key_path))
-      keys.push_back(primary_key);
+    // If the object_store is using a key generator to produce the primary key,
+    // and the store uses in-line keys, index key paths may reference it.
+    if (key_was_generated && !object_store.key_path.IsNull()) {
+      if (index.key_path == object_store.key_path) {
+        // The index key path is the same as the store's key path - no index key
+        // will have been sent by the front end, so synthesize one here.
+        keys.push_back(primary_key);
+
+      } else if (index.key_path.type() == blink::mojom::IDBKeyPathType::Array) {
+        // An index with compound keys for a store with a key generator and
+        // in-line keys may need subkeys filled in. These are represented as
+        // "holes", which are not otherwise allowed.
+        for (size_t i = 0; i < keys.size(); ++i) {
+          if (keys[i].HasHoles())
+            keys[i] = keys[i].FillHoles(primary_key);
+        }
+      }
+    }
 
     std::unique_ptr<IndexWriter> index_writer(
-        std::make_unique<IndexWriter>(index, keys));
+        std::make_unique<IndexWriter>(index, std::move(keys)));
     bool can_add_keys = false;
     bool backing_store_success =
         index_writer->VerifyIndexKeys(backing_store,

third_party/blink/common/indexeddb/indexeddb_key.cc

@@ -92,6 +92,38 @@ bool IndexedDBKey::Equals(const IndexedDBKey& other) const {
   return !CompareTo(other);
 }
 
+bool IndexedDBKey::HasHoles() const {
+  if (type_ != mojom::IDBKeyType::Array)
+    return false;
+
+  for (const auto& subkey : array_) {
+    if (subkey.type() == mojom::IDBKeyType::None)
+      return true;
+  }
+  return false;
+}
+
+IndexedDBKey IndexedDBKey::FillHoles(const IndexedDBKey& primary_key) const {
+  if (type_ != mojom::IDBKeyType::Array)
+    return IndexedDBKey(*this);
+
+  std::vector<IndexedDBKey> subkeys;
+  subkeys.reserve(array_.size());
+  for (const auto& subkey : array_) {
+    if (subkey.type() == mojom::IDBKeyType::None) {
+      subkeys.push_back(primary_key);
+    } else {
+      // "Holes" can only exist at the top level of an array key, as (1) they
+      // are produced by an index's array keypath when a member matches the
+      // store's keypath, and (2) array keypaths are flat (no
+      // arrays-of-arrays).
+      DCHECK(!subkey.HasHoles());
+      subkeys.push_back(subkey);
+    }
+  }
+  return IndexedDBKey(subkeys);
+}
+
 int IndexedDBKey::CompareTo(const IndexedDBKey& other) const {
   DCHECK(IsValid());
   DCHECK(other.IsValid());

third_party/blink/public/common/indexeddb/indexeddb_key.h

@@ -61,6 +61,15 @@ class BLINK_COMMON_EXPORT IndexedDBKey {
 
   size_t size_estimate() const { return size_estimate_; }
 
+  // Tests if this array-type key has "holes". Used in cases where a compound
+  // key references an auto-generated primary key.
+  bool HasHoles() const;
+
+  // Returns a copy of this array-type key, but with "holes" replaced by the
+  // given primary key. Used in cases where a compound key references an
+  // auto-generated primary key.
+  IndexedDBKey FillHoles(const IndexedDBKey&) const WARN_UNUSED_RESULT;
+
  private:
   int CompareTo(const IndexedDBKey& other) const;
 

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules.cc

@@ -439,6 +439,57 @@ static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPath(
                                          exception_state);
 }
 
+// Evaluate an index's key path against a value and return a key. This
+// handles the special case for indexes where a compound key path
+// may result in "holes", depending on the store's properties.
+// Otherwise, nullptr is returned.
+// https://w3c.github.io/IndexedDB/#evaluate-a-key-path-on-a-value
+// A V8 exception may be thrown on bad data or by script's getters; if so,
+// callers should not make further V8 calls.
+static std::unique_ptr<IDBKey> CreateIDBKeyFromValueAndKeyPaths(
+    v8::Isolate* isolate,
+    v8::Local<v8::Value> value,
+    const IDBKeyPath& store_key_path,
+    const IDBKeyPath& index_key_path,
+    ExceptionState& exception_state) {
+  DCHECK(!index_key_path.IsNull());
+  v8::HandleScope handle_scope(isolate);
+  if (index_key_path.GetType() == mojom::IDBKeyPathType::Array) {
+    const Vector<String>& array = index_key_path.Array();
+    const bool uses_inline_keys =
+        store_key_path.GetType() == mojom::IDBKeyPathType::String;
+    IDBKey::KeyArray result;
+    result.ReserveInitialCapacity(array.size());
+    for (const String& path : array) {
+      auto key = CreateIDBKeyFromValueAndKeyPath(isolate, value, path,
+                                                 exception_state);
+      if (exception_state.HadException())
+        return nullptr;
+      if (!key && uses_inline_keys && store_key_path.GetString() == path) {
+        // Compound keys that include the store's inline primary key which
+        // will be generated lazily are represented as "holes".
+        key = IDBKey::CreateNone();
+      } else if (!key) {
+        // Key path evaluation failed.
+        return nullptr;
+      } else if (!key->IsValid()) {
+        // An Invalid key is returned if not valid in this case (but not the
+        // other CreateIDBKeyFromValueAndKeyPath function) because:
+        // * Invalid members are only allowed for multi-entry arrays.
+        // * Array key paths can't be multi-entry.
+        return IDBKey::CreateInvalid();
+      }
+
+      result.emplace_back(std::move(key));
+    }
+    return IDBKey::CreateArray(std::move(result));
+  }
+
+  DCHECK_EQ(index_key_path.GetType(), mojom::IDBKeyPathType::String);
+  return CreateIDBKeyFromValueAndKeyPath(
+      isolate, value, index_key_path.GetString(), exception_state);
+}
+
 // Deserialize just the value data & blobInfo from the given IDBValue.
 //
 // Primary key injection is performed in deserializeIDBValue() below.
@@ -713,6 +764,17 @@ std::unique_ptr<IDBKey> NativeValueTraits<std::unique_ptr<IDBKey>>::NativeValue(
                                          exception_state);
 }
 
+std::unique_ptr<IDBKey> NativeValueTraits<std::unique_ptr<IDBKey>>::NativeValue(
+    v8::Isolate* isolate,
+    v8::Local<v8::Value> value,
+    ExceptionState& exception_state,
+    const IDBKeyPath& store_key_path,
+    const IDBKeyPath& index_key_path) {
+  IDB_TRACE("createIDBKeyFromValueAndKeyPaths");
+  return CreateIDBKeyFromValueAndKeyPaths(isolate, value, store_key_path,
+                                          index_key_path, exception_state);
+}
+
 IDBKeyRange* NativeValueTraits<IDBKeyRange*>::NativeValue(
     v8::Isolate* isolate,
     v8::Local<v8::Value> value,

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules.h

@@ -66,7 +66,7 @@ struct NativeValueTraits<std::unique_ptr<IDBKey>> {
   // Implementation for ScriptValue::To<std::unique_ptr<IDBKey>>().
   //
   // Used by Indexed DB when generating the primary key for a record that is
-  // being stored in an object store that uses in-line keys, or an index key.
+  // being stored in an object store that uses in-line keys.
   // https://w3c.github.io/IndexedDB/#extract-key-from-value
   //
   // Evaluates the given key path against the script value to produce an
@@ -83,6 +83,34 @@ struct NativeValueTraits<std::unique_ptr<IDBKey>> {
       v8::Local<v8::Value>,
       ExceptionState&,
       const IDBKeyPath&);
+
+  // Implementation for ScriptValue::To<std::unique_ptr<IDBKey>>().
+  //
+  // Used by Indexed DB when generating the index key for a record that is being
+  // stored.
+  // https://w3c.github.io/IndexedDB/#extract-key-from-value
+  //
+  // Evaluates the given key path against the script value to produce an IDBKey.
+  // Returns either:
+  // * A nullptr, if key path evaluation fails.
+  // * An Invalid key, if the evaluation yielded a non-key.
+  // * An IDBKey, otherwise.
+  //
+  // Note that an Array key may contain Invalid members, as the
+  // "multi-entry" index case allows these, and will filter them out later. Use
+  // IsValid() to recursively check.
+  //
+  // If evaluating an index's key path which is an array, and the sub-key path
+  // matches the object store's key path, and that evaluation fails, then a
+  // None key member will be present in the Array key result. This should only
+  // occur when the store has a key generator, which would fill in the primary
+  // key lazily.
+  MODULES_EXPORT static std::unique_ptr<IDBKey> NativeValue(
+      v8::Isolate*,
+      v8::Local<v8::Value>,
+      ExceptionState&,
+      const IDBKeyPath& store_key_path,
+      const IDBKeyPath& index_key_path);
 };
 
 template <>

third_party/blink/renderer/bindings/modules/v8/v8_binding_for_modules_test.cc

@@ -139,6 +139,20 @@ void CheckKeyPathNumberValue(v8::Isolate* isolate,
   ASSERT_TRUE(expected == idb_key->Number());
 }
 
+// Compare a key against an array of keys. Supports keys with "holes" (keys of
+// type None), so IDBKey::Compare() can't be used directly.
+void CheckArrayKey(const IDBKey* key, const IDBKey::KeyArray& expected) {
+  EXPECT_EQ(mojom::IDBKeyType::Array, key->GetType());
+  const IDBKey::KeyArray& array = key->Array();
+  EXPECT_EQ(expected.size(), array.size());
+  for (wtf_size_t i = 0; i < array.size(); ++i) {
+    EXPECT_EQ(array[i]->GetType(), expected[i]->GetType());
+    if (array[i]->GetType() != mojom::IDBKeyType::None) {
+      EXPECT_EQ(0, expected[i]->Compare(array[i].get()));
+    }
+  }
+}
+
 // SerializedScriptValue header format offsets are inferred from the Blink and
 // V8 serialization code. The code below DCHECKs that
 constexpr static size_t kSSVHeaderBlinkVersionTagOffset = 0;
@@ -299,6 +313,66 @@ TEST(IDBKeyFromValueAndKeyPathTest, Exceptions) {
         IDBKeyPath(Vector<String>{"id", "throws"})));
     EXPECT_TRUE(exception_state.HadException());
   }
+
+  {
+    // Compound key path references a property that throws, index case.
+    DummyExceptionStateForTesting exception_state;
+    EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+        scope.GetIsolate(), script_value, exception_state,
+        /*store_key_path=*/IDBKeyPath("id"),
+        /*index_key_path=*/IDBKeyPath(Vector<String>{"id", "throws"})));
+    EXPECT_TRUE(exception_state.HadException());
+  }
+}
+
+TEST(IDBKeyFromValueAndKeyPathsTest, IndexKeys) {
+  V8TestingScope scope;
+  ScriptState* script_state = scope.GetScriptState();
+  v8::Isolate* isolate = scope.GetIsolate();
+  NonThrowableExceptionState exception_state;
+
+  // object = { foo: { bar: "zee" }, bad: null }
+  ScriptValue script_value =
+      V8ObjectBuilder(script_state)
+          .Add("foo", V8ObjectBuilder(script_state).Add("bar", "zee"))
+          .AddNull("bad")
+          .GetScriptValue();
+
+  // Index key path member matches store key path.
+  std::unique_ptr<IDBKey> key = ScriptValue::To<std::unique_ptr<IDBKey>>(
+      isolate, script_value, exception_state,
+      /*store_key_path=*/IDBKeyPath("id"),
+      /*index_key_path=*/IDBKeyPath(Vector<String>{"id", "foo.bar"}));
+  IDBKey::KeyArray expected;
+  expected.emplace_back(IDBKey::CreateNone());
+  expected.emplace_back(IDBKey::CreateString("zee"));
+  CheckArrayKey(key.get(), expected);
+
+  // Index key path member matches, but there are unmatched members too.
+  EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+      isolate, script_value, exception_state,
+      /*store_key_path=*/IDBKeyPath("id"),
+      /*index_key_path=*/IDBKeyPath(Vector<String>{"id", "foo.bar", "nope"})));
+
+  // Index key path member matches, but there are invalid subkeys too.
+  EXPECT_FALSE(
+      ScriptValue::To<std::unique_ptr<IDBKey>>(
+          isolate, script_value, exception_state,
+          /*store_key_path=*/IDBKeyPath("id"),
+          /*index_key_path=*/IDBKeyPath(Vector<String>{"id", "foo.bar", "bad"}))
+          ->IsValid());
+
+  // Index key path member does not match store key path.
+  EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+      isolate, script_value, exception_state,
+      /*store_key_path=*/IDBKeyPath("id"),
+      /*index_key_path=*/IDBKeyPath(Vector<String>{"id2", "foo.bar"})));
+
+  // Index key path is not array, matches store key path.
+  EXPECT_FALSE(ScriptValue::To<std::unique_ptr<IDBKey>>(
+      isolate, script_value, exception_state,
+      /*store_key_path=*/IDBKeyPath("id"),
+      /*index_key_path=*/IDBKeyPath("id")));
 }
 
 TEST(InjectIDBKeyTest, ImplicitValues) {

third_party/blink/renderer/modules/indexeddb/idb_object_store.cc

@@ -300,11 +300,19 @@ IDBRequest* IDBObjectStore::getAllKeys(ScriptState* script_state,
 
 static Vector<std::unique_ptr<IDBKey>> GenerateIndexKeysForValue(
     v8::Isolate* isolate,
+    const IDBObjectStoreMetadata& store_metadata,
     const IDBIndexMetadata& index_metadata,
     const ScriptValue& object_value) {
   NonThrowableExceptionState exception_state;
+
+  // Look up the key using the index's key path.
   std::unique_ptr<IDBKey> index_key = ScriptValue::To<std::unique_ptr<IDBKey>>(
-      isolate, object_value, exception_state, index_metadata.key_path);
+      isolate, object_value, exception_state, store_metadata.key_path,
+      index_metadata.key_path);
+
+  // No match. (In the special case for a store with a key generator and in-line
+  // keys and where the store and index key paths match, the back-end will
+  // synthesize an index key.)
   if (!index_key)
     return Vector<std::unique_ptr<IDBKey>>();
 
@@ -542,10 +550,10 @@ IDBRequest* IDBObjectStore::DoPut(ScriptState* script_state,
   for (const auto& it : Metadata().indexes) {
     if (clone.IsEmpty())
       value_wrapper.Clone(script_state, &clone);
-    index_keys.emplace_back(
-        IDBIndexKeys{.id = it.key,
-                     .keys = GenerateIndexKeysForValue(
-                         script_state->GetIsolate(), *it.value, clone)});
+    index_keys.emplace_back(IDBIndexKeys{
+        .id = it.key,
+        .keys = GenerateIndexKeysForValue(script_state->GetIsolate(),
+                                          Metadata(), *it.value, clone)});
   }
   // Records 1KB to 1GB.
   UMA_HISTOGRAM_COUNTS_1M(
@@ -687,11 +695,13 @@ class IndexPopulator final : public NativeEventListener {
                  IDBDatabase* database,
                  int64_t transaction_id,
                  int64_t object_store_id,
+                 scoped_refptr<const IDBObjectStoreMetadata> store_metadata,
                  scoped_refptr<const IDBIndexMetadata> index_metadata)
       : script_state_(script_state),
         database_(database),
         transaction_id_(transaction_id),
         object_store_id_(object_store_id),
+        store_metadata_(store_metadata),
         index_metadata_(std::move(index_metadata)) {
     DCHECK(index_metadata_.get());
   }
@@ -703,6 +713,9 @@ class IndexPopulator final : public NativeEventListener {
   }
 
  private:
+  const IDBObjectStoreMetadata& ObjectStoreMetadata() const {
+    return *store_metadata_;
+  }
   const IDBIndexMetadata& IndexMetadata() const { return *index_metadata_; }
 
   void Invoke(ExecutionContext* execution_context, Event* event) override {
@@ -737,6 +750,7 @@ class IndexPopulator final : public NativeEventListener {
       index_keys.emplace_back(IDBIndexKeys{
           .id = IndexMetadata().id,
           .keys = GenerateIndexKeysForValue(script_state_->GetIsolate(),
+                                            ObjectStoreMetadata(),
                                             IndexMetadata(), value)});
 
       database_->Backend()->SetIndexKeys(transaction_id_, object_store_id_,
@@ -757,6 +771,7 @@ class IndexPopulator final : public NativeEventListener {
   Member<IDBDatabase> database_;
   const int64_t transaction_id_;
   const int64_t object_store_id_;
+  scoped_refptr<const IDBObjectStoreMetadata> store_metadata_;
   scoped_refptr<const IDBIndexMetadata> index_metadata_;
 };
 }  // namespace
@@ -838,7 +853,7 @@ IDBIndex* IDBObjectStore::createIndex(ScriptState* script_state,
   // This is kept alive by being the success handler of the request, which is in
   // turn kept alive by the owning transaction.
   auto* index_populator = MakeGarbageCollected<IndexPopulator>(
-      script_state, transaction()->db(), transaction_->Id(), Id(),
+      script_state, transaction()->db(), transaction_->Id(), Id(), metadata_,
       std::move(index_metadata));
   index_request->setOnsuccess(index_populator);
   return index;

third_party/blink/web_tests/external/wpt/IndexedDB/idbobjectstore_createIndex15-autoincrement-expected.txt

-This is a testharness.js-based test.
-PASS Explicit Primary Key
-FAIL Auto-Increment Primary Key assert_equals: Expected 100. expected (number) 100 but got (object) null
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/IndexedDB/idbobjectstore_createIndex15-autoincrement.htm

@@ -49,4 +49,57 @@
     },
     "Auto-Increment Primary Key"
   );
+
+  indexeddb_test(
+    function(t, db, txn) {
+      // Auto-increment
+      var store = db.createObjectStore("Store3", {keyPath: "id", autoIncrement: true});
+      store.createIndex("CompoundKey", ["num", "id", "other"]);
+
+      var num = 100;
+
+      // Add data to Store3 - valid keys
+      // Objects will be stored in Store3 and keys will get added
+      // to the CompoundKeys index.
+      store.put({num: num++, other: 0});
+      store.put({num: num++, other: [0]});
+
+      // Add data - missing key
+      // Objects will be stored in Store3 but keys won't get added to
+      // the CompoundKeys index because the 'other' keypath doesn't
+      // resolve to a value.
+      store.put({num: num++});
+
+      // Add data to Store3 - invalid keys
+      // Objects will be stored in Store3 but keys won't get added to
+      // the CompoundKeys index because the 'other' property values
+      // aren't valid keys.
+      store.put({num: num++, other: null});
+      store.put({num: num++, other: {}});
+      store.put({num: num++, other: [null]});
+      store.put({num: num++, other: [{}]});
+    },
+    function(t, db) {
+      var store = db.transaction("Store3", "readwrite").objectStore("Store3");
+      const keys = [];
+      let count;
+      store.count().onsuccess = t.step_func(e => { count = e.target.result; });
+      store.index("CompoundKey").openCursor().onsuccess = t.step_func(function(e) {
+        const cursor = e.target.result;
+        if (cursor !== null) {
+          keys.push(cursor.key);
+          cursor.continue();
+          return;
+        }
+
+        // Done iteration, check results.
+        assert_equals(count, 7, 'Expected all 7 records to be stored.');
+        assert_equals(keys.length, 2, 'Expected exactly two index entries.');
+        assert_array_equals(keys[0], [100, 1, 0]);
+        assert_object_equals(keys[1], [101, 2, [0]]);
+        t.done();
+      });
+    },
+    "Auto-Increment Primary Key - invalid key values elsewhere"
+  );
 </script>