Align IndexedDB metadata rollback on transaction abort to spec.

third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp

Index: third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
diff --git a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
index 4b6d7e00202ab5ace9f25310e2ca83c7442fa287..38a23eaf4fc8ba5be6dbd5669c4d8c763020308d 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
@@ -145,9 +145,8 @@ DEFINE_TRACE(IDBTransaction) {
   visitor->trace(m_error);
   visitor->trace(m_requestList);
   visitor->trace(m_objectStoreMap);
-  visitor->trace(m_createdObjectStores);
-  visitor->trace(m_deletedObjectStores);
-  visitor->trace(m_objectStoreCleanupMap);
+  visitor->trace(m_oldStoreMetadata);
+  visitor->trace(m_deletedIndexes);
   EventTargetWithInlineData::trace(visitor);
   ActiveDOMObject::trace(visitor);
 }
@@ -158,9 +157,8 @@ void IDBTransaction::setError(DOMException* error) {
 
   // The first error to be set is the true cause of the
   // transaction abort.
-  if (!m_error) {
+  if (!m_error)
     m_error = error;
-  }
 }
 
 IDBObjectStore* IDBTransaction::objectStore(const String& name,
@@ -190,14 +188,22 @@ IDBObjectStore* IDBTransaction::objectStore(const String& name,
   }
 
   DCHECK(m_database->metadata().objectStores.contains(objectStoreId));
-  const IDBObjectStoreMetadata& objectStoreMetadata =
+  RefPtr<IDBObjectStoreMetadata> objectStoreMetadata =
       m_database->metadata().objectStores.get(objectStoreId);
+  DCHECK(objectStoreMetadata.get());
 
   IDBObjectStore* objectStore =
-      IDBObjectStore::create(objectStoreMetadata, this);
+      IDBObjectStore::create(std::move(objectStoreMetadata), this);
   DCHECK(!m_objectStoreMap.contains(name));
   m_objectStoreMap.set(name, objectStore);
-  m_objectStoreCleanupMap.set(objectStore, objectStore->metadata());
+
+  if (isVersionChange()) {
+    DCHECK(objectStore->id() <= oldMaxObjectStoreId())
+        << "Object store IDs are not assigned sequentially";
+    RefPtr<IDBObjectStoreMetadata> backupMetadata =
+        objectStore->metadata().createCopy();
+    m_oldStoreMetadata.set(objectStore, std::move(backupMetadata));
+  }
   return objectStore;
 }
 
@@ -209,23 +215,44 @@ void IDBTransaction::objectStoreCreated(const String& name,
       << "A non-versionchange transaction created an object store";
   DCHECK(!m_objectStoreMap.contains(name))
       << "An object store was created with the name of an existing store";
+  DCHECK(objectStore->id() > oldMaxObjectStoreId())
+      << "Object store IDs are not assigned sequentially";
   m_objectStoreMap.set(name, objectStore);
-  m_objectStoreCleanupMap.set(objectStore, objectStore->metadata());
-  m_createdObjectStores.add(objectStore);
 }
 
