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 127 matching lines @@
   DCHECK(m_state == Finished || m_contextStopped);
   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 =
+  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;
 }
 
 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()) {
+    // 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));
+    }
   }
 }
 
 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);
+  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") << ")";
   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 83 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 {

[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:
       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 63 matching lines @@
   for (IDBRequest* request : m_requestList)
     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) {

[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->revertObjectStoreMetadata(oldMetadata);
+    objectStore->revertMetadata(oldMetadata);
   }
-
-  m_database->setMetadata(m_oldDatabaseMetadata);
+  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() {
 #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
+  // 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