Align IndexedDB metadata rollback on transaction abort to spec.

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

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 109 matching lines @@
     DCHECK(m_requestList.isEmpty() || m_contextStopped);
 }
 
 DEFINE_TRACE(IDBTransaction)
 {
     visitor->trace(m_database);
     visitor->trace(m_openDBRequest);
     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);
 }
 
 void IDBTransaction::setError(DOMException* error)
 {
     DCHECK_NE(m_state, Finished);
     DCHECK(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)
 {
     if (isFinished()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::transactionFinishedErrorMessage);
         return nullptr;
     }
 
     IDBObjectStoreMap::iterator it = m_objectStoreMap.find(name);
     if (it != m_objectStoreMap.end())
         return it->value;
 
     if (!isVersionChange() && !m_scope.contains(name)) {
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchObjectStoreErrorMessage);
         return nullptr;
     }
 
     int64_t objectStoreId = m_database->findObjectStoreId(name);
     if (objectStoreId == IDBObjectStoreMetadata::InvalidId) {
         DCHECK(isVersionChange());
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchObjectStoreErrorMessage);
         return nullptr;
     }
 
     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;
 }
 
 void IDBTransaction::objectStoreCreated(const String& name, IDBObjectStore* objectStore)
 {
     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));
+        }
     }
 }
 
 void IDBTransaction::objectStoreRenamed(const String& oldName, const String& newName)
 {
     DCHECK_NE(m_state, Finished) << "A finished transaction renamed an object store";
     DCHECK_EQ(m_mode, WebIDBTransactionModeVersionChange) << "A non-versionchange transaction renamed an object store";
 
     DCHECK(!m_objectStoreMap.contains(newName));
     DCHECK(m_objectStoreMap.contains(oldName)) << "The object store had to be accessed in order to be renamed.";
     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") << ")";
     if (m_state == Finishing)
         return;
     DCHECK_NE(active, (m_state == Active));
     m_state = active ? Active : Inactive;
 
     if (!active && m_requestList.isEmpty() && backendDB())
         backendDB()->commit(m_id);
@@ skipping 89 matching lines @@
     if (modeString == IndexedDBNames::readonly)
         return WebIDBTransactionModeReadOnly;
     if (modeString == IndexedDBNames::readwrite)
         return WebIDBTransactionModeReadWrite;
     if (modeString == IndexedDBNames::versionchange)
         return WebIDBTransactionModeVersionChange;
     NOTREACHED();
     return WebIDBTransactionModeReadOnly;
 }
 
+WebIDBDatabase* IDBTransaction::backendDB() const
+{
+    return m_database->backend();
+}
+
 const String& IDBTransaction::mode() const
 {
     switch (m_mode) {
     case WebIDBTransactionModeReadOnly:
         return IndexedDBNames::readonly;
 
     case WebIDBTransactionModeReadWrite:
         return IndexedDBNames::readwrite;
 
     case WebIDBTransactionModeVersionChange:
         return IndexedDBNames::versionchange;
     }
 
     NOTREACHED();
     return IndexedDBNames::readonly;
 }
 
-WebIDBDatabase* IDBTransaction::backendDB() const
-{
-    return m_database->backend();
-}
-
 DOMStringList* IDBTransaction::objectStoreNames() const
 {
     if (isVersionChange())
         return m_database->objectStoreNames();
 
     DOMStringList* objectStoreNames = DOMStringList::create(DOMStringList::IndexedDB);
     for (const String& objectStoreName : m_scope)
         objectStoreNames->append(objectStoreName);
     objectStoreNames->sort();
     return objectStoreNames;
@@ skipping 66 matching lines @@
         request->abort();
     m_requestList.clear();
 }
 
 void IDBTransaction::revertDatabaseMetadata()
 {
     DCHECK_NE(m_state, Active);
     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->revertObjectStoreMetadata(oldMetadata);
+        objectStore->revertMetadata(oldMetadata);
     }
-
-    m_database->setMetadata(m_oldDatabaseMetadata);
+    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()
 {
 #if DCHECK_IS_ON()
     DCHECK(!m_finishCalled);
     m_finishCalled = true;
 #endif // DCHECK_IS_ON()
 
     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();
+
+    for (auto& it : m_oldStoreMetadata) {
+        IDBObjectStore* objectStore = it.key;
+        objectStore->clearIndexCache();
+    }
+    m_oldStoreMetadata.clear();
+
+    m_deletedIndexes.clear();
     m_deletedObjectStores.clear();
-
-    m_objectStoreCleanupMap.clear();
 }
 
 } // namespace blink