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

content/browser/indexed_db/indexed_db_database_unittest.cc

Index: content/browser/indexed_db/indexed_db_database_unittest.cc
diff --git a/content/browser/indexed_db/indexed_db_database_unittest.cc b/content/browser/indexed_db/indexed_db_database_unittest.cc
index 7feed8201d7ac179ded65f9390f22d8b21885a45..4aaf192dfe488c3719069020c81c0c45da6d2946 100644
--- a/content/browser/indexed_db/indexed_db_database_unittest.cc
+++ b/content/browser/indexed_db/indexed_db_database_unittest.cc
@@ -24,7 +24,7 @@
 using base::ASCIIToUTF16;
 
 namespace {
-const int FAKE_CHILD_PROCESS_ID = 0;
+const int kFakeChildProcessId = 0;
 }
 
 namespace content {
@@ -71,7 +71,7 @@ TEST(IndexedDBDatabaseTest, ConnectionLifecycle) {
   IndexedDBPendingConnection connection1(
       request1,
       callbacks1,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id1,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection1);
@@ -85,7 +85,7 @@ TEST(IndexedDBDatabaseTest, ConnectionLifecycle) {
   IndexedDBPendingConnection connection2(
       request2,
       callbacks2,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id2,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection2);
@@ -129,7 +129,7 @@ TEST(IndexedDBDatabaseTest, ForcedClose) {
   IndexedDBPendingConnection connection(
       request,
       callbacks,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       upgrade_transaction_id,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   database->OpenConnection(connection);
@@ -193,7 +193,7 @@ TEST(IndexedDBDatabaseTest, PendingDelete) {
   IndexedDBPendingConnection connection(
       request1,
       callbacks1,
-      FAKE_CHILD_PROCESS_ID,
+      kFakeChildProcessId,
       transaction_id1,
       IndexedDBDatabaseMetadata::DEFAULT_INT_VERSION);
   db->OpenConnection(connection);
@@ -213,4 +213,152 @@ TEST(IndexedDBDatabaseTest, PendingDelete) {
   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