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 1775c2106a2be37c6b02fd6a49ca52ff2a79f208..bf6a139b2e47a3a9477acacb1d0e63af3a54898e 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
@@ -127,9 +127,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);
 }
@@ -141,9 +140,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, ExceptionState& exceptionState)
@@ -170,12 +168,18 @@ IDBObjectStore* IDBTransaction::objectStore(const String& name, ExceptionState&
     }
 
     DCHECK(m_database->metadata().objectStores.contains(objectStoreId));
-    const IDBObjectStoreMetadata& objectStoreMetadata = m_database->metadata().objectStores.get(objectStoreId);
+    RefPtr<IDBObjectStoreMetadata> objectStoreMetadata = m_database->metadata().objectStores.get(objectStoreId);
+    DCHECK(objectStoreMetadata.get());
 
-    IDBObjectStore* objectStore = IDBObjectStore::create(objectStoreMetadata, this);
+    IDBObjectStore* objectStore = 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;
 }
 
@@ -184,22 +188,34 @@ void IDBTransaction::objectStoreCreated(const String& name, IDBObjectStore* obje
     DCHECK_NE(m_state, Finished) << "A finished transaction created an object store";
     DCHECK_EQ(m_mode, WebIDBTransactionModeVersionChange) << "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()) {
+        DCHECK(m_database->metadata().objectStores.contains(objectStoreId));

[jsbell, 2016/09/26 22:23:40]

Can you add a comment about what it means when the name is not found?

[pwnall, 2016/09/27 00:12:19]

Done.

Thank you! I think future me will be really grateful to you for this suggestion!

+        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) {
+            // This store was created and deleted in the same 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 {
+            DCHECK(m_oldStoreMetadata.contains(objectStore));
+        }
     }
 }
 
@@ -213,6 +229,27 @@ void IDBTransaction::objectStoreRenamed(const String& oldName, const String& new
     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);

[jsbell, 2016/09/26 22:23:40]

Can you add a comment about what this case means?

[pwnall, 2016/09/27 00:12:19]

Done.
Future me thanks you!

+    if (objectStoreIterator == m_oldStoreMetadata.end())
+        return;
+
+    const IDBObjectStoreMetadata* oldStoreMetadata = objectStoreIterator->value.get();
+    DCHECK(oldStoreMetadata);

[jsbell, 2016/09/26 22:23:40]

Can you add a comment about what this case means?

[pwnall, 2016/09/27 00:12:19]

Done.
Once again, future me thanks you!

+    if (!oldStoreMetadata->indexes.contains(index->id()))
+        return;
+
+    m_deletedIndexes.append(index);
+}
+
 void IDBTransaction::setActive(bool active)
 {
     DCHECK_NE(m_state, Finished) << "A finished transaction tried to setActive(" << (active ? "true" : "false") << ")";
@@ -322,6 +359,11 @@ WebIDBTransactionMode IDBTransaction::stringToMode(const String& modeString)
     return WebIDBTransactionModeReadOnly;
 }
 
+WebIDBDatabase* IDBTransaction::backendDB() const
+{
+    return m_database->backend();
+}
+
 const String& IDBTransaction::mode() const
 {
     switch (m_mode) {
@@ -339,11 +381,6 @@ const String& IDBTransaction::mode() const
     return IndexedDBNames::readonly;
 }
 
-WebIDBDatabase* IDBTransaction::backendDB() const
-{
-    return m_database->backend();
-}
-
 DOMStringList* IDBTransaction::objectStoreNames() const
 {
     if (isVersionChange())
@@ -430,19 +467,36 @@ 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) {
+        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) {
+        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) {
+        IDBIndex* index = static_cast<IDBIndex*>(it);
+        index->objectStore()->revertDeletedIndexMetadata(*index);
+    }
+    for (auto& it: m_deletedObjectStores) {
+        RefPtr<IDBObjectStoreMetadata> oldMedata = static_cast<RefPtr<IDBObjectStoreMetadata>>(it);
+        m_database->revertObjectStoreMetadata(std::move(it));
+    }
+    m_database->setDatabaseMetadata(m_oldDatabaseMetadata);
 }
 
 void IDBTransaction::finished()
@@ -454,17 +508,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

[jsbell, 2016/09/26 22:23:40]

This comment is misleading; they types can be GC'd even without this since there
should be no external references and Oilpan understands cycles. We also
shouldn't refer to the project name ("Oilpan") itself in the comments.

Per comment in the old code, in theory the rest of this method (from here down)
can be deleted now.

[pwnall, 2016/09/27 00:12:19]

Gah, sorry for the poor explanation.

I was thinking of the case where JavaScript holds on to the IDBTransaction
instance for some reason. (Our edge-case tests tend to do that, but I don't
think they're representative.) In that case, I think the IDBTransaction will
hold onto all the IDBObjectStore instances in its m_objectStoreMap (and,
transitively, onto the IDBIndex instances that they have in their
"m_indexMap"s), even if those instances are not referenced by JavaScript.

Do you think this case is worth handling (and explaining)? If not, I'd be happy
to delete code :)

+    // 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