IndexedDB: Prevent store/index deletion from racing ahead of use

content/browser/indexed_db/indexed_db_database_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/indexed_db/indexed_db_database.h"
 
 #include "base/auto_reset.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/indexed_db/indexed_db.h"
 #include "content/browser/indexed_db/indexed_db_backing_store.h"
 #include "content/browser/indexed_db/indexed_db_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_connection.h"
 #include "content/browser/indexed_db/indexed_db_cursor.h"
 #include "content/browser/indexed_db/indexed_db_database.h"
 #include "content/browser/indexed_db/indexed_db_factory.h"
 #include "content/browser/indexed_db/indexed_db_fake_backing_store.h"
 #include "content/browser/indexed_db/indexed_db_transaction.h"
 #include "content/browser/indexed_db/mock_indexed_db_callbacks.h"
 #include "content/browser/indexed_db/mock_indexed_db_database_callbacks.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
 
 namespace {
-const int FAKE_CHILD_PROCESS_ID = 0;
+const int kFakeChildProcessId = 0;
 }
 
 namespace content {
 
 TEST(IndexedDBDatabaseTest, BackingStoreRetention) {
   scoped_refptr<IndexedDBFakeBackingStore> backing_store =
       new IndexedDBFakeBackingStore();
   EXPECT_TRUE(backing_store->HasOneRef());
 
   IndexedDBFactory* factory = 0;
@@ skipping 26 matching lines @@
   ASSERT_TRUE(s.ok());
   EXPECT_FALSE(backing_store->HasOneRef());  // local and db
 
   scoped_refptr<MockIndexedDBCallbacks> request1(new MockIndexedDBCallbacks());
   scoped_refptr<MockIndexedDBDatabaseCallbacks> callbacks1(
       new MockIndexedDBDatabaseCallbacks());
   const int64 transaction_id1 = 1;
   IndexedDBPendingConnection connection1(
       request1,
       callbacks1,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id1,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection1);
 
   EXPECT_FALSE(backing_store->HasOneRef());  // db, connection count > 0
 
   scoped_refptr<MockIndexedDBCallbacks> request2(new MockIndexedDBCallbacks());
   scoped_refptr<MockIndexedDBDatabaseCallbacks> callbacks2(
       new MockIndexedDBDatabaseCallbacks());
   const int64 transaction_id2 = 2;
   IndexedDBPendingConnection connection2(
       request2,
       callbacks2,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id2,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection2);
 
   EXPECT_FALSE(backing_store->HasOneRef());  // local and connection
 
   request1->connection()->ForceClose();
   EXPECT_FALSE(request1->connection()->IsConnected());
 
   EXPECT_FALSE(backing_store->HasOneRef());  // local and connection
@@ skipping 23 matching lines @@
   ASSERT_TRUE(s.ok());
   EXPECT_FALSE(backing_store->HasOneRef());  // local and db
 
   scoped_refptr<MockIndexedDBDatabaseCallbacks> callbacks(
       new MockIndexedDBDatabaseCallbacks());
   scoped_refptr<MockIndexedDBCallbacks> request(new MockIndexedDBCallbacks());
   const int64 upgrade_transaction_id = 3;
   IndexedDBPendingConnection connection(
       request,
       callbacks,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       upgrade_transaction_id,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   database->OpenConnection(connection);
   EXPECT_EQ(database, request->connection()->database());
 
   const int64 transaction_id = 123;
   const std::vector<int64> scope;
   database->CreateTransaction(transaction_id,
                               request->connection(),
                               scope,
@@ skipping 43 matching lines @@
   ASSERT_TRUE(s.ok());
   EXPECT_FALSE(backing_store->HasOneRef());  // local and db
 
   scoped_refptr<MockIndexedDBCallbacks> request1(new MockIndexedDBCallbacks());
   scoped_refptr<MockIndexedDBDatabaseCallbacks> callbacks1(
       new MockIndexedDBDatabaseCallbacks());
   const int64 transaction_id1 = 1;
   IndexedDBPendingConnection connection(
       request1,
       callbacks1,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id1,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection);
 
   EXPECT_FALSE(backing_store->HasOneRef());  // local and db
 
   scoped_refptr<MockDeleteCallbacks> request2(new MockDeleteCallbacks());
   db->DeleteDatabase(request2);
 
   EXPECT_TRUE(request2->blocked_called());
   EXPECT_FALSE(backing_store->HasOneRef());  // local and db
 
   db->Close(request1->connection(), true /* forced */);
 
   EXPECT_FALSE(db->backing_store());
   EXPECT_TRUE(backing_store->HasOneRef());  // local
   EXPECT_TRUE(request2->success_called());
 }
 
+void DummyOperation(IndexedDBTransaction* transaction) {
+}
+
+class IndexedDBDatabaseOperationTest : public testing::Test {
+ public:
+  IndexedDBDatabaseOperationTest() : commit_success_(true) {}
+
+  virtual void SetUp() {
+    backing_store_ = new IndexedDBFakeBackingStore();
+    leveldb::Status s;
+    db_ = IndexedDBDatabase::Create(ASCIIToUTF16("db"),
+                                    backing_store_,
+                                    NULL /*factory*/,
+                                    IndexedDBDatabase::Identifier(),
+                                    &s);
+    ASSERT_TRUE(s.ok());
+
+    request_ = new MockIndexedDBCallbacks();
+    callbacks_ = new MockIndexedDBDatabaseCallbacks();
+    const int64 transaction_id = 1;
+    db_->OpenConnection(IndexedDBPendingConnection(
+        request_,
+        callbacks_,
+        kFakeChildProcessId,
+        transaction_id,
+        IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION));
+    EXPECT_EQ(IndexedDBDatabaseMetadata::NO_INT_VERSION,
+              db_->metadata().int_version);
+
+    transaction_ = new IndexedDBTransaction(
+        transaction_id,
+        callbacks_,
+        std::set<int64>() /*scope*/,
+        indexed_db::TRANSACTION_VERSION_CHANGE,
+        db_,
+        new IndexedDBFakeBackingStore::FakeTransaction(commit_success_));
+    db_->TransactionCreated(transaction_);
+
+    // Add a dummy task which takes the place of the VersionChangeOperation
+    // which kicks off the upgrade. This ensures that the transaction has
+    // processed at least one task before the CreateObjectStore call.

[ericu, 2014/05/14 01:08:00]

Why is that desirable?

[jsbell, 2014/05/14 11:05:52]

The previous formulation of abort tasks was WEIRD. In pseudocode they had the
form:

updateMetadata();
schedule(updateBackingStore, revertMetadata);

So the async "undo" task did not actually correspond the async "do" task, it
corresponded to the thing done synchronously before the schedule() call. It
would be clearer as:

updateMetadata();
addAbortTask(revertMetadata); // if we need to abort, undo previous
scheduleTask(updateBackingStore); // asynchronously, do this

But... the guts of IndexedDBTransaction are still wired such that abort tasks
only run iff the transaction was "used", otherwise it's considered a no-op.
That's okay, since (1) the only abort tasks correspond to schema changes, (2)
schema changes are only allowed in version change transactions, and (3) version
change transactions always kick off with a real task.

IMHO that's worth simplifying, but as a different patch.

+    transaction_->ScheduleTask(base::Bind(&DummyOperation));
+  }
+
+  void RunPostedTasks() { message_loop_.RunUntilIdle(); }

[ericu, 2014/05/14 01:08:00]

MessageLoop::RunUntilIdle is deprecated.
All the cool kids use "base::RunLoop().RunUntilIdle();".

[jsbell, 2014/05/14 11:05:52]

Will fix, thanks.

+
+ protected:
+  scoped_refptr<IndexedDBFakeBackingStore> backing_store_;
+  scoped_refptr<IndexedDBDatabase> db_;
+  scoped_refptr<MockIndexedDBCallbacks> request_;
+  scoped_refptr<MockIndexedDBDatabaseCallbacks> callbacks_;
+  scoped_refptr<IndexedDBTransaction> transaction_;
+
+  bool commit_success_;
+
+ private:
+  base::MessageLoop message_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(IndexedDBDatabaseOperationTest);
+};
+
+TEST_F(IndexedDBDatabaseOperationTest, CreateObjectStore) {
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+  const int64 store_id = 1001;
+  db_->CreateObjectStore(transaction_->id(),
+                         store_id,
+                         ASCIIToUTF16("store"),
+                         IndexedDBKeyPath(),
+                         false /*auto_increment*/);
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+  RunPostedTasks();
+  transaction_->Commit();
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+}
+

[ericu, 2014/05/14 01:08:00]

How about a test that captures the create-put-delete sequence that revealed the
DCHECK failure?

[jsbell, 2014/05/14 11:05:52]

Will add one. (I have a blink layout test, but can probably do a unit test too.)

+TEST_F(IndexedDBDatabaseOperationTest, CreateIndex) {
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+  const int64 store_id = 1001;
+  db_->CreateObjectStore(transaction_->id(),
+                         store_id,
+                         ASCIIToUTF16("store"),
+                         IndexedDBKeyPath(),
+                         false /*auto_increment*/);
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+  const int64 index_id = 2002;
+  db_->CreateIndex(transaction_->id(),
+                   store_id,
+                   index_id,
+                   ASCIIToUTF16("index"),
+                   IndexedDBKeyPath(),
+                   false /*unique*/,
+                   false /*multi_entry*/);
+  EXPECT_EQ(
+      1ULL,
+      db_->metadata().object_stores.find(store_id)->second.indexes.size());
+  RunPostedTasks();
+  transaction_->Commit();
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+  EXPECT_EQ(
+      1ULL,
+      db_->metadata().object_stores.find(store_id)->second.indexes.size());
+}
+
+class IndexedDBDatabaseOperationAbortTest
+    : public IndexedDBDatabaseOperationTest {
+ public:
+  IndexedDBDatabaseOperationAbortTest() { commit_success_ = false; }
+};
+
+TEST_F(IndexedDBDatabaseOperationAbortTest, CreateObjectStore) {
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+  const int64 store_id = 1001;
+  db_->CreateObjectStore(transaction_->id(),
+                         store_id,
+                         ASCIIToUTF16("store"),
+                         IndexedDBKeyPath(),
+                         false /*auto_increment*/);
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+  RunPostedTasks();
+  transaction_->Commit();
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+}
+
+TEST_F(IndexedDBDatabaseOperationAbortTest, CreateIndex) {
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+  const int64 store_id = 1001;
+  db_->CreateObjectStore(transaction_->id(),
+                         store_id,
+                         ASCIIToUTF16("store"),
+                         IndexedDBKeyPath(),
+                         false /*auto_increment*/);
+  EXPECT_EQ(1ULL, db_->metadata().object_stores.size());
+  const int64 index_id = 2002;
+  db_->CreateIndex(transaction_->id(),
+                   store_id,
+                   index_id,
+                   ASCIIToUTF16("index"),
+                   IndexedDBKeyPath(),
+                   false /*unique*/,
+                   false /*multi_entry*/);
+  EXPECT_EQ(
+      1ULL,
+      db_->metadata().object_stores.find(store_id)->second.indexes.size());
+  RunPostedTasks();
+  transaction_->Commit();
+  EXPECT_EQ(0ULL, db_->metadata().object_stores.size());
+}
+
 }  // namespace content