[TransactionalLevelDB] Fix iterating 'Prev' from evicted iterators

Iterators in TransactionalLevelDB are evicted when there are any
changes to the database. There is an edge case where an iterator is
evicted while on the 'last' key of the database, and that key is
deleted. When reloaded, it Seek()s to the previous key, only to
become 'Invalid' because it reaches the end of the database.
Unfortunately leveldb doesn't allow 'Prev' to be called from the end
of the database and instead just stays invalid.

The fix checks for the state where 'the iterator was valid before
eviction and is invalid after reloading' in the Prev() method. In
this state, there must be no no keys at or after the previously
loaded key. Thus calling SeekToLast() will correctly position the
iterator at the first element 'Prev' the previously loaded key.

Bug: 1022594
Change-Id: Ifa938b441683ea9f2cac5d926ff22b734ab4bb67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897007
Commit-Queue: Daniel Murphy <[email protected]>
Auto-Submit: Daniel Murphy <[email protected]>
Reviewed-by: Ken Rockot <[email protected]>
Cr-Commit-Position: refs/heads/master@{#713947}

content/browser/indexed_db/leveldb/transactional_leveldb_iterator.cc

@@ -146,11 +146,14 @@ leveldb::Status TransactionalLevelDBIterator::Prev() {
   if (!s.ok())
     return s;
 
-  // Exit early if not valid.
+  // If invalid, that means the current key has been deleted AND it was at the
+  // end of the database. In this case, seeking to the last item is the same as
+  // 'Prev'-ing from the deleted item.
   if (!IsValid())
-    return WrappedIteratorStatus();
+    iterator_->SeekToLast();
+  else
+    iterator_->Prev();
 
-  iterator_->Prev();
   PrevPastScopesMetadata();
   return WrappedIteratorStatus();
 }

content/browser/indexed_db/leveldb/transactional_leveldb_transaction_unittest.cc

@@ -924,4 +924,74 @@ TEST_F(TransactionalLevelDBTransactionTest,
   EXPECT_TRUE(KeysEqual(it->Key(), key2)) << it->Key() << ", " << key2;
 }
 
+TEST_F(TransactionalLevelDBTransactionTest,
+       IteratorPrevAfterRemovingCurrentKeyAtDatabaseEnd) {
+  SetUpRealDatabase();
+  SetupLevelDBDatabase();
+
+  // This tests that the iterator reloading logic correctly handles not calling
+  // Next when it reloads after the current key was removed.
+
+  const std::string key1("b-key1");
+  const std::string key2("b-key2");
+  const std::string value("value");
+
+  Put(key1, value);
+  Put(key2, value);
+
+  scoped_refptr<TransactionalLevelDBTransaction> transaction =
+      CreateTransaction();
+  std::unique_ptr<TransactionalLevelDBIterator> it =
+      transaction->CreateIterator();
+
+  leveldb::Status s = it->Seek(std::string("b-key2"));
+  ASSERT_TRUE(it->IsValid());
+  EXPECT_TRUE(s.ok());
+
+  // Make sure the iterator is detached, and remove the current key.
+  it->EvictLevelDBIterator();
+  TransactionRemove(transaction.get(), key2);
+
+  // This call reloads the iterator at key "b-key2", which is now deleted. It
+  // should seek to the end of the database instead, which is "b-key1"
+  s = it->Prev();
+  ASSERT_TRUE(it->IsValid());
+  EXPECT_TRUE(s.ok());
+  EXPECT_TRUE(KeysEqual(it->Key(), key1)) << it->Key() << ", " << key1;
+}
+
+TEST_F(TransactionalLevelDBTransactionTest,
+       IteratorPrevAfterRemovingCurrentKeyAtDatabaseStart) {
+  SetUpRealDatabase();
+  SetupLevelDBDatabase();
+
+  // This tests that the iterator reloading logic correctly handles not calling
+  // Next when it reloads after the current key was removed.
+
+  const std::string key1("b-key1");
+  const std::string key2("b-key2");
+  const std::string value("value");
+
+  Put(key1, value);
+  Put(key2, value);
+
+  scoped_refptr<TransactionalLevelDBTransaction> transaction =
+      CreateTransaction();
+  std::unique_ptr<TransactionalLevelDBIterator> it =
+      transaction->CreateIterator();
+
+  leveldb::Status s = it->Seek(std::string("b-key1"));
+  ASSERT_TRUE(it->IsValid());
+  EXPECT_TRUE(s.ok());
+
+  // Make sure the iterator is detached, and remove the current key.
+  it->EvictLevelDBIterator();
+  TransactionRemove(transaction.get(), key1);
+
+  // This call reloads the iterator at key "b-key1", which is now deleted. Since
+  // there is no key before it, it should be invalid.
+  s = it->Prev();
+  ASSERT_FALSE(it->IsValid());
+}
+
 }  // namespace content

third_party/blink/web_tests/external/wpt/IndexedDB/idbindex_reverse_cursor.any.js

@@ -0,0 +1,60 @@
+// META: title=Reverse Cursor Validity
+// META: script=support-promises.js
+
+async function iterateAndReturnAllCursorResult(testCase, cursor) {
+  return new Promise((resolve, reject) => {
+    let results = [];
+    cursor.onsuccess = testCase.step_func(function(e) {
+      let cursor = e.target.result;
+      if (!cursor) {
+        resolve(results);
+        return;
+      }
+      results.push(cursor.value);
+      cursor.continue();
+    });
+    cursor.onerror = reject;
+  });
+}
+
+promise_test(async testCase => {
+  const db = await createDatabase(testCase, db => {
+    db.createObjectStore('objectStore', {keyPath: 'key'})
+              .createIndex('index', 'indexedOn');
+  });
+  const txn1 = db.transaction(['objectStore'], 'readwrite');
+  txn1.objectStore('objectStore').add({'key': 'firstItem', 'indexedOn': 3});
+  const txn2 = db.transaction(['objectStore'], 'readwrite');
+  txn2.objectStore('objectStore').put({'key': 'firstItem', 'indexedOn': -1});
+  const txn3= db.transaction(['objectStore'], 'readwrite');
+  txn3.objectStore('objectStore').add({'key': 'secondItem', 'indexedOn': 2});
+
+  const txn4 = db.transaction(['objectStore'], 'readonly');
+  cursor = txn4.objectStore('objectStore').index('index').openCursor(IDBKeyRange.bound(0, 10), "prev");
+  let results = await iterateAndReturnAllCursorResult(testCase, cursor);
+
+  assert_equals(results.length, 1);
+
+  await promiseForTransaction(testCase, txn4);
+  db.close();
+}, 'Reverse cursor sees update from separate transactions.');
+
+promise_test(async testCase => {
+  const db = await createDatabase(testCase, db => {
+    db.createObjectStore('objectStore', {keyPath: 'key'})
+              .createIndex('index', 'indexedOn');
+  });
+  const txn = db.transaction(['objectStore'], 'readwrite');
+  txn.objectStore('objectStore').add({'key': '1', 'indexedOn': 2});
+  txn.objectStore('objectStore').put({'key': '1', 'indexedOn': -1});
+  txn.objectStore('objectStore').add({'key': '2', 'indexedOn': 1});
+
+  const txn2 = db.transaction(['objectStore'], 'readonly');
+  cursor = txn2.objectStore('objectStore').index('index').openCursor(IDBKeyRange.bound(0, 10), "prev");
+  let results = await iterateAndReturnAllCursorResult(testCase, cursor);
+
+  assert_equals(1, results.length);
+
+  await promiseForTransaction(testCase, txn2);
+  db.close();
+}, 'Reverse cursor sees in-transaction update.');