-void IDBTransaction::objectStoreDeleted(const String& name) {
+void IDBTransaction::objectStoreDeleted(const int64_t objectStoreId,
+                                        const String& name) {
   DCHECK_NE(m_state, Finished)
       << "A finished transaction deleted an object store";
   DCHECK_EQ(m_mode, WebIDBTransactionModeVersionChange)
       << "A non-versionchange transaction deleted an object store";
   IDBObjectStoreMap::iterator it = m_objectStoreMap.find(name);
-  if (it != m_objectStoreMap.end()) {
+  if (it == m_objectStoreMap.end()) {
+    // No IDBObjectStore instance was created for the deleted store in this
+    // transaction. We only need to be able to revert the metadata change
+    // if the transaction aborts.
+    DCHECK(m_database->metadata().objectStores.contains(objectStoreId));
+    RefPtr<IDBObjectStoreMetadata> metadata =
+        m_database->metadata().objectStores.get(objectStoreId);
+    DCHECK(metadata.get());
+    DCHECK_EQ(metadata->name, name);
+    m_deletedObjectStores.append(std::move(metadata));
+  } else {
     IDBObjectStore* objectStore = it->value;
     m_objectStoreMap.remove(name);
     objectStore->markDeleted();
-    m_objectStoreCleanupMap.set(objectStore, objectStore->metadata());
-    m_deletedObjectStores.add(objectStore);
+    if (objectStore->id() > m_oldDatabaseMetadata.maxObjectStoreId) {
+      // The store was created and deleted in this transaction, so it will
+      // not be restored even if the transaction aborts. We have just
+      // removed our last reference to it.
+      DCHECK(!m_oldStoreMetadata.contains(objectStore));
+      objectStore->clearIndexCache();
+    } else {
+      // The store was created before this transaction, and we created an
+      // IDBObjectStore instance for it. When that happened, we must have
+      // snapshotted the store's metadata as well.
+      DCHECK(m_oldStoreMetadata.contains(objectStore));
+    }
   }
 }
 
@@ -242,6 +269,43 @@ void IDBTransaction::objectStoreRenamed(const String& oldName,
   m_objectStoreMap.set(newName, m_objectStoreMap.take(oldName));
 }
 
+void IDBTransaction::indexDeleted(IDBIndex* index) {
+  DCHECK(index);
+  DCHECK(!index->isDeleted()) << "indexDeleted called twice for the same index";
+
+  IDBObjectStore* objectStore = index->objectStore();
+  DCHECK_EQ(objectStore->transaction(), this);
+  DCHECK(m_objectStoreMap.contains(objectStore->name()))
+      << "An index was deleted without accessing its object store";
+
+  const auto& objectStoreIterator = m_oldStoreMetadata.find(objectStore);
+  if (objectStoreIterator == m_oldStoreMetadata.end()) {
+    // The index's object store was created in this transaction, so this
+    // index was also created (and deleted) in this transaction, and will
+    // not be restored if the transaction aborts.
+    //
+    // Subtle proof for the first sentence above: Deleting an index requires
+    // calling deleteIndex() on the store's IDBObjectStore instance.
+    // Whenever we create an IDBObjectStore instance for a previously
+    // created store, we snapshot the store's metadata. So, deleting an
+    // index of an "old" store can only be done after the store's metadata
+    // is snapshotted.
+    return;
+  }
+
+  const IDBObjectStoreMetadata* oldStoreMetadata =
+      objectStoreIterator->value.get();
+  DCHECK(oldStoreMetadata);
+  if (!oldStoreMetadata->indexes.contains(index->id())) {
+    // The index's object store was created before this transaction, but the
+    // index was created (and deleted) in this transaction, so it will not
+    // be restored if the transaction aborts.
+    return;
+  }
+
+  m_deletedIndexes.append(index);
+}
+
 void IDBTransaction::setActive(bool active) {
   DCHECK_NE(m_state, Finished) << "A finished transaction tried to setActive("
                                << (active ? "true" : "false") << ")";
@@ -345,6 +409,10 @@ WebIDBTransactionMode IDBTransaction::stringToMode(const String& modeString) {
   return WebIDBTransactionModeReadOnly;
 }
 
+WebIDBDatabase* IDBTransaction::backendDB() const {

[cmumford, 2016/10/05 21:15:27]

Why move method within file?

[pwnall, 2016/10/05 23:15:45]

I moved it to match the order in the header -- I'm guessing the method became
public at some point, which caused it to move in the header, and the move wasn't
matched in the .cpp file.

If we don't care about the ordering, I can move it back. WDYT?

[jsbell, 2016/10/06 20:01:31]

I'd leave it alone for this CL since it's untouched, do cleanup in a separate CL
just to minimize the diffs.

[cmumford, 2016/10/06 20:22:55]

What Josh said, plus moving it also makes it less likely that "git blame" will
tell you who added the function and why.

+  return m_database->backend();
+}
+
 const String& IDBTransaction::mode() const {
   switch (m_mode) {
     case WebIDBTransactionModeReadOnly:
@@ -361,10 +429,6 @@ const String& IDBTransaction::mode() const {
   return IndexedDBNames::readonly;
 }
 
-WebIDBDatabase* IDBTransaction::backendDB() const {
-  return m_database->backend();
-}
-
 DOMStringList* IDBTransaction::objectStoreNames() const {
   if (isVersionChange())
     return m_database->objectStoreNames();
@@ -448,19 +512,37 @@ void IDBTransaction::revertDatabaseMetadata() {
   if (!isVersionChange())
     return;
 
-  // Newly created stores must be marked as deleted.
-  for (IDBObjectStore* store : m_createdObjectStores) {
-    store->abort();
-    store->markDeleted();
+  // Mark stores created by this transaction as deleted.
+  for (auto& it : m_objectStoreMap) {

[cmumford, 2016/10/05 21:15:27]

for (auto& objectStore : m_objectStoreMap.values()) {

[pwnall, 2016/10/05 23:15:45]

Done.
Cool, thank you for teaching me this!

+    IDBObjectStore* objectStore = it.value;
+    const int64_t objectStoreId = objectStore->id();
+    if (objectStoreId <= oldMaxObjectStoreId()) {
+      DCHECK(m_oldStoreMetadata.contains(objectStore));
+      continue;
+    }
+
+    DCHECK(!m_oldStoreMetadata.contains(objectStore));
+    m_database->revertObjectStoreCreation(objectStoreId);
+    objectStore->markDeleted();
   }
 
-  // Used stores may need to mark indexes as deleted.
-  for (auto& it : m_objectStoreCleanupMap) {
-    it.key->abort();
-    it.key->setMetadata(it.value);
-  }
+  for (auto& it : m_oldStoreMetadata) {

[cmumford, 2016/10/05 21:15:27]

for (auto& objectStore : m_oldStoreMetadata.keys()) {

[pwnall, 2016/10/05 23:15:45]

I need the value too, though -- I'm saving it as oldMetadata two lines below. I
think (but don't know for sure) that using HashMap::get() to get the value for
each key is more expensive than extracting the keys and values from the
iterator.

WDYT?

[jsbell, 2016/10/06 20:01:31]

Seems fine to me.

[cmumford, 2016/10/06 20:22:55]

Yeah, if you need both key and value then absolutely stick with the iterator and
don't call get().

+    IDBObjectStore* objectStore = it.key;
+    RefPtr<IDBObjectStoreMetadata> oldMetadata = it.value;
 
-  m_database->setMetadata(m_oldDatabaseMetadata);
+    m_database->revertObjectStoreMetadata(oldMetadata);
+    objectStore->revertMetadata(oldMetadata);
+  }
+  for (auto& it : m_deletedIndexes) {

[cmumford, 2016/10/05 21:15:27]

Why not just?

  for (auto& index : m_deletedIndexes) {

[pwnall, 2016/10/05 23:15:45]

Done.

Thank you for catching this!

+    IDBIndex* index = static_cast<IDBIndex*>(it);
+    index->objectStore()->revertDeletedIndexMetadata(*index);
+  }
+  for (auto& it : m_deletedObjectStores) {
+    RefPtr<IDBObjectStoreMetadata> oldMedata =

[cmumford, 2016/10/05 21:15:27]

Do you need oldMetadata?

[pwnall, 2016/10/05 23:15:45]

Done.
Derp, I didn't.

I thought we'd have a compiler warning for unused variables, so I wouldn't look
dumb?
Thanks for catching this!

[jsbell, 2016/10/06 20:01:31]

cpplint.py may be handy - not run by default; not sure if it would have caught
this. Also, need to ignore several things due to blink style/legacy code.

+        static_cast<RefPtr<IDBObjectStoreMetadata>>(it);
+    m_database->revertObjectStoreMetadata(std::move(it));
+  }
+  m_database->setDatabaseMetadata(m_oldDatabaseMetadata);
 }
 
 void IDBTransaction::finished() {
@@ -471,17 +553,30 @@ void IDBTransaction::finished() {
 
   m_database->transactionFinished(this);
 
-  // Break reference cycles.
-  // TODO(jsbell): This can be removed c/o Oilpan.
-  for (auto& it : m_objectStoreMap)
-    it.value->transactionFinished();
+  // Remove references to the IDBObjectStore and IDBIndex instances held by
+  // this transaction, so OilPan can garbage-collect the instances that aren't
+  // used by JavaScript.
+
+  for (auto& it : m_objectStoreMap) {
+    IDBObjectStore* objectStore = it.value;
+    if (!isVersionChange() || objectStore->id() > oldMaxObjectStoreId()) {
+      DCHECK(!m_oldStoreMetadata.contains(objectStore));
+      objectStore->clearIndexCache();
+    } else {
+      // We'll call clearIndexCache() on this store in the loop below.
+      DCHECK(m_oldStoreMetadata.contains(objectStore));
+    }
+  }
   m_objectStoreMap.clear();
-  for (auto& it : m_deletedObjectStores)
-    it->transactionFinished();
-  m_createdObjectStores.clear();
-  m_deletedObjectStores.clear();
 
-  m_objectStoreCleanupMap.clear();
+  for (auto& it : m_oldStoreMetadata) {
+    IDBObjectStore* objectStore = it.key;
+    objectStore->clearIndexCache();
+  }
+  m_oldStoreMetadata.clear();
+
+  m_deletedIndexes.clear();
+  m_deletedObjectStores.clear();
 }
 
 }  // namespace blink