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

content/browser/indexed_db/indexed_db_database.cc

 // Copyright (c) 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 <math.h>
 #include <set>
 
 #include "base/auto_reset.h"
@@ skipping 259 matching lines @@
   IndexedDBTransaction* transaction = GetTransaction(transaction_id);
   if (!transaction)
     return;
   DCHECK_EQ(transaction->mode(), indexed_db::TRANSACTION_VERSION_CHANGE);
 
   if (ContainsKey(metadata_.object_stores, object_store_id)) {
     DLOG(ERROR) << "Invalid object_store_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,
       key_path,
       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(),
                                         object_store_metadata.id,
                                         object_store_metadata.name,
                                         object_store_metadata.key_path,
                                         object_store_metadata.auto_increment);
   if (!s.ok()) {
     IndexedDBDatabaseError error(
         blink::WebIDBDatabaseExceptionUnknownError,
         ASCIIToUTF16("Internal error creating object store '") +
             object_store_metadata.name + ASCIIToUTF16("'."));
     transaction->Abort(error);
     if (s.IsCorruption())
       factory_->HandleBackingStoreCorruption(backing_store_->origin_url(),
                                              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,
                                           int64 object_store_id) {
   IDB_TRACE("IndexedDBDatabase::DeleteObjectStore");
   IndexedDBTransaction* transaction = GetTransaction(transaction_id);
   if (!transaction)
     return;
   DCHECK_EQ(transaction->mode(), indexed_db::TRANSACTION_VERSION_CHANGE);
 
   if (!ValidateObjectStoreId(object_store_id))
     return;
 
   const IndexedDBObjectStoreMetadata& object_store_metadata =
       metadata_.object_stores[object_store_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,
                                     int64 object_store_id,
                                     int64 index_id,
                                     const base::string16& name,
                                     const IndexedDBKeyPath& key_path,
                                     bool unique,
                                     bool multi_entry) {
   IDB_TRACE("IndexedDBDatabase::CreateIndex");
   IndexedDBTransaction* transaction = GetTransaction(transaction_id);
   if (!transaction)
     return;
   DCHECK_EQ(transaction->mode(), indexed_db::TRANSACTION_VERSION_CHANGE);
 
   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,
                                    index_metadata.id,
                                    index_metadata.name,
                                    index_metadata.key_path,
                                    index_metadata.unique,
                                    index_metadata.multi_entry).ok()) {
     base::string16 error_string =
         ASCIIToUTF16("Internal error creating index '") +
         index_metadata.name + ASCIIToUTF16("'.");
     transaction->Abort(IndexedDBDatabaseError(
         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(
     int64 object_store_id,
     int64 index_id,
     IndexedDBTransaction* transaction) {
   IDB_TRACE("IndexedDBDatabase::CreateIndexAbortOperation");
   DCHECK(!transaction);
   RemoveIndex(object_store_id, index_id);
 }
 
 void IndexedDBDatabase::DeleteIndex(int64 transaction_id,
                                     int64 object_store_id,
                                     int64 index_id) {
   IDB_TRACE("IndexedDBDatabase::DeleteIndex");
   IndexedDBTransaction* transaction = GetTransaction(transaction_id);
   if (!transaction)
     return;
   DCHECK_EQ(transaction->mode(), indexed_db::TRANSACTION_VERSION_CHANGE);
 
   if (!ValidateObjectStoreIdAndIndexId(object_store_id, index_id))
     return;
   const IndexedDBIndexMetadata& index_metadata =
       metadata_.object_stores[object_store_id].indexes[index_id];
 
   transaction->ScheduleTask(
       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(
     int64 object_store_id,
     const IndexedDBIndexMetadata& index_metadata,
     IndexedDBTransaction* transaction) {
   IDB_TRACE("IndexedDBDatabase::DeleteIndexOperation");
   leveldb::Status s =
       backing_store_->DeleteIndex(transaction->BackingStoreTransaction(),
                                   transaction->database()->id(),
                                   object_store_id,
                                   index_metadata.id);
   if (!s.ok()) {
     base::string16 error_string =
         ASCIIToUTF16("Internal error deleting index '") +
         index_metadata.name + ASCIIToUTF16("'.");
     IndexedDBDatabaseError error(blink::WebIDBDatabaseExceptionUnknownError,
                                  error_string);
     transaction->Abort(error);
     if (s.IsCorruption())
       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(
     int64 object_store_id,
     const IndexedDBIndexMetadata& index_metadata,
     IndexedDBTransaction* transaction) {
   IDB_TRACE("IndexedDBDatabase::DeleteIndexAbortOperation");
   DCHECK(!transaction);
   AddIndex(object_store_id, index_metadata, IndexedDBIndexMetadata::kInvalidId);
 }
@@ skipping 296 matching lines @@
       &IndexedDBDatabase::PutOperation, this, base::Passed(&params)));
 }
 
 void IndexedDBDatabase::PutOperation(scoped_ptr<PutOperationParams> params,
                                      IndexedDBTransaction* transaction) {
   IDB_TRACE("IndexedDBDatabase::PutOperation");
   DCHECK_NE(transaction->mode(), indexed_db::TRANSACTION_READ_ONLY);
   bool key_was_generated = false;
 
   DCHECK(metadata_.object_stores.find(params->object_store_id) !=
          metadata_.object_stores.end());

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

Is using [] here the right fallback, or should we instead do a runtime check and
return if we don't find the object store?  What happens with the empty object
store we're creating in release mode in the case that we'd fail the DCHECK?

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

The check is done at Put() time, so with this patch the DCHECK should actually
be valid. But for defense-in-depth, a runtime check is reasonable.

   const IndexedDBObjectStoreMetadata& object_store =
       metadata_.object_stores[params->object_store_id];
   DCHECK(object_store.auto_increment || params->key->IsValid());
 
   scoped_ptr<IndexedDBKey> key;
   if (params->put_mode != IndexedDBDatabase::CURSOR_UPDATE &&
       object_store.auto_increment && !params->key->IsValid()) {
     scoped_ptr<IndexedDBKey> auto_inc_key = GenerateKey(
         backing_store_.get(), transaction, id(), params->object_store_id);
     key_was_generated = true;
@@ skipping 527 matching lines @@
     base::string16 error_string =
         ASCIIToUTF16("Internal error deleting object store '") +
         object_store_metadata.name + ASCIIToUTF16("'.");
     IndexedDBDatabaseError error(blink::WebIDBDatabaseExceptionUnknownError,
                                  error_string);
     transaction->Abort(error);
     if (s.IsCorruption())
       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(
     int64 version,
     scoped_refptr<IndexedDBCallbacks> callbacks,
     scoped_ptr<IndexedDBConnection> connection,
     IndexedDBTransaction* transaction) {
   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(
             "Internal error writing data to stable storage when "
             "updating version."));
     callbacks->OnError(error);
     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));
   callbacks->OnUpgradeNeeded(old_version, connection.Pass(), metadata());
 }
 
 void IndexedDBDatabase::TransactionFinished(IndexedDBTransaction* transaction,
                                             bool committed) {
   DCHECK(transactions_.find(transaction->id()) != transactions_.end());
   DCHECK_EQ(transactions_[transaction->id()], transaction);
@@ skipping 262 matching lines @@
     scoped_ptr<IndexedDBConnection> connection,
     int64 transaction_id,
     int64 requested_version) {
 
   std::vector<int64> object_store_ids;
   CreateTransaction(transaction_id,
                     connection.get(),
                     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_);
 }
 
 void IndexedDBDatabase::DeleteDatabase(
     scoped_refptr<IndexedDBCallbacks> callbacks) {
 
   if (IsDeleteDatabaseBlocked()) {
     for (ConnectionSet::const_iterator it = connections_.begin();
          it != connections_.end();
          ++it) {
@@ skipping 119 matching lines @@
     const base::string16& previous_version,
     int64 previous_int_version,
     IndexedDBTransaction* transaction) {
   IDB_TRACE("IndexedDBDatabase::VersionChangeAbortOperation");
   DCHECK(!transaction);
   metadata_.version = previous_version;
   metadata_.int_version = previous_int_version;
 }
 
 }  // namespace content