Align IndexedDB metadata rollback on transaction abort to spec.

third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.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 37 matching lines @@
 #include "public/platform/modules/indexeddb/WebIDBKeyRange.h"
 #include <memory>
 #include <v8.h>
 
 using blink::WebBlobInfo;
 using blink::WebIDBCallbacks;
 using blink::WebIDBCursor;
 using blink::WebIDBDatabase;
 using blink::WebVector;
 
+

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

Nit: remove extra blank line

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

Done.

 namespace blink {
 
 namespace {
 
 using IndexKeys = HeapVector<Member<IDBKey>>;
-
 }
 
-IDBObjectStore::IDBObjectStore(const IDBObjectStoreMetadata& metadata, IDBTransaction* transaction)
-    : m_metadata(metadata)
+IDBObjectStore::IDBObjectStore(RefPtr<IDBObjectStoreMetadata> metadata, IDBTransaction* transaction)
+    : m_metadata(std::move(metadata))
     , m_transaction(transaction)
 {
     DCHECK(m_transaction);
+    DCHECK(m_metadata.get());
 }
 
 DEFINE_TRACE(IDBObjectStore)
 {
     visitor->trace(m_transaction);
     visitor->trace(m_indexMap);
-    visitor->trace(m_createdIndexes);
 }
 
 void IDBObjectStore::setName(const String& name, ExceptionState& exceptionState)
 {
     if (!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
         return;
 
     IDB_TRACE("IDBObjectStore::setName");
     if (!m_transaction->isVersionChange()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::notVersionChangeTransactionErrorMessage);
@@ skipping 16 matching lines @@
         return;
     if (m_transaction->db()->containsObjectStore(name)) {
         exceptionState.throwDOMException(ConstraintError, IDBDatabase::objectStoreNameTakenErrorMessage);
         return;
     }
     if (!backendDB()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::databaseClosedErrorMessage);
         return;
     }
 
-    backendDB()->renameObjectStore(m_transaction->id(), id(), name);
-    m_transaction->objectStoreRenamed(m_metadata.name, name);
-    m_metadata.name = name;
-
-    // The name inside the database's version of the object store metadata is used by IDBDatabase.objectStoreNames().
-    // If the transaction is aborted, this name will be reverted when the metadata is overwritten with the previousMetadata in IDBTransaction.
-    m_transaction->db()->objectStoreRenamed(id(), name);
+    m_transaction->db()->renameObjectStore(id(), name);
 }
 
 ScriptValue IDBObjectStore::keyPath(ScriptState* scriptState) const
 {
     return ScriptValue::from(scriptState, metadata().keyPath);
 }
 
 DOMStringList* IDBObjectStore::indexNames() const
 {
     IDB_TRACE("IDBObjectStore::indexNames");
     DOMStringList* indexNames = DOMStringList::create(DOMStringList::IndexedDB);
     for (const auto& it : metadata().indexes)
-        indexNames->append(it.value.name);
+        indexNames->append(it.value->name);
     indexNames->sort();
     return indexNames;
 }
 
 IDBRequest* IDBObjectStore::get(ScriptState* scriptState, const ScriptValue& key, ExceptionState& exceptionState)
 {
     IDB_TRACE("IDBObjectStore::get");
     if (isDeleted()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return nullptr;
@@ skipping 261 matching lines @@
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::databaseClosedErrorMessage);
         return nullptr;
     }
 
     Vector<int64_t> indexIds;
     HeapVector<IndexKeys> indexKeys;
     for (const auto& it : metadata().indexes) {
         if (clone.isEmpty())
             clone = deserializeScriptValue(scriptState, serializedValue.get(), &blobInfo);
         IndexKeys keys;
-        generateIndexKeysForValue(scriptState->isolate(), it.value, clone, &keys);
+        generateIndexKeysForValue(scriptState->isolate(), *it.value, clone, &keys);
         indexIds.append(it.key);
         indexKeys.append(keys);
     }
 
     IDBRequest* request = IDBRequest::create(scriptState, source, m_transaction.get());
     Vector<char> wireBytes;
     serializedValue->toWireBytes(wireBytes);
     RefPtr<SharedBuffer> valueBuffer = SharedBuffer::adoptVector(wireBytes);
 
     backendDB()->put(m_transaction->id(), id(), WebData(valueBuffer), blobInfo, key, static_cast<WebIDBPutMode>(putMode), WebIDBCallbacksImpl::create(request).release(), indexIds, indexKeys);
@@ skipping 67 matching lines @@
 }
 
 namespace {
 // This class creates the index keys for a given index by extracting
 // them from the SerializedScriptValue, for all the existing values in
 // the objectStore. It only needs to be kept alive by virtue of being
 // a listener on an IDBRequest object, in the same way that JavaScript
 // cursor success handlers are kept alive.
 class IndexPopulator final : public EventListener {
 public:
-    static IndexPopulator* create(ScriptState* scriptState, IDBDatabase* database, int64_t transactionId, int64_t objectStoreId, const IDBIndexMetadata& indexMetadata)
+    static IndexPopulator* create(ScriptState* scriptState, IDBDatabase* database, int64_t transactionId, int64_t objectStoreId, RefPtr<const IDBIndexMetadata> indexMetadata)
     {
-        return new IndexPopulator(scriptState, database, transactionId, objectStoreId, indexMetadata);
+        return new IndexPopulator(scriptState, database, transactionId, objectStoreId, std::move(indexMetadata));
     }
 
     bool operator==(const EventListener& other) const override
     {
         return this == &other;
     }
 
     DEFINE_INLINE_VIRTUAL_TRACE()
     {
         visitor->trace(m_database);
         EventListener::trace(visitor);
     }
 
 private:
-    IndexPopulator(ScriptState* scriptState, IDBDatabase* database, int64_t transactionId, int64_t objectStoreId, const IDBIndexMetadata& indexMetadata)
+    IndexPopulator(ScriptState* scriptState, IDBDatabase* database, int64_t transactionId, int64_t objectStoreId, RefPtr<const IDBIndexMetadata> indexMetadata)
         : EventListener(CPPEventListenerType)
         , m_scriptState(scriptState)
         , m_database(database)
         , m_transactionId(transactionId)
         , m_objectStoreId(objectStoreId)
-        , m_indexMetadata(indexMetadata)
+        , m_indexMetadata(std::move(indexMetadata))
     {
+        DCHECK(m_indexMetadata.get());
     }
 
-    const IDBIndexMetadata& indexMetadata() const { return m_indexMetadata; }
+    const IDBIndexMetadata& indexMetadata() const { return *m_indexMetadata; }
 
     void handleEvent(ExecutionContext* executionContext, Event* event) override
     {
         DCHECK_EQ(m_scriptState->getExecutionContext(), executionContext);
         DCHECK_EQ(event->type(), EventTypeNames::success);
         EventTarget* target = event->target();
         IDBRequest* request = static_cast<IDBRequest*>(target);
 
         if (!m_database->backend()) // If database is stopped?
             return;
@@ skipping 24 matching lines @@
             m_database->backend()->setIndexesReady(m_transactionId, m_objectStoreId, indexIds);
             m_database.clear();
         }
 
     }
 
     RefPtr<ScriptState> m_scriptState;
     Member<IDBDatabase> m_database;
     const int64_t m_transactionId;
     const int64_t m_objectStoreId;
-    const IDBIndexMetadata m_indexMetadata;
+    RefPtr<const IDBIndexMetadata> m_indexMetadata;
 };
 } // namespace
 
 IDBIndex* IDBObjectStore::createIndex(ScriptState* scriptState, const String& name, const IDBKeyPath& keyPath, const IDBIndexParameters& options, ExceptionState& exceptionState)
 {
     IDB_TRACE("IDBObjectStore::createIndex");
     if (!m_transaction->isVersionChange()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::notVersionChangeTransactionErrorMessage);
         return nullptr;
     }
