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

content/browser/indexed_db/indexed_db_database.cc

Index: content/browser/indexed_db/indexed_db_database.cc
diff --git a/content/browser/indexed_db/indexed_db_database.cc b/content/browser/indexed_db/indexed_db_database.cc
index 1d9ccbbc5f273bfda9efabeb901453c50a71a0da..fa6036ea4ec459b9f9bbaf00fc699de734970124 100644
--- a/content/browser/indexed_db/indexed_db_database.cc
+++ b/content/browser/indexed_db/indexed_db_database.cc
@@ -277,6 +277,9 @@ void IndexedDBDatabase::CreateObjectStore(int64 transaction_id,
     return;
   }
 
+  // Store creation is done synchronously, as it may be followed by
+  // index creation (also sync) since preemptive OpenCursor/SetIndexKeys
+  // may follow.
   IndexedDBObjectStoreMetadata object_store_metadata(
       name,
       object_store_id,
@@ -284,21 +287,6 @@ void IndexedDBDatabase::CreateObjectStore(int64 transaction_id,
       auto_increment,
       IndexedDBDatabase::kMinimumIndexId);
 
-  transaction->ScheduleTask(
-      base::Bind(&IndexedDBDatabase::CreateObjectStoreOperation,
-                 this,
-                 object_store_metadata),
-      base::Bind(&IndexedDBDatabase::CreateObjectStoreAbortOperation,
-                 this,
-                 object_store_id));
-
-  AddObjectStore(object_store_metadata, object_store_id);
-}
-
-void IndexedDBDatabase::CreateObjectStoreOperation(
-    const IndexedDBObjectStoreMetadata& object_store_metadata,
-    IndexedDBTransaction* transaction) {
-  IDB_TRACE("IndexedDBDatabase::CreateObjectStoreOperation");
   leveldb::Status s =
       backing_store_->CreateObjectStore(transaction->BackingStoreTransaction(),
                                         transaction->database()->id(),
@@ -317,6 +305,12 @@ void IndexedDBDatabase::CreateObjectStoreOperation(
                                              error);
     return;
   }
+
+  AddObjectStore(object_store_metadata, object_store_id);
+  transaction->ScheduleAbortTask(
+      base::Bind(&IndexedDBDatabase::CreateObjectStoreAbortOperation,
+                 this,
+                 object_store_id));
 }
 
 void IndexedDBDatabase::DeleteObjectStore(int64 transaction_id,
@@ -336,11 +330,7 @@ void IndexedDBDatabase::DeleteObjectStore(int64 transaction_id,
   transaction->ScheduleTask(
       base::Bind(&IndexedDBDatabase::DeleteObjectStoreOperation,

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

This change makes the RemoveObjectStore call async.  Does that mean that one
could accidentally use an objectStore after deleting it, if the race condition
worked out right?  Or do we protect against that elsewhere?

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

That's the key bit of the patch.

Currently, script code can do:

store.put();
db.deleteObjectStore('store');

When this hits the back end, the put() validates the store's ID, then schedules
a PutOperation. Then the deleteObjectStore() *synchronously* deletes it from the
metadata, and schedules backing store cleanup. Then the PutOperation runs -
which only doesn't fail because the code uses `store = metadata.store_map[id]`
which magically generates a new entry in the map (thanks, STL!).

So the fix is to do the 'delete' in the correct order.

'create' is done early, since the indexing runs preemptively.

                  this,
-                 object_store_metadata),
-      base::Bind(&IndexedDBDatabase::DeleteObjectStoreAbortOperation,
-                 this,
                  object_store_metadata));
-  RemoveObjectStore(object_store_id);
 }
 
 void IndexedDBDatabase::CreateIndex(int64 transaction_id,
@@ -358,27 +348,12 @@ void IndexedDBDatabase::CreateIndex(int64 transaction_id,
 
   if (!ValidateObjectStoreIdAndNewIndexId(object_store_id, index_id))
     return;
+
+  // Index creation is done synchronously since preemptive
+  // OpenCursor/SetIndexKeys may follow.
   const IndexedDBIndexMetadata index_metadata(
       name, index_id, key_path, unique, multi_entry);
 
-  transaction->ScheduleTask(
-      base::Bind(&IndexedDBDatabase::CreateIndexOperation,
-                 this,
-                 object_store_id,
-                 index_metadata),
-      base::Bind(&IndexedDBDatabase::CreateIndexAbortOperation,
-                 this,
-                 object_store_id,
-                 index_id));
-
-  AddIndex(object_store_id, index_metadata, index_id);
-}
-
-void IndexedDBDatabase::CreateIndexOperation(
-    int64 object_store_id,
-    const IndexedDBIndexMetadata& index_metadata,
-    IndexedDBTransaction* transaction) {
-  IDB_TRACE("IndexedDBDatabase::CreateIndexOperation");
   if (!backing_store_->CreateIndex(transaction->BackingStoreTransaction(),
                                    transaction->database()->id(),
                                    object_store_id,
@@ -394,6 +369,13 @@ void IndexedDBDatabase::CreateIndexOperation(
         blink::WebIDBDatabaseExceptionUnknownError, error_string));
     return;
   }
+
+  AddIndex(object_store_id, index_metadata, index_id);
+  transaction->ScheduleAbortTask(
+      base::Bind(&IndexedDBDatabase::CreateIndexAbortOperation,
+                 this,
+                 object_store_id,
+                 index_id));
 }
 
 void IndexedDBDatabase::CreateIndexAbortOperation(
@@ -423,13 +405,7 @@ void IndexedDBDatabase::DeleteIndex(int64 transaction_id,
       base::Bind(&IndexedDBDatabase::DeleteIndexOperation,
                  this,
                  object_store_id,
-                 index_metadata),
-      base::Bind(&IndexedDBDatabase::DeleteIndexAbortOperation,
-                 this,
-                 object_store_id,
                  index_metadata));
-
-  RemoveIndex(object_store_id, index_id);
 }
 
 void IndexedDBDatabase::DeleteIndexOperation(
@@ -453,6 +429,13 @@ void IndexedDBDatabase::DeleteIndexOperation(
       factory_->HandleBackingStoreCorruption(backing_store_->origin_url(),
                                              error);
   }
+
+  RemoveIndex(object_store_id, index_metadata.id);
+  transaction->ScheduleAbortTask(
+      base::Bind(&IndexedDBDatabase::DeleteIndexAbortOperation,
+                 this,
+                 object_store_id,
+                 index_metadata));
 }
 
 void IndexedDBDatabase::DeleteIndexAbortOperation(
@@ -1317,6 +1300,12 @@ void IndexedDBDatabase::DeleteObjectStoreOperation(
       factory_->HandleBackingStoreCorruption(backing_store_->origin_url(),
                                              error);
   }
+
+  RemoveObjectStore(object_store_metadata.id);
+  transaction->ScheduleAbortTask(
+      base::Bind(&IndexedDBDatabase::DeleteObjectStoreAbortOperation,
+                 this,
+                 object_store_metadata));
 }
 
 void IndexedDBDatabase::VersionChangeOperation(
@@ -1327,11 +1316,9 @@ void IndexedDBDatabase::VersionChangeOperation(
   IDB_TRACE("IndexedDBDatabase::VersionChangeOperation");
   int64 old_version = metadata_.int_version;
   DCHECK_GT(version, old_version);
-  metadata_.int_version = version;
+
   if (!backing_store_->UpdateIDBDatabaseIntVersion(
-          transaction->BackingStoreTransaction(),
-          id(),
-          metadata_.int_version)) {
+          transaction->BackingStoreTransaction(), id(), version)) {
     IndexedDBDatabaseError error(
         blink::WebIDBDatabaseExceptionUnknownError,
         ASCIIToUTF16(
@@ -1341,6 +1328,15 @@ void IndexedDBDatabase::VersionChangeOperation(
     transaction->Abort(error);
     return;
   }
+
+  transaction->ScheduleAbortTask(
+      base::Bind(&IndexedDBDatabase::VersionChangeAbortOperation,
+                 this,
+                 metadata_.version,
+                 metadata_.int_version));
+  metadata_.int_version = version;
+  metadata_.version = kNoStringVersion;

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

Given that metadata_.version is obsolete, I don't see any docs on what it should
be here.  But this initialization to kNoStringVersion is new--just checking that
that's intentional.

[jsbell, 2014/05/14 11:17:12]

Intentional. It appeared to be missing, possibly a bug in the case where you
were upgrading a string-version to an int-version and it aborted, the metadata
might have not been rewound correctly. 

Maybe I should not touch the versionchange task/abort task stuff in this CL,
just to keep things simpler.

+
   DCHECK(!pending_second_half_open_);
   pending_second_half_open_.reset(
       new PendingSuccessCall(callbacks, connection.get(), version));
@@ -1623,17 +1619,12 @@ void IndexedDBDatabase::RunVersionChangeTransactionFinal(
                     object_store_ids,
                     indexed_db::TRANSACTION_VERSION_CHANGE);
 
-  transactions_[transaction_id]
-      ->ScheduleTask(base::Bind(&IndexedDBDatabase::VersionChangeOperation,
-                                this,
-                                requested_version,
-                                callbacks,
-                                base::Passed(&connection)),
-                     base::Bind(&IndexedDBDatabase::VersionChangeAbortOperation,
-                                this,
-                                metadata_.version,
-                                metadata_.int_version));
-
+  transactions_[transaction_id]->ScheduleTask(
+      base::Bind(&IndexedDBDatabase::VersionChangeOperation,
+                 this,
+                 requested_version,
+                 callbacks,
+                 base::Passed(&connection)));
   DCHECK(!pending_second_half_open_);
 }