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

Unified Diff: third_party/WebKit/Source/modules/indexeddb/IDBTransaction.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/IDBTransaction.cpp
diff --git a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
index 71ccf3d4f7fa6e7cf935eddf98135f78e28983c8..1d1ad267e0128ee4d62c98e0422057b4d893762b 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
@@ -46,17 +46,19 @@ using blink::WebIDBDatabase;
namespace blink {
-IDBTransaction* IDBTransaction::create(ScriptState* scriptState, int64_t id, const HashSet<String>& objectStoreNames, WebIDBTransactionMode mode, IDBDatabase* db)
+IDBTransaction* IDBTransaction::createNonVersionChange(ScriptState* scriptState, int64_t id, const HashSet<String>& objectStoreNames, WebIDBTransactionMode mode, IDBDatabase* db)
{
+ DCHECK_NE(mode, WebIDBTransactionModeVersionChange);
+ DCHECK(!objectStoreNames.isEmpty()) << "Non-version transactions should operate on a well-defined set of stores";
IDBOpenDBRequest* openDBRequest = nullptr;
- IDBTransaction* transaction = new IDBTransaction(scriptState, id, objectStoreNames, mode, db, openDBRequest, IDBDatabaseMetadata());
+ IDBTransaction* transaction = new IDBTransaction(scriptState, id, objectStoreNames, mode, db, openDBRequest, IDBDatabaseOwnMetadata());
transaction->suspendIfNeeded();
return transaction;
}
-IDBTransaction* IDBTransaction::create(ScriptState* scriptState, int64_t id, IDBDatabase* db, IDBOpenDBRequest* openDBRequest, const IDBDatabaseMetadata& previousMetadata)
+IDBTransaction* IDBTransaction::createVersionChange(ScriptState* scriptState, int64_t id, IDBDatabase* db, IDBOpenDBRequest* openDBRequest, const IDBDatabaseOwnMetadata& oldMetadata)
{
- IDBTransaction* transaction = new IDBTransaction(scriptState, id, HashSet<String>(), WebIDBTransactionModeVersionChange, db, openDBRequest, previousMetadata);
+ IDBTransaction* transaction = new IDBTransaction(scriptState, id, HashSet<String>(), WebIDBTransactionModeVersionChange, db, openDBRequest, oldMetadata);
transaction->suspendIfNeeded();
return transaction;
}
@@ -85,19 +87,23 @@ private:
} // namespace
-IDBTransaction::IDBTransaction(ScriptState* scriptState, int64_t id, const HashSet<String>& objectStoreNames, WebIDBTransactionMode mode, IDBDatabase* db, IDBOpenDBRequest* openDBRequest, const IDBDatabaseMetadata& previousMetadata)
+IDBTransaction::IDBTransaction(ScriptState* scriptState, int64_t id, const HashSet<String>& objectStoreNames, WebIDBTransactionMode mode, IDBDatabase* db, IDBOpenDBRequest* openDBRequest, const IDBDatabaseOwnMetadata& oldMetadata)
: ActiveScriptWrappable(this)
, ActiveDOMObject(scriptState->getExecutionContext())
, m_id(id)
, m_database(db)
- , m_objectStoreNames(objectStoreNames)
+ , m_scope(objectStoreNames)
, m_openDBRequest(openDBRequest)
, m_mode(mode)
- , m_previousMetadata(previousMetadata)
+ , m_oldDatabaseMetadata(oldMetadata)
{
- if (mode == WebIDBTransactionModeVersionChange) {
+ if (isVersionChange()) {
+ DCHECK(objectStoreNames.isEmpty());
+
// Not active until the callback.
m_state = Inactive;
+ } else {
+ DCHECK(!objectStoreNames.isEmpty()) << "Non-versionchange transactions must operate on a well-defined set of stores";
}
if (m_state == Active)
@@ -118,28 +124,26 @@ 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);
}
void IDBTransaction::setError(DOMException* error)
{
- ASSERT(m_state != Finished);
- ASSERT(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 (m_state == Finished) {
+ if (isFinished()) {
exceptionState.throwDOMException(InvalidStateError, IDBDatabase::transactionFinishedErrorMessage);
return nullptr;
}
@@ -148,7 +152,7 @@ IDBObjectStore* IDBTransaction::objectStore(const String& name, ExceptionState&
if (it != m_objectStoreMap.end())
return it->value;
- if (!isVersionChange() && !m_objectStoreNames.contains(name)) {
+ if (!isVersionChange() && !m_scope.contains(name)) {
exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchObjectStoreErrorMessage);
return nullptr;
}
@@ -162,46 +166,75 @@ IDBObjectStore* IDBTransaction::objectStore(const String& name, ExceptionState&
const IDBDatabaseMetadata& metadata = m_database->metadata();
- IDBObjectStore* objectStore = IDBObjectStore::create(metadata.objectStores.get(objectStoreId), this);
+ RefPtr<IDBObjectStoreMetadata> objectStoreMetadata = metadata.objectStores.get(objectStoreId);
+ DCHECK(objectStoreMetadata.get());
+ 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()) {
+ RefPtr<IDBObjectStoreMetadata> backupMetadata = objectStore->metadata().createCopy();
+ m_oldStoreMetadata.set(objectStore, std::move(backupMetadata));
+ }
return objectStore;
}
void IDBTransaction::objectStoreCreated(const String& name, IDBObjectStore* objectStore)
{
- ASSERT(m_state != Finished);
- ASSERT(isVersionChange());
+ 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";
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)
{
- ASSERT(m_state != Finished);
- ASSERT(isVersionChange());
+ 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));
+ RefPtr<IDBObjectStoreMetadata> metadata = m_database->metadata().objectStores.get(objectStoreId);
+ DCHECK(metadata.get());
+ DCHECK(metadata->own.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);
}
}
void IDBTransaction::objectStoreRenamed(const String& oldName, const String& newName)
{
- DCHECK(m_state != Finished);
- DCHECK(isVersionChange());
+ 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.";
+ IDBObjectStore* objectStore = m_objectStoreMap.take(oldName);
+ m_objectStoreMap.set(newName, objectStore);
+}
+
+void IDBTransaction::indexDeleted(IDBIndex* index)
+{
+ DCHECK(index);
+ DCHECK(!index->isDeleted()) << "indexDeleted called twice for the same index";
- IDBObjectStoreMap::iterator it = m_objectStoreMap.find(oldName);
- m_objectStoreMap.set(newName, it->value);
- m_objectStoreMap.remove(oldName);
+ IDBObjectStore* objectStore = index->objectStore();
+ DCHECK(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())
+ return;
+
+ const IDBObjectStoreMetadata* oldStoreMetadata = objectStoreIterator->value.get();
+ DCHECK(oldStoreMetadata);
+ if (!oldStoreMetadata->indexes.contains(index->id()))
+ return;
+
+ m_deletedIndexes.append(index);
}
void IDBTransaction::setActive(bool active)
@@ -228,14 +261,8 @@ void IDBTransaction::abort(ExceptionState& exceptionState)
if (m_contextStopped)
return;
- for (IDBRequest* request : m_requestList)
- request->abort();
- m_requestList.clear();
-
- for (IDBObjectStore* store : m_createdObjectStores) {
- store->abort();
- store->markDeleted();
- }
+ abortOutstandingRequests();
+ revertDatabaseMetadata();
if (backendDB())
backendDB()->abort(m_id);
@@ -259,7 +286,7 @@ void IDBTransaction::onAbort(DOMException* error)
{
IDB_TRACE("IDBTransaction::onAbort");
if (m_contextStopped) {
- m_database->transactionFinished(this);
+ finish();
return;
}
@@ -269,52 +296,34 @@ void IDBTransaction::onAbort(DOMException* error)
ASSERT(error);
setError(error);
- // Outstanding requests must be aborted.
- for (IDBRequest* request : m_requestList)
- request->abort();
- m_requestList.clear();
-
- // Newly created stores must be marked as deleted.
- for (IDBObjectStore* store : m_createdObjectStores)
- store->markDeleted();
-
- // Used stores may need to mark indexes as deleted.
- for (auto& it : m_objectStoreCleanupMap)
- it.key->abort();
+ abortOutstandingRequests();
+ revertDatabaseMetadata();
m_state = Finishing;
}
- if (isVersionChange()) {
- for (auto& it : m_objectStoreCleanupMap)
- it.key->setMetadata(it.value);
- m_database->setMetadata(m_previousMetadata);
+ if (isVersionChange())
m_database->close();
- }
- m_objectStoreCleanupMap.clear();
// Enqueue events before notifying database, as database may close which enqueues more events and order matters.
enqueueEvent(Event::createBubble(EventTypeNames::abort));
-
- m_database->transactionFinished(this);
+ finish();
}
void IDBTransaction::onComplete()
{
IDB_TRACE("IDBTransaction::onComplete");
if (m_contextStopped) {
- m_database->transactionFinished(this);
+ finish();
return;
}
ASSERT(m_state != Finished);
m_state = Finishing;
- m_objectStoreCleanupMap.clear();
// Enqueue events before notifying database, as database may close which enqueues more events and order matters.
enqueueEvent(Event::create(EventTypeNames::complete));
-
- m_database->transactionFinished(this);
+ finish();
}
bool IDBTransaction::hasPendingActivity() const
@@ -337,6 +346,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) {
@@ -356,12 +370,12 @@ const String& IDBTransaction::mode() const
DOMStringList* IDBTransaction::objectStoreNames() const
{
- if (m_mode == WebIDBTransactionModeVersionChange)
+ if (isVersionChange())
return m_database->objectStoreNames();
DOMStringList* objectStoreNames = DOMStringList::create(DOMStringList::IndexedDB);
- for (const String& name : m_objectStoreNames)
- objectStoreNames->append(name);
+ for (const String& objectStoreName : m_scope)
+ objectStoreNames->append(objectStoreName);
objectStoreNames->sort();
return objectStoreNames;
}
@@ -389,16 +403,6 @@ DispatchEventResult IDBTransaction::dispatchEventInternal(Event* event)
ASSERT(event->target() == this);
m_state = Finished;
- // Break reference cycles.
- // TODO(jsbell): This can be removed c/o Oilpan.
- for (auto& it : m_objectStoreMap)
- it.value->transactionFinished();
- m_objectStoreMap.clear();
- for (auto& it : m_deletedObjectStores)
- it->transactionFinished();
- m_createdObjectStores.clear();
- m_deletedObjectStores.clear();
-
HeapVector<Member<EventTarget>> targets;
targets.append(this);
targets.append(db());
@@ -437,9 +441,64 @@ void IDBTransaction::enqueueEvent(Event* event)
eventQueue->enqueueEvent(event);
}
-WebIDBDatabase* IDBTransaction::backendDB() const
+void IDBTransaction::abortOutstandingRequests()
{
- return m_database->backend();
+ for (IDBRequest* request : m_requestList)
+ request->abort();
+ m_requestList.clear();
+}
+
+void IDBTransaction::revertDatabaseMetadata()
+{
+ DCHECK_NE(m_state, Active);
+ if (!isVersionChange())
+ return;
+
+ // Mark stores created by this transaction as deleted.
+ // Object store IDs are allocated sequentially, so we can tell if an object
+ // store was created in this transaction by comparing its ID against the
+ // database's maximum object store ID at the time when the transaction was
+ // started.
+ const int64_t oldMaxObjectStoreId = m_oldDatabaseMetadata.maxObjectStoreId;
+ for (auto& it : m_objectStoreMap) {
+ IDBObjectStore* objectStore = it.value;
+ const int64_t objectStoreId = objectStore->id();
+ if (objectStoreId < oldMaxObjectStoreId)
+ continue;
+
+ m_database->revertObjectStoreCreation(objectStoreId);
+ objectStore->markDeleted();
+ }
+
+ for (auto& it : m_oldStoreMetadata) {
+ IDBObjectStore* objectStore = it.key;
+ RefPtr<IDBObjectStoreMetadata> oldMetadata = it.value;
+
+ 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->setOwnMetadata(m_oldDatabaseMetadata);
+}
+
+void IDBTransaction::finish()
+{
+ m_database->transactionFinished(this);
+
+ // 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.
+ m_objectStoreMap.clear();
+ m_oldStoreMetadata.clear();
+ m_deletedIndexes.clear();
+ m_deletedObjectStores.clear();
}
} // namespace blink

Powered by Google App Engine
This is Rietveld 408576698