Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(895)

Unified Diff: third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp

Issue 2314933005: Align IndexedDB metadata rollback on transaction abort to spec. (Closed)
Patch Set: Rebased. Created 4 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
diff --git a/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp b/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
index c5dbcc858911bd88419b1c1c5ec0f5247487b25e..c0f6524e7860de3cf8e57fcc49d7803e83b733c7 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
@@ -55,26 +55,26 @@ 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)
@@ -111,13 +111,7 @@ void IDBObjectStore::setName(const String& name, ExceptionState& exceptionState)
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
@@ -130,7 +124,7 @@ 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;
}
@@ -412,7 +406,7 @@ IDBRequest* IDBObjectStore::put(ScriptState* scriptState, WebIDBPutMode putMode,
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);
}
@@ -500,9 +494,9 @@ namespace {
// 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
@@ -517,17 +511,18 @@ public:
}
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
{
@@ -572,7 +567,7 @@ private:
Member<IDBDatabase> m_database;
const int64_t m_transactionId;
const int64_t m_objectStoreId;
- const IDBIndexMetadata m_indexMetadata;
+ RefPtr<const IDBIndexMetadata> m_indexMetadata;
};
} // namespace
@@ -612,18 +607,17 @@ IDBIndex* IDBObjectStore::createIndex(ScriptState* scriptState, const String& na
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())
@@ -633,7 +627,7 @@ IDBIndex* IDBObjectStore::createIndex(ScriptState* scriptState, const String& na
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;
}
@@ -661,8 +655,9 @@ IDBIndex* IDBObjectStore::index(const String& name, ExceptionState& exceptionSta
}
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;
}
@@ -698,10 +693,10 @@ void IDBObjectStore::deleteIndex(const String& name, ExceptionState& exceptionSt
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);
}
@@ -811,45 +806,104 @@ IDBRequest* IDBObjectStore::count(ScriptState* scriptState, const ScriptValue& r
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);
+
+ // 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 ne
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;
+ }
- // Break reference cycles.
- // TODO(jsbell): This can be removed c/o Oilpan.
- m_indexMap.clear();
- m_createdIndexes.clear();
+ // 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 cou
pwnall 2016/09/27 00:12:19 Done. Sure, as long as one of us remembers why I w
+ // 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::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::indexRenamed(int64_t indexId, const String& newName)
+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;
}

Powered by Google App Engine
This is Rietveld 408576698