Bug 1147942 - Don't warn when aborting finished IndexedDB transactions, r=janv.
authorBen Turner <bent.mozilla@gmail.com>
Thu, 25 Jun 2015 15:22:59 -0700
changeset 250234 195067eb3141098731da347a746d00c7eae62d1b
parent 250233 41c7369a3090636616cbfd949228f5c096496fbf
child 250235 42fa3442672cb2b9fe573428b07de64056c9b126
push id61488
push userbturner@mozilla.com
push dateThu, 25 Jun 2015 22:23:31 +0000
treeherdermozilla-inbound@195067eb3141 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanv
bugs1147942
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1147942 - Don't warn when aborting finished IndexedDB transactions, r=janv.
dom/indexedDB/IDBDatabase.cpp
dom/indexedDB/IDBObjectStore.cpp
dom/indexedDB/IDBTransaction.cpp
dom/indexedDB/IDBTransaction.h
--- a/dom/indexedDB/IDBDatabase.cpp
+++ b/dom/indexedDB/IDBDatabase.cpp
@@ -891,80 +891,114 @@ IDBDatabase::UnregisterTransaction(IDBTr
 
 void
 IDBDatabase::AbortTransactions(bool aShouldWarn)
 {
   AssertIsOnOwningThread();
 
   class MOZ_STACK_CLASS Helper final
   {
+    typedef nsAutoTArray<nsRefPtr<IDBTransaction>, 20> StrongTransactionArray;
+    typedef nsAutoTArray<IDBTransaction*, 20> WeakTransactionArray;
+
   public:
     static void
-    AbortTransactions(nsTHashtable<nsPtrHashKey<IDBTransaction>>& aTable,
-                      nsTArray<nsRefPtr<IDBTransaction>>& aAbortedTransactions)
+    AbortTransactions(IDBDatabase* aDatabase, const bool aShouldWarn)
     {
-      const uint32_t count = aTable.Count();
-      if (!count) {
+      MOZ_ASSERT(aDatabase);
+      aDatabase->AssertIsOnOwningThread();
+
+      nsTHashtable<nsPtrHashKey<IDBTransaction>>& transactionTable =
+        aDatabase->mTransactions;
+
+      if (!transactionTable.Count()) {
+        return;
+      }
+
+      StrongTransactionArray transactionsToAbort;
+      transactionsToAbort.SetCapacity(transactionTable.Count());
+
+      transactionTable.EnumerateEntries(Collect, &transactionsToAbort);
+      MOZ_ASSERT(transactionsToAbort.Length() <= transactionTable.Count());
+
+      if (transactionsToAbort.IsEmpty()) {
         return;
       }
 
-      nsAutoTArray<nsRefPtr<IDBTransaction>, 20> transactions;
-      transactions.SetCapacity(count);
+      // We want to abort transactions as soon as possible so we iterate the
+      // transactions once and abort them all first, collecting the transactions
+      // that need to have a warning issued along the way. Those that need a
+      // warning will be a subset of those that are aborted, so we don't need
+      // additional strong references here.
+      WeakTransactionArray transactionsThatNeedWarning;
 
-      aTable.EnumerateEntries(Collect, &transactions);
+      for (nsRefPtr<IDBTransaction>& transaction : transactionsToAbort) {
+        MOZ_ASSERT(transaction);
+        MOZ_ASSERT(!transaction->IsDone());
+
+        if (aShouldWarn) {
+          switch (transaction->GetMode()) {
+            // We ignore transactions that could not have written any data.
+            case IDBTransaction::READ_ONLY:
+              break;
 
-      MOZ_ASSERT(transactions.Length() == count);
+            // We warn for any transactions that could have written data.
+            case IDBTransaction::READ_WRITE:
+            case IDBTransaction::READ_WRITE_FLUSH:
+            case IDBTransaction::VERSION_CHANGE:
+              transactionsThatNeedWarning.AppendElement(transaction);
+              break;
 
-      for (uint32_t index = 0; index < count; index++) {
-        nsRefPtr<IDBTransaction> transaction = Move(transactions[index]);
+            default:
+              MOZ_CRASH("Unknown mode!");
+          }
+        }
+
+        transaction->Abort(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
+      }
+
+      static const char kWarningMessage[] =
+        "IndexedDBTransactionAbortNavigation";
+
+      for (IDBTransaction* transaction : transactionsThatNeedWarning) {
         MOZ_ASSERT(transaction);
 
-        transaction->Abort(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
+        nsString filename;
+        uint32_t lineNo;
+        transaction->GetCallerLocation(filename, &lineNo);
 
-        // We only care about warning for write transactions.
-        if (transaction->GetMode() != IDBTransaction::READ_ONLY) {
-          aAbortedTransactions.AppendElement(Move(transaction));
-        }
+        aDatabase->LogWarning(kWarningMessage, filename, lineNo);
       }
     }
 
   private:
     static PLDHashOperator
-    Collect(nsPtrHashKey<IDBTransaction>* aTransaction, void* aClosure)
+    Collect(nsPtrHashKey<IDBTransaction>* aTransactionKey, void* aClosure)
     {
-      MOZ_ASSERT(aTransaction);
-      aTransaction->GetKey()->AssertIsOnOwningThread();
+      MOZ_ASSERT(aTransactionKey);
       MOZ_ASSERT(aClosure);
 
-      auto* array = static_cast<nsTArray<nsRefPtr<IDBTransaction>>*>(aClosure);
-      array->AppendElement(aTransaction->GetKey());
+      IDBTransaction* transaction = aTransactionKey->GetKey();
+      MOZ_ASSERT(transaction);
+
+      transaction->AssertIsOnOwningThread();
+
+      // Transactions that are already done can simply be ignored. Otherwise
+      // there is a race here and it's possible that the transaction has not
+      // been successfully committed yet so we will warn the user.
+      if (!transaction->IsDone()) {
+        auto* array = static_cast<StrongTransactionArray*>(aClosure);
+        array->AppendElement(transaction);
+      }
 
       return PL_DHASH_NEXT;
     }
   };
 
-  nsAutoTArray<nsRefPtr<IDBTransaction>, 5> abortedTransactions;
-  Helper::AbortTransactions(mTransactions, abortedTransactions);
-
-  if (aShouldWarn && !abortedTransactions.IsEmpty()) {
-    static const char kWarningMessage[] = "IndexedDBTransactionAbortNavigation";
-
-    for (uint32_t count = abortedTransactions.Length(), index = 0;
-         index < count;
-         index++) {
-      nsRefPtr<IDBTransaction>& transaction = abortedTransactions[index];
-      MOZ_ASSERT(transaction);
-
-      nsString filename;
-      uint32_t lineNo;
-      transaction->GetCallerLocation(filename, &lineNo);
-
-      LogWarning(kWarningMessage, filename, lineNo);
-    }
-  }
+  Helper::AbortTransactions(this, aShouldWarn);
 }
 
 PBackgroundIDBDatabaseFileChild*
 IDBDatabase::GetOrCreateFileActorForBlob(Blob* aBlob)
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(aBlob);
   MOZ_ASSERT(mBackgroundActor);
--- a/dom/indexedDB/IDBObjectStore.cpp
+++ b/dom/indexedDB/IDBObjectStore.cpp
@@ -1432,17 +1432,17 @@ IDBObjectStore::Clear(ErrorResult& aRv)
   return request.forget();
 }
 
 already_AddRefed<IDBIndex>
 IDBObjectStore::Index(const nsAString& aName, ErrorResult &aRv)
 {
   AssertIsOnOwningThread();
 
-  if (mTransaction->IsFinished()) {
+  if (mTransaction->IsCommittingOrDone() || mDeletedSpec) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
 
   const nsTArray<IndexMetadata>& indexes = mSpec->indexes();
 
   const IndexMetadata* metadata = nullptr;
 
--- a/dom/indexedDB/IDBTransaction.cpp
+++ b/dom/indexedDB/IDBTransaction.cpp
@@ -443,17 +443,17 @@ IDBTransaction::OnRequestFinished(bool a
   }
 }
 
 void
 IDBTransaction::SendCommit()
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(NS_SUCCEEDED(mAbortCode));
-  MOZ_ASSERT(IsFinished());
+  MOZ_ASSERT(IsCommittingOrDone());
   MOZ_ASSERT(!mSentCommitOrAbort);
   MOZ_ASSERT(!mPendingRequestCount);
 
   // Don't do this in the macro because we always need to increment the serial
   // number to keep in sync with the parent.
   const uint64_t requestSerialNumber = IDBRequest::NextSerialNumber();
 
   IDB_LOG_MARK("IndexedDB %s: Child  Transaction[%lld] Request[%llu]: "
@@ -476,17 +476,17 @@ IDBTransaction::SendCommit()
 #endif
 }
 
 void
 IDBTransaction::SendAbort(nsresult aResultCode)
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(NS_FAILED(aResultCode));
-  MOZ_ASSERT(IsFinished());
+  MOZ_ASSERT(IsCommittingOrDone());
   MOZ_ASSERT(!mSentCommitOrAbort);
 
   // Don't do this in the macro because we always need to increment the serial
   // number to keep in sync with the parent.
   const uint64_t requestSerialNumber = IDBRequest::NextSerialNumber();
 
   IDB_LOG_MARK("IndexedDB %s: Child  Transaction[%lld] Request[%llu]: "
                  "Aborting transaction with result 0x%x",
@@ -635,24 +635,20 @@ IDBTransaction::DeleteIndex(IDBObjectSto
 }
 
 void
 IDBTransaction::AbortInternal(nsresult aAbortCode,
                               already_AddRefed<DOMError> aError)
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(NS_FAILED(aAbortCode));
+  MOZ_ASSERT(!IsCommittingOrDone());
 
   nsRefPtr<DOMError> error = aError;
 
-  if (IsFinished()) {
-    // Already finished, nothing to do here.
-    return;
-  }
-
   const bool isVersionChange = mMode == VERSION_CHANGE;
   const bool isInvalidated = mDatabase->IsInvalidated();
   bool needToSendAbort = mReadyState == INITIAL && !isInvalidated;
 
   if (isInvalidated) {
 #ifdef DEBUG
     mSentCommitOrAbort = true;
 #endif
@@ -735,37 +731,49 @@ IDBTransaction::AbortInternal(nsresult a
 }
 
 void
 IDBTransaction::Abort(IDBRequest* aRequest)
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(aRequest);
 
+  if (IsCommittingOrDone()) {
+    // Already started (and maybe finished) the commit or abort so there is
+    // nothing to do here.
+    return;
+  }
+
   ErrorResult rv;
   nsRefPtr<DOMError> error = aRequest->GetError(rv);
 
   AbortInternal(aRequest->GetErrorCode(), error.forget());
 }
 
 void
 IDBTransaction::Abort(nsresult aErrorCode)
 {
   AssertIsOnOwningThread();
 
+  if (IsCommittingOrDone()) {
+    // Already started (and maybe finished) the commit or abort so there is
+    // nothing to do here.
+    return;
+  }
+
   nsRefPtr<DOMError> error = new DOMError(GetOwner(), aErrorCode);
   AbortInternal(aErrorCode, error.forget());
 }
 
 void
 IDBTransaction::Abort(ErrorResult& aRv)
 {
   AssertIsOnOwningThread();
 
-  if (IsFinished()) {
+  if (IsCommittingOrDone()) {
     aRv = NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR;
     return;
   }
 
   AbortInternal(NS_ERROR_DOM_INDEXEDDB_ABORT_ERR, nullptr);
 
   MOZ_ASSERT(!mAbortedByScript);
   mAbortedByScript = true;
@@ -900,17 +908,17 @@ IDBTransaction::ObjectStoreNames()
   return list.forget();
 }
 
 already_AddRefed<IDBObjectStore>
 IDBTransaction::ObjectStore(const nsAString& aName, ErrorResult& aRv)
 {
   AssertIsOnOwningThread();
 
-  if (IsFinished()) {
+  if (IsCommittingOrDone()) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
 
   const ObjectStoreSpec* spec = nullptr;
 
   if (IDBTransaction::VERSION_CHANGE == mMode ||
       mObjectStoreNames.Contains(aName)) {
@@ -1024,21 +1032,22 @@ WorkerFeature::Notify(JSContext* aCx, St
 {
   MOZ_ASSERT(mWorkerPrivate);
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(aStatus > Running);
 
   if (mTransaction && aStatus > Terminating) {
     mTransaction->AssertIsOnOwningThread();
 
-    nsRefPtr<IDBTransaction> transaction = mTransaction;
-    mTransaction = nullptr;
+    nsRefPtr<IDBTransaction> transaction = Move(mTransaction);
 
-    IDB_REPORT_INTERNAL_ERR();
-    transaction->AbortInternal(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, nullptr);
+    if (!transaction->IsCommittingOrDone()) {
+      IDB_REPORT_INTERNAL_ERR();
+      transaction->AbortInternal(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR, nullptr);
+    }
   }
 
   return true;
 }
 
 } // namespace indexedDB
 } // namespace dom
 } // namespace mozilla
--- a/dom/indexedDB/IDBTransaction.h
+++ b/dom/indexedDB/IDBTransaction.h
@@ -161,20 +161,29 @@ public:
 
   void
   RefreshSpec(bool aMayDelete);
 
   bool
   IsOpen() const;
 
   bool
-  IsFinished() const
+  IsCommittingOrDone() const
   {
     AssertIsOnOwningThread();
-    return mReadyState > LOADING;
+
+    return mReadyState == COMMITTING || mReadyState == DONE;
+  }
+
+  bool
+  IsDone() const
+  {
+    AssertIsOnOwningThread();
+
+    return mReadyState == DONE;
   }
 
   bool
   IsWriteAllowed() const
   {
     AssertIsOnOwningThread();
     return mMode == READ_WRITE ||
            mMode == READ_WRITE_FLUSH ||