Minor IndexedDB refactorings.

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 28 matching lines @@
 #include "modules/indexeddb/IDBObjectStore.h"
 #include "modules/indexeddb/IDBOpenDBRequest.h"
 #include "modules/indexeddb/IDBTracing.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 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";

[cmumford, 2016/09/19 23:26:48]

Add a space before 'Non'

[pwnall, 2016/09/20 09:11:14]

I apologize in advance for being clueless, but can you please explain why I need
a space here?

Based on sampling, the codebase doesn't seem to use space.
https://cs.chromium.org/search/?q=DCHECK.*%3C%3C&sq=package:chromium&type=cs

If I understand the code correctly, my message will be added after a ". "
(period, space), so adding a space seems redundant.
https://cs.chromium.org/chromium/src/base/logging.h?q=DCHECK&sq=package:chrom...

Thank you in advance!

     IDBOpenDBRequest* openDBRequest = nullptr;
     IDBTransaction* transaction = new IDBTransaction(scriptState, id, objectStoreNames, mode, db, openDBRequest, IDBDatabaseMetadata());
     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 IDBDatabaseMetadata& 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;
 }
 
 namespace {
 
 class DeactivateTransactionTask : public V8PerIsolateData::EndOfScopeTask {
 public:
     static std::unique_ptr<DeactivateTransactionTask> create(IDBTransaction* transaction)
     {
         return wrapUnique(new DeactivateTransactionTask(transaction));
     }
 
     void run() override
     {
         m_transaction->setActive(false);
         m_transaction.clear();
     }
 
 private:
     explicit DeactivateTransactionTask(IDBTransaction* transaction)
         : m_transaction(transaction) { }
 
     Persistent<IDBTransaction> m_transaction;
 };
 
 } // 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 IDBDatabaseMetadata& oldMetadata)
     : ActiveScriptWrappable(this)
     , ActiveDOMObject(scriptState->getExecutionContext())
     , m_id(id)
     , m_database(db)
-    , m_objectStoreNames(objectStoreNames)
+    , m_scope(objectStoreNames)

[cmumford, 2016/09/19 23:26:48]

Nit: It does seem a bit odd to have a public name "objectStoreNames", but a
different internal name: "m_scope".

[pwnall, 2016/09/20 09:11:14]

I agree that it does seem weird. Here is my rationale:

(1) the spec states that a transaction has a scope:
https://w3c.github.io/IndexedDB/#transaction-scope
(2) the WebIDL for creating transactions uses the storeNames parameter name:
https://w3c.github.io/IndexedDB/#dom-idbdatabase-transaction
(3) We have an objectStoreNames method (WebIDL accessor) that sometimes returns
m_scope, and sometimes returns something else. (for versionchange)

I flipped "objectStoreNames" to "scope", and kept the "storeNames" parameter in
IDBDatabase::transaction(). I think this name is a bit better, because it maps
directly to a spec concept. Granted, it'd be pretty easy to guess what a list of
objectStoreNames is for, so the difference doesn't seem big. I'll be happy to
switch everything back to objectStoreNames if it makes more sense to you!

     , m_openDBRequest(openDBRequest)
     , m_mode(mode)
