Bug 1147942 - Don't warn when aborting finished IndexedDB transactions, r=janv.

dom/indexedDB/IDBDatabase.cpp

@@ -896,70 +896,104 @@ IDBDatabase::AbortTransactions(bool aShouldWarn)
 
   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;
       }
 
-      nsAutoTArray<nsRefPtr<IDBTransaction>, 20> transactions;
-      transactions.SetCapacity(count);
+      StrongTransactionArray transactionsToAbort;
+      transactionsToAbort.SetCapacity(transactionTable.Count());
 
-      aTable.EnumerateEntries(Collect, &transactions);
+      transactionTable.EnumerateEntries(Collect, &transactionsToAbort);
+      MOZ_ASSERT(transactionsToAbort.Length() <= transactionTable.Count());
 
-      MOZ_ASSERT(transactions.Length() == count);
+      if (transactionsToAbort.IsEmpty()) {
+        return;
+      }
 
-      for (uint32_t index = 0; index < count; index++) {
-        nsRefPtr<IDBTransaction> transaction = Move(transactions[index]);
+      // 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;
+
+      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;
+
+            // 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;
+
+            default:
+              MOZ_CRASH("Unknown mode!");
+          }
+        }
 
         transaction->Abort(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
+      }
 
-        // We only care about warning for write transactions.
-        if (transaction->GetMode() != IDBTransaction::READ_ONLY) {
-          aAbortedTransactions.AppendElement(Move(transaction));
-        }
+      static const char kWarningMessage[] =
+        "IndexedDBTransactionAbortNavigation";
+
+      for (IDBTransaction* transaction : transactionsThatNeedWarning) {
+        MOZ_ASSERT(transaction);
+
+        nsString filename;
+        uint32_t lineNo;
+        transaction->GetCallerLocation(filename, &lineNo);
+
+        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*

dom/indexedDB/IDBObjectStore.cpp

@@ -1437,7 +1437,7 @@ 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;
   }

dom/indexedDB/IDBTransaction.cpp

@@ -448,7 +448,7 @@ IDBTransaction::SendCommit()
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(NS_SUCCEEDED(mAbortCode));
-  MOZ_ASSERT(IsFinished());
+  MOZ_ASSERT(IsCommittingOrDone());
   MOZ_ASSERT(!mSentCommitOrAbort);
   MOZ_ASSERT(!mPendingRequestCount);
 
@@ -481,7 +481,7 @@ 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
@@ -640,14 +640,10 @@ IDBTransaction::AbortInternal(nsresult aAbortCode,
 {
   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;
@@ -740,6 +736,12 @@ 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);
 
@@ -751,6 +753,12 @@ 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());
 }
@@ -760,7 +768,7 @@ IDBTransaction::Abort(ErrorResult& aRv)
 {
   AssertIsOnOwningThread();
 
-  if (IsFinished()) {
+  if (IsCommittingOrDone()) {
     aRv = NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR;
     return;
   }
@@ -905,7 +913,7 @@ IDBTransaction::ObjectStore(const nsAString& aName, ErrorResult& aRv)
 {
   AssertIsOnOwningThread();
 
-  if (IsFinished()) {
+  if (IsCommittingOrDone()) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
@@ -1029,11 +1037,12 @@ WorkerFeature::Notify(JSContext* aCx, Status aStatus)
   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;

dom/indexedDB/IDBTransaction.h

@@ -166,10 +166,19 @@ public:
   IsOpen() const;
 
   bool
-  IsFinished() const
+  IsCommittingOrDone() const
   {
     AssertIsOnOwningThread();
-    return mReadyState > LOADING;
+
+    return mReadyState == COMMITTING || mReadyState == DONE;
+  }
+
+  bool
+  IsDone() const
+  {
+    AssertIsOnOwningThread();
+
+    return mReadyState == DONE;
   }
 
   bool