@@ skipping 19 matching lines @@
     }
     if (keyPath.getType() == IDBKeyPath::ArrayType && options.multiEntry()) {
         exceptionState.throwDOMException(InvalidAccessError, "The keyPath argument was an array and the multiEntry option is true.");
         return nullptr;
     }
     if (!backendDB()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::databaseClosedErrorMessage);
         return nullptr;
     }
 
-    int64_t indexId = m_metadata.maxIndexId + 1;
+    int64_t indexId = m_metadata->maxIndexId + 1;
     DCHECK_NE(indexId, IDBIndexMetadata::InvalidId);
     backendDB()->createIndex(m_transaction->id(), id(), indexId, name, keyPath, options.unique(), options.multiEntry());
 
-    ++m_metadata.maxIndexId;
+    ++m_metadata->maxIndexId;
 
-    IDBIndexMetadata indexMetadata(name, indexId, keyPath, options.unique(), options.multiEntry());
+    RefPtr<IDBIndexMetadata> indexMetadata = adoptRef(new IDBIndexMetadata(
+        name, indexId, keyPath, options.unique(), options.multiEntry()));
     IDBIndex* index = IDBIndex::create(indexMetadata, this, m_transaction.get());
     m_indexMap.set(name, index);
-    m_createdIndexes.add(index);
-    m_metadata.indexes.set(indexId, indexMetadata);
-    m_transaction->db()->indexCreated(id(), indexMetadata);
+    m_metadata->indexes.set(indexId, indexMetadata);
 
     DCHECK(!exceptionState.hadException());
     if (exceptionState.hadException())
         return nullptr;
 
     IDBRequest* indexRequest = openCursor(scriptState, nullptr, WebIDBCursorDirectionNext, WebIDBTaskTypePreemptive);
     indexRequest->preventPropagation();
 
     // This is kept alive by being the success handler of the request, which is in turn kept alive by the owning transaction.