-    , m_previousMetadata(previousMetadata)
+    , m_oldDatabaseMetadata(oldMetadata)
 {
-    if (mode == WebIDBTransactionModeVersionChange) {
+    if (isVersionChange()) {

[cmumford, 2016/09/19 23:26:48]

Is there a need to DCHECK mode?

[pwnall, 2016/09/20 09:11:14]

We got by without it, but it seems like a good idea. I added a couple of DCHECKs
in this constructor, because I think it's the best place to check rep
invariants. Thank you for pointing this out!

+        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";

[cmumford, 2016/09/19 23:26:48]

Space before 'Non'

[pwnall, 2016/09/20 09:11:14]

The same question as above applies. After I understand the reasoning, I'll make
a pass through all the DCHECKs I've added to IndexedDB.

     }
 
     if (m_state == Active)
         V8PerIsolateData::from(scriptState->isolate())->addEndOfScopeTask(DeactivateTransactionTask::create(this));
     m_database->transactionCreated(this);
 }
 
 IDBTransaction::~IDBTransaction()
 {
     ASSERT(m_state == Finished || m_contextStopped);
     ASSERT(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);
     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) {
         m_error = error;
     }
 }
 
 IDBObjectStore* IDBTransaction::objectStore(const String& name, ExceptionState& exceptionState)
 {
-    if (m_state == Finished) {
+    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_objectStoreNames.contains(name)) {
+    if (!isVersionChange() && !m_scope.contains(name)) {
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchObjectStoreErrorMessage);
         return nullptr;
     }
 
     int64_t objectStoreId = m_database->findObjectStoreId(name);
     if (objectStoreId == IDBObjectStoreMetadata::InvalidId) {
         ASSERT(isVersionChange());
         exceptionState.throwDOMException(NotFoundError, IDBDatabase::noSuchObjectStoreErrorMessage);
         return nullptr;
     }
 
-    const IDBDatabaseMetadata& metadata = m_database->metadata();
+    DCHECK(m_database->metadata().objectStores.contains(objectStoreId));
+    const IDBObjectStoreMetadata& objectStoreMetadata = m_database->metadata().objectStores.get(objectStoreId);
 
-    IDBObjectStore* objectStore = IDBObjectStore::create(metadata.objectStores.get(objectStoreId), this);
+    IDBObjectStore* objectStore = IDBObjectStore::create(objectStoreMetadata, this);
+    DCHECK(!m_objectStoreMap.contains(name));
     m_objectStoreMap.set(name, objectStore);
     m_objectStoreCleanupMap.set(objectStore, objectStore->metadata());
     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";

[cmumford, 2016/09/19 23:26:48]

spaces before these (and all) strings, so:

  DCHECK(...) << "Foo";

Should be:

  DCHECK(...) << " Foo";

[pwnall, 2016/09/20 09:11:14]

The same question as above applies. After I understand the reasoning, I'll make
a pass through all the DCHECKs I've added to IndexedDB.

+    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)
 {
-    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()) {
         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.";
-
-    IDBObjectStoreMap::iterator it = m_objectStoreMap.find(oldName);
-    m_objectStoreMap.set(newName, it->value);
-    m_objectStoreMap.remove(oldName);
+    IDBObjectStore* objectStore = m_objectStoreMap.take(oldName);
+    m_objectStoreMap.set(newName, objectStore);
 }
 
 void IDBTransaction::setActive(bool active)
 {
     DCHECK_NE(m_state, Finished) << "A finished transaction tried to setActive(" << (active ? "true" : "false") << ")";
     if (m_state == Finishing)
         return;
     ASSERT(active != (m_state == Active));
     m_state = active ? Active : Inactive;
 
     if (!active && m_requestList.isEmpty() && backendDB())
         backendDB()->commit(m_id);
 }
 
 void IDBTransaction::abort(ExceptionState& exceptionState)
 {
     if (m_state == Finishing || m_state == Finished) {
         exceptionState.throwDOMException(InvalidStateError, IDBDatabase::transactionFinishedErrorMessage);
         return;
     }
 
     m_state = Finishing;
 
     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);
 }
 
 void IDBTransaction::registerRequest(IDBRequest* request)
 {
     ASSERT(request);
     ASSERT(m_state == Active);
     m_requestList.add(request);
 }
 
 void IDBTransaction::unregisterRequest(IDBRequest* request)
 {
     ASSERT(request);
     // If we aborted the request, it will already have been removed.
     m_requestList.remove(request);
 }
 
 void IDBTransaction::onAbort(DOMException* error)
 {
     IDB_TRACE("IDBTransaction::onAbort");
     if (m_contextStopped) {
-        m_database->transactionFinished(this);
+        finish();
         return;
     }
 
     ASSERT(m_state != Finished);
     if (m_state != Finishing) {
         // Abort was not triggered by front-end.
-        ASSERT(error);
+        DCHECK(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
 {
     // FIXME: In an ideal world, we should return true as long as anyone has a or can
     //        get a handle to us or any child request object and any of those have
     //        event listeners. This is  in order to handle user generated events properly.
     return m_hasPendingActivity && !m_contextStopped;
 }
 
@@ skipping 19 matching lines @@
         return IndexedDBNames::readwrite;
 
     case WebIDBTransactionModeVersionChange:
         return IndexedDBNames::versionchange;
     }
 
     ASSERT_NOT_REACHED();
     return IndexedDBNames::readonly;
 }
 
+WebIDBDatabase* IDBTransaction::backendDB() const
+{
+    return m_database->backend();
+}
+
 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;
 }
 
 const AtomicString& IDBTransaction::interfaceName() const
 {
     return EventTargetNames::IDBTransaction;
 }
 
 ExecutionContext* IDBTransaction::getExecutionContext() const
 {
     return ActiveDOMObject::getExecutionContext();
 }
 
 DispatchEventResult IDBTransaction::dispatchEventInternal(Event* event)
 {
     IDB_TRACE("IDBTransaction::dispatchEvent");
     if (m_contextStopped || !getExecutionContext()) {
         m_state = Finished;
         return DispatchEventResult::CanceledBeforeDispatch;
     }
-    ASSERT(m_state != Finished);
-    ASSERT(m_hasPendingActivity);
-    ASSERT(getExecutionContext());
-    ASSERT(event->target() == this);
+    DCHECK_NE(m_state, Finished);
+    DCHECK(m_hasPendingActivity);
+    DCHECK(getExecutionContext());
+    DCHECK_EQ(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());
 
     // FIXME: When we allow custom event dispatching, this will probably need to change.
-    ASSERT(event->type() == EventTypeNames::complete || event->type() == EventTypeNames::abort);
+    DCHECK(event->type() == EventTypeNames::complete || event->type() == EventTypeNames::abort);
     DispatchEventResult dispatchResult = IDBEventDispatcher::dispatch(event, targets);
     // FIXME: Try to construct a test where |this| outlives openDBRequest and we
     // get a crash.
     if (m_openDBRequest) {
-        ASSERT(isVersionChange());
+        DCHECK(isVersionChange());
         m_openDBRequest->transactionDidFinishAndDispatch();
     }
     m_hasPendingActivity = false;
     return dispatchResult;
 }
 
 void IDBTransaction::stop()
 {
     if (m_contextStopped)
         return;
 
     m_contextStopped = true;
 
     abort(IGNORE_EXCEPTION);
 }
 
 void IDBTransaction::enqueueEvent(Event* event)
 {
     DCHECK_NE(m_state, Finished) << "A finished transaction tried to enqueue an event of type " << event->type() << ".";
     if (m_contextStopped || !getExecutionContext())
         return;
 
     EventQueue* eventQueue = getExecutionContext()->getEventQueue();
     event->setTarget(this);
     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;
+
+    // Newly created stores must be marked as deleted.
+    for (IDBObjectStore* store : m_createdObjectStores) {
+        store->abort();
+        store->markDeleted();
+    }
+
+    // Used stores may need to mark indexes as deleted.
+    for (auto& it : m_objectStoreCleanupMap) {
+        it.key->abort();
+        it.key->setMetadata(it.value);
+    }
+
+    m_database->setMetadata(m_oldDatabaseMetadata);
+}
+
+void IDBTransaction::finish()
+{
+#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();
+    m_objectStoreMap.clear();
+    for (auto& it : m_deletedObjectStores)
+        it->transactionFinished();
+    m_createdObjectStores.clear();
+    m_deletedObjectStores.clear();
+
+    m_objectStoreCleanupMap.clear();
 }
 
 } // namespace blink