Backed out 3 changesets (bug 1497007) for assertion failures on IDBTransaction.cpp. CLOSED TREE

Backed out changeset 72bc4f39b659 (bug 1497007)
Backed out changeset 99fc4eedacc7 (bug 1497007)
Backed out changeset 787c340dd3d0 (bug 1497007)

dom/indexedDB/ActorsChild.cpp

@@ -692,7 +692,7 @@ void DispatchErrorEvent(IDBRequest* aRequest, nsresult aErrorCode,
   }
 
   MOZ_ASSERT(!transaction || transaction->IsActive() ||
-             transaction->IsAborted() || transaction->WasExplicitlyCommitted());
+             transaction->IsAborted());
 
   if (transaction && transaction->IsActive()) {
     transaction->TransitionToInactive();
@@ -760,7 +760,7 @@ void DispatchSuccessEvent(ResultHelper* aResultHelper,
                                IDB_LOG_STRINGIFY(aEvent, kSuccessEventType));
   }
 
-  MOZ_ASSERT_IF(transaction && !transaction->WasExplicitlyCommitted(),
+  MOZ_ASSERT_IF(transaction,
                 transaction->IsActive() && !transaction->IsAborted());
 
   IgnoredErrorResult rv;
@@ -779,7 +779,7 @@ void DispatchSuccessEvent(ResultHelper* aResultHelper,
       transaction->Abort(NS_ERROR_DOM_INDEXEDDB_ABORT_ERR);
     } else {
       // To handle upgrade transaction.
-      transaction->CommitIfNotStarted();
+      transaction->Run();
     }
   }
 }

dom/indexedDB/ActorsParent.cpp