-    IndexPopulator* indexPopulator = IndexPopulator::create(scriptState, transaction()->db(), m_transaction->id(), id(), indexMetadata);
+    IndexPopulator* indexPopulator = IndexPopulator::create(scriptState, transaction()->db(), m_transaction->id(), id(), std::move(indexMetadata));
     indexRequest->setOnsuccess(indexPopulator);
     return index;
 }
 
 IDBIndex* IDBObjectStore::index(const String& name, ExceptionState& exceptionState)
 {
     IDB_TRACE("IDBObjectStore::index");
     if (isDeleted()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return nullptr;
     }
     if (m_transaction->isFinished() || m_transaction->isFinishing()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::transactionFinishedErrorMessage);
         return nullptr;
     }
 
     IDBIndexMap::iterator it = m_indexMap.find(name);
     if (it != m_indexMap.end())
         return it->value;
 
     int64_t indexId = findIndexId(name);
     if (indexId == IDBIndexMetadata::InvalidId) {
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchIndexErrorMessage);
         return nullptr;
     }
 
     DCHECK(metadata().indexes.contains(indexId));
-    const IDBIndexMetadata& indexMetadata = metadata().indexes.get(indexId);
-    IDBIndex* index = IDBIndex::create(indexMetadata, this, m_transaction.get());
+    RefPtr<IDBIndexMetadata> indexMetadata = metadata().indexes.get(indexId);
+    DCHECK(indexMetadata.get());
+    IDBIndex* index = IDBIndex::create(std::move(indexMetadata), this, m_transaction.get());
     m_indexMap.set(name, index);
     return index;
 }
 
 void IDBObjectStore::deleteIndex(const String& name, ExceptionState& exceptionState)
 {
     IDB_TRACE("IDBObjectStore::deleteIndex");
     if (!m_transaction->isVersionChange()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::notVersionChangeTransactionErrorMessage);
         return;
@@ skipping 15 matching lines @@
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchIndexErrorMessage);
         return;
     }
     if (!backendDB()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::databaseClosedErrorMessage);
         return;
     }
 
     backendDB()->deleteIndex(m_transaction->id(), id(), indexId);
 
-    m_metadata.indexes.remove(indexId);
-    m_transaction->db()->indexDeleted(id(), indexId);
+    m_metadata->indexes.remove(indexId);
     IDBIndexMap::iterator it = m_indexMap.find(name);
     if (it != m_indexMap.end()) {
+        m_transaction->indexDeleted(it->value);
         it->value->markDeleted();
         m_indexMap.remove(name);
     }
 }
 
 IDBRequest* IDBObjectStore::openCursor(ScriptState* scriptState, const ScriptValue& range, const String& directionString, ExceptionState& exceptionState)
 {
     IDB_TRACE("IDBObjectStore::openCursor");
     if (isDeleted()) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
@@ skipping 89 matching lines @@
     }
 
     IDBRequest* request = IDBRequest::create(scriptState, IDBAny::create(this), m_transaction.get());
     backendDB()->count(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, keyRange, WebIDBCallbacksImpl::create(request).release());
     return request;
 }
 
 void IDBObjectStore::markDeleted()
 {
     DCHECK(m_transaction->isVersionChange()) << "An object store got deleted outside a versionchange transaction.";
+
     m_deleted = true;
+    m_metadata->indexes.clear();
+
+    for (auto& it : m_indexMap) {
+        IDBIndex* index = it.value;
+        index->markDeleted();
+    }
 }
 
-void IDBObjectStore::abort()
+void IDBObjectStore::clearIndexCache()
 {
-    for (auto& index : m_createdIndexes)
-        index->markDeleted();
+    DCHECK(!m_transaction->isActive() || (isDeleted() && id() > m_transaction->oldMaxObjectStoreId()));
+
+    // There is no harm in having clearIndexCache() happen multiple times for
+    // the same object. We assert that it is called once to uncover potential
+    // object store accounting bugs.
+#if DCHECK_IS_ON()
+    DCHECK(!m_clearIndexCacheCalled);
+    m_clearIndexCacheCalled = true;
+#endif // DCHECK_IS_ON()
+
+    m_indexMap.clear();
 }
 
-void IDBObjectStore::transactionFinished()
+void IDBObjectStore::revertMetadata(RefPtr<IDBObjectStoreMetadata> oldMetadata)
 {
+    DCHECK(m_transaction->isVersionChange());
     DCHECK(!m_transaction->isActive());
+    DCHECK(oldMetadata.get());
+    DCHECK(id() == oldMetadata->id);
 
-    // Break reference cycles.
-    // TODO(jsbell): This can be removed c/o Oilpan.
-    m_indexMap.clear();
-    m_createdIndexes.clear();
+    // Index IDs are allocated sequentially, so we can tell if an index was
+    // created in this transaction by comparing its ID with the object store's
+    // maximum index ID at the time when the transaction was started.
+    const int64_t oldMaxIndexId = oldMetadata->maxIndexId;
+    for (auto& it : m_indexMap) {
+        IDBIndex* index = it.value;
+        const int64_t indexId = index->id();
+
+        if (indexId > oldMaxIndexId) {
+            // The index was created by this transaction. According to the spec,
+            // its metadata will remain as-is. We just need to look up the

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

You can drop the second sentence here ("We just need...") as that describes what
the code does, not why.

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

Done.
Thanks, that's a very good point!

+            // index's IDBIndex instance, and mark it as deleted.
+            DCHECK(!oldMetadata->indexes.contains(indexId));
+            index->markDeleted();
+            continue;
+        }
+
+        // The index was created in a previous transaction. We need to revert
+        // its metadata. The index might have been deleted, so we
+        // unconditionally reset the deletion marker. We could find out if the

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

I think you can drop the alternative here ("We could...")

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

Done.
Sure, as long as one of us remembers why I wrote it like that :D

+        // index was deleted by looking up the index ID in the store's metadata,
+        // but that would be more work than the unconditional reset.
+        DCHECK(oldMetadata->indexes.contains(indexId));
+        RefPtr<IDBIndexMetadata> oldIndexMetadata = oldMetadata->indexes.get(indexId);
+        index->revertMetadata(std::move(oldIndexMetadata));
+    }
+    m_metadata = std::move(oldMetadata);
+
+    // An object store's metadata will only get reverted if the index was in the
+    // database when the versionchange transaction started.
+    m_deleted = false;
 }
 
-void IDBObjectStore::indexRenamed(int64_t indexId, const String& newName)
+void IDBObjectStore::revertDeletedIndexMetadata(IDBIndex& deletedIndex)
+{
+    DCHECK(m_transaction->isVersionChange());
+    DCHECK(!m_transaction->isActive());
+    DCHECK(deletedIndex.objectStore() == this);
+    DCHECK(deletedIndex.isDeleted());
+
+    const int64_t indexId = deletedIndex.id();
+    DCHECK(m_metadata->indexes.contains(indexId)) << "The object store's metadata was not correctly reverted";
+    RefPtr<IDBIndexMetadata> oldIndexMetadata = m_metadata->indexes.get(indexId);
+    deletedIndex.revertMetadata(std::move(oldIndexMetadata));
+}
+
+void IDBObjectStore::renameIndex(int64_t indexId, const String& newName)
 {
     DCHECK(m_transaction->isVersionChange());
     DCHECK(m_transaction->isActive());
 
-    auto metadataIterator = m_metadata.indexes.find(indexId);
-    DCHECK_NE(metadataIterator, m_metadata.indexes.end()) << "Invalid indexId";
-    const String& oldName = metadataIterator->value.name;
+    auto metadataIterator = m_metadata->indexes.find(indexId);
+    DCHECK_NE(metadataIterator, m_metadata->indexes.end()) << "Invalid indexId";
+    const String& oldName = metadataIterator->value->name;
 
     DCHECK(m_indexMap.contains(oldName)) << "The index had to be accessed in order to be renamed.";
     DCHECK(!m_indexMap.contains(newName));
     m_indexMap.set(newName, m_indexMap.take(oldName));
 
-    metadataIterator->value.name = newName;
+    metadataIterator->value->name = newName;
 }
 
 int64_t IDBObjectStore::findIndexId(const String& name) const
 {
     for (const auto& it : metadata().indexes) {
-        if (it.value.name == name) {
+        if (it.value->name == name) {
             DCHECK_NE(it.key, IDBIndexMetadata::InvalidId);
             return it.key;
         }
     }
     return IDBIndexMetadata::InvalidId;
 }
 
 WebIDBDatabase* IDBObjectStore::backendDB() const
 {
     return m_transaction->backendDB();
 }
 
 } // namespace blink