@@ -6288,7 +6288,6 @@ class TransactionBase {
   FlippedOnce<false> mCommitOrAbortReceived;
   FlippedOnce<false> mCommittedOrAborted;
   FlippedOnce<false> mForceAborted;
-  bool mHasFailedRequest = false;
 
  public:
   void AssertIsOnConnectionThread() const {
@@ -6381,8 +6380,6 @@ class TransactionBase {
 
   void NoteFinishedRequest();
 
-  void NoteFailedRequest(nsresult aResultCode);
-
   void Invalidate();
 
  protected:
@@ -14082,10 +14079,6 @@ bool TransactionBase::RecvCommit() {
 
   mCommitOrAbortReceived.Flip();
 
-  // Ignore requests that failed before we were committing (cf.
-  // https://w3c.github.io/IndexedDB/#async-execute-request step 5.3 vs. 5.4).
-  mHasFailedRequest = false;
-
   MaybeCommitOrAbort();
   return true;
 }
@@ -14642,15 +14635,6 @@ void TransactionBase::NoteFinishedRequest() {
   MaybeCommitOrAbort();
 }
 
-void TransactionBase::NoteFailedRequest(const nsresult aResultCode) {
-  AssertIsOnConnectionThread();
-
-  // TODO: Should we store the result code? Maybe also the request that failed?
-  MOZ_ASSERT(NS_FAILED(aResultCode));
-
-  mHasFailedRequest = true;
-}
-
 void TransactionBase::Invalidate() {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(mInvalidated == mInvalidatedOnAnyThread);
@@ -22609,8 +22593,6 @@ void TransactionDatabaseOperationBase::RunOnConnectionThread() {
         }
 
         if (NS_FAILED(rv)) {
-          (*mTransaction)->NoteFailedRequest(rv);
-
           SetFailureCode(rv);
         }
       }
@@ -22919,10 +22901,6 @@ TransactionBase::CommitOp::Run() {
   MOZ_ASSERT(mTransaction);
   mTransaction->AssertIsOnConnectionThread();
 
-  if (NS_SUCCEEDED(mResultCode) && mTransaction->mHasFailedRequest) {
-    mResultCode = NS_ERROR_DOM_INDEXEDDB_ABORT_ERR;
-  }
-
   AUTO_PROFILER_LABEL("TransactionBase::CommitOp::Run", DOM);
 
   IDB_LOG_MARK_PARENT_TRANSACTION_REQUEST(

dom/indexedDB/IDBTransaction.cpp

@@ -104,6 +104,7 @@ IDBTransaction::IDBTransaction(IDBDatabase* const aDatabase,
       mLineNo(aLineNo),
       mColumn(aColumn),
       mMode(aMode),
+      mCreating(false),
       mRegistered(false),
       mNotedActiveTransaction(false) {
   MOZ_ASSERT(aDatabase);
@@ -132,7 +133,7 @@ IDBTransaction::IDBTransaction(IDBDatabase* const aDatabase,
 IDBTransaction::~IDBTransaction() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(!mPendingRequestCount);
-  MOZ_ASSERT(mReadyState == ReadyState::Finished);
+  MOZ_ASSERT(!mCreating);
   MOZ_ASSERT(!mNotedActiveTransaction);
   MOZ_ASSERT(mSentCommitOrAbort);
   MOZ_ASSERT_IF(HasTransactionChild(), mFiredCompleteOrAbort);
@@ -239,6 +240,8 @@ RefPtr<IDBTransaction> IDBTransaction::Create(
   nsCOMPtr<nsIRunnable> runnable = do_QueryObject(transaction);
   nsContentUtils::AddPendingIDBTransaction(runnable.forget());
 
+  transaction->mCreating = true;
+
   aDatabase->RegisterTransaction(transaction);
   transaction->mRegistered = true;
 
@@ -340,32 +343,30 @@ void IDBTransaction::OnNewRequest() {
 void IDBTransaction::OnRequestFinished(
     const bool aRequestCompletedSuccessfully) {
   AssertIsOnOwningThread();
-  MOZ_ASSERT(mReadyState != ReadyState::Active);
+  MOZ_ASSERT(mReadyState == ReadyState::Inactive ||
+             mReadyState == ReadyState::Finished);
   MOZ_ASSERT_IF(mReadyState == ReadyState::Finished, !NS_SUCCEEDED(mAbortCode));
   MOZ_ASSERT(mPendingRequestCount);
 
   --mPendingRequestCount;
 
   if (!mPendingRequestCount) {
-    MOZ_ASSERT(mReadyState != ReadyState::Active || mStarted);
-    if (mSentCommitOrAbort) {
-      return;
-    }
-
     if (mReadyState == ReadyState::Inactive) {
       mReadyState = ReadyState::Committing;
     }
 
     if (aRequestCompletedSuccessfully) {
       if (NS_SUCCEEDED(mAbortCode)) {
-        SendCommit(true);
+        SendCommit();
       } else {
         SendAbort(mAbortCode);
       }
     } else {
       // Don't try to send any more messages to the parent if the request actor
       // was killed.
+#ifdef DEBUG
       mSentCommitOrAbort.Flip();
+#endif
       IDB_LOG_MARK_CHILD_TRANSACTION(
           "Request actor was killed, transaction will be aborted",
           "IDBTransaction abort", LoggingSerialNumber());
@@ -373,23 +374,25 @@ void IDBTransaction::OnRequestFinished(
   }
 }
 
-void IDBTransaction::SendCommit(const bool aAutoCommit) {
+void IDBTransaction::SendCommit() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(NS_SUCCEEDED(mAbortCode));
   MOZ_ASSERT(IsCommittingOrFinished());
+  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_CHILD_TRANSACTION_REQUEST(
-      "Committing transaction (%s)", "IDBTransaction commit (%s)",
+      "All requests complete, committing transaction", "IDBTransaction commit",
-      LoggingSerialNumber(), requestSerialNumber,
+      LoggingSerialNumber(), requestSerialNumber);
-      aAutoCommit ? "automatically" : "explicitly");
 
   DoWithTransactionChild([](auto& actor) { actor.SendCommit(); });
 
+#ifdef DEBUG
   mSentCommitOrAbort.Flip();
+#endif
 }
 
 void IDBTransaction::SendAbort(const nsresult aResultCode) {
@@ -408,7 +411,9 @@ void IDBTransaction::SendAbort(const nsresult aResultCode) {
   DoWithTransactionChild(
       [aResultCode](auto& actor) { actor.SendAbort(aResultCode); });
 
+#ifdef DEBUG
   mSentCommitOrAbort.Flip();
+#endif
 }
 
 void IDBTransaction::NoteActiveTransaction() {
@@ -431,7 +436,14 @@ void IDBTransaction::MaybeNoteInactiveTransaction() {
 bool IDBTransaction::CanAcceptRequests() const {
   AssertIsOnOwningThread();
 
-  return mReadyState == ReadyState::Active;
+  // If we haven't started anything then we can accept requests.
+  // If we've already started then we need to check to see if we still have the
+  // mCreating flag set. If we do (i.e. we haven't returned to the event loop
+  // from the time we were created) then we can accept requests. Otherwise check
+  // the currently running transaction to see if it's the same. We only allow
+  // other requests to be made if this transaction is currently running.
+  return mReadyState == ReadyState::Active &&
+         (!mStarted || mCreating || GetCurrent() == this);
 }
 
 IDBTransaction::AutoRestoreState<IDBTransaction::ReadyState::Inactive,
@@ -702,26 +714,13 @@ void IDBTransaction::Abort(ErrorResult& aRv) {
 void IDBTransaction::Commit(ErrorResult& aRv) {
   AssertIsOnOwningThread();
 
-  if (mReadyState != ReadyState::Active || !mNotedActiveTransaction) {
+  if (IsCommittingOrFinished()) {
-    aRv = NS_ERROR_DOM_INVALID_STATE_ERR;
+    aRv = NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR;
     return;
   }
 
-  MOZ_ASSERT(!mSentCommitOrAbort);
+  // TODO
-
+  aRv = NS_ERROR_NOT_IMPLEMENTED;
-  MOZ_ASSERT(mReadyState == ReadyState::Active);
-  mReadyState = ReadyState::Committing;
-  if (NS_WARN_IF(NS_FAILED(mAbortCode))) {
-    SendAbort(mAbortCode);
-    aRv = mAbortCode;
-    return;
-  }
-
-#ifdef DEBUG
-  mWasExplicitlyCommitted.Flip();
-#endif
-
-  SendCommit(false);
 }
 
 void IDBTransaction::FireCompleteOrAbortEvents(const nsresult aResult) {
@@ -970,43 +969,21 @@ NS_IMETHODIMP
 IDBTransaction::Run() {
   AssertIsOnOwningThread();
 
-  // TODO: Instead of checking for Finished and Committing states here, we could
+  // We're back at the event loop, no longer newborn.
-  // remove the transaction from the pending IDB transactions list on
+  mCreating = false;
-  // abort/commit.
 
-  if (ReadyState::Finished == mReadyState) {
+  MOZ_ASSERT_IF(mReadyState == ReadyState::Finished, IsAborted());
-    MOZ_ASSERT(IsAborted());
-    return NS_OK;
-  }
-
-  if (ReadyState::Committing == mReadyState) {
-    MOZ_ASSERT(mWasExplicitlyCommitted);
-    return NS_OK;
-  }
-
-  // We're back at the event loop, no longer newborn, so
-  // return to Inactive state:
-  // https://w3c.github.io/IndexedDB/#cleanup-indexed-database-transactions.
-  MOZ_ASSERT(ReadyState::Active == mReadyState);
-  mReadyState = ReadyState::Inactive;
-
-  CommitIfNotStarted();
-
-  return NS_OK;
-}
-
-void IDBTransaction::CommitIfNotStarted() {
-  AssertIsOnOwningThread();
-
-  MOZ_ASSERT(ReadyState::Inactive == mReadyState);
 
   // Maybe commit if there were no requests generated.
-  if (!mStarted) {
+  if (!mStarted && mReadyState != ReadyState::Finished) {
-    MOZ_ASSERT(!mPendingRequestCount);
+    MOZ_ASSERT(mReadyState == ReadyState::Inactive ||
+               mReadyState == ReadyState::Active);
     mReadyState = ReadyState::Finished;
 
-    SendCommit(true);
+    SendCommit();
   }
+
+  return NS_OK;
 }
 
 }  // namespace dom

dom/indexedDB/IDBTransaction.h

@@ -108,16 +108,17 @@ class IDBTransaction final
   FlippedOnce<false> mStarted;
   const Mode mMode;
 
+  bool mCreating;    ///< Set between successful creation until the transaction
+                     ///< has run on the event-loop.
   bool mRegistered;  ///< Whether mDatabase->RegisterTransaction() has been
                      ///< called (which may not be the case if construction was
                      ///< incomplete).
   FlippedOnce<false> mAbortedByScript;
   bool mNotedActiveTransaction;
-  FlippedOnce<false> mSentCommitOrAbort;
 
 #ifdef DEBUG
+  FlippedOnce<false> mSentCommitOrAbort;
   FlippedOnce<false> mFiredCompleteOrAbort;
-  FlippedOnce<false> mWasExplicitlyCommitted;
 #endif
 
  public:
@@ -204,10 +205,6 @@ class IDBTransaction final
     return NS_FAILED(mAbortCode);
   }
 
-#ifdef DEBUG
-  bool WasExplicitlyCommitted() const { return mWasExplicitlyCommitted; }
-#endif
-
   template <ReadyState OriginalState, ReadyState TemporaryState>
   class AutoRestoreState {
    public:
@@ -319,8 +316,6 @@ class IDBTransaction final
   NS_DECL_NSIRUNNABLE
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(IDBTransaction, DOMEventTargetHelper)
 
-  void CommitIfNotStarted();
-
   // nsWrapperCache
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;
@@ -362,7 +357,7 @@ class IDBTransaction final
 
   void AbortInternal(nsresult aAbortCode, RefPtr<DOMException> aError);
 
-  void SendCommit(bool aAutoCommit);
+  void SendCommit();
 
   void SendAbort(nsresult aResultCode);
 

testing/web-platform/meta/IndexedDB/idb-explicit-commit-throw.any.js.ini

@@ -0,0 +1,9 @@
+[idb-explicit-commit-throw.any.worker.html]
+  [Any errors in callbacks that run after an explicit commit will not stop the commit from being processed.]
+    expected: FAIL
+
+
+[idb-explicit-commit-throw.any.html]
+  [Any errors in callbacks that run after an explicit commit will not stop the commit from being processed.]
+    expected: FAIL
+

testing/web-platform/meta/IndexedDB/idb-explicit-commit.any.js.ini

@@ -0,0 +1,69 @@
+[idb-explicit-commit.any.worker.html]
+  [Calling commit on a committed transaction throws.]
+    expected: FAIL
+
+  [Explicitly committed data can be read back out.]
+    expected: FAIL
+
+  [Calling txn.commit() when txn is inactive should throw.]
+    expected: FAIL
+
+  [commit() on a version change transaction does not cause errors.]
+    expected: FAIL
+
+  [Puts issued after commit are not fulfilled.]
+    expected: FAIL
+
+  [Calling abort on a committed transaction throws and does not prevent persisting the data.]
+    expected: FAIL
+
+  [A committed transaction becomes inactive immediately.]
+    expected: FAIL
+
+  [A committed transaction is inactive in future request callbacks.]
+    expected: FAIL
+
+  [Transactions that explicitly commit and have errors should abort.]
+    expected: FAIL
+
+  [Transactions with same scope should stay in program order, even if one calls commit.]
+    expected: FAIL
+
+  [Transactions that handle all errors properly should behave as expected when an explicit commit is called in an onerror handler.]
+    expected: FAIL
+
+
+[idb-explicit-commit.any.html]
+  [Calling commit on a committed transaction throws.]
+    expected: FAIL
+
+  [Explicitly committed data can be read back out.]
+    expected: FAIL
+
+  [Calling txn.commit() when txn is inactive should throw.]
+    expected: FAIL
+
+  [commit() on a version change transaction does not cause errors.]
+    expected: FAIL
+
+  [Puts issued after commit are not fulfilled.]
+    expected: FAIL
+
+  [Calling abort on a committed transaction throws and does not prevent persisting the data.]
+    expected: FAIL
+
+  [A committed transaction becomes inactive immediately.]
+    expected: FAIL
+
+  [A committed transaction is inactive in future request callbacks.]
+    expected: FAIL
+
+  [Transactions that explicitly commit and have errors should abort.]
+    expected: FAIL
+
+  [Transactions with same scope should stay in program order, even if one calls commit.]
+    expected: FAIL
+
+  [Transactions that handle all errors properly should behave as expected when an explicit commit is called in an onerror handler.]
+    expected: FAIL
+

testing/web-platform/tests/IndexedDB/idb-explicit-commit.any.js

@@ -197,8 +197,8 @@ promise_test(async testCase => {
 
   // Exercise the IndexedDB transaction ordering by executing one with a
   // different scope.
-  const txn3 = db.transaction(['not_books'], 'readonly');
+  const txn3 = db.transaction(['not_books'], 'readwrite');
-  txn3.objectStore('not_books').getAllKeys();
+  txn3.objectStore('not_books').put({'title': 'not_title'}, 'key');
   txn3.oncomplete = function() {
     releaseTxnFunction();
   }