From 8ee62c60e1d5a3a31c5dc88577518c9dd34729e5 Mon Sep 17 00:00:00 2001 From: Csoregi Natalia Date: Tue, 14 Jan 2020 15:22:26 +0200 Subject: [PATCH] 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 | 6 +- dom/indexedDB/ActorsParent.cpp | 22 ---- dom/indexedDB/IDBTransaction.cpp | 103 +++++++----------- dom/indexedDB/IDBTransaction.h | 13 +-- .../idb-explicit-commit-throw.any.js.ini | 9 ++ .../IndexedDB/idb-explicit-commit.any.js.ini | 69 ++++++++++++ .../IndexedDB/idb-explicit-commit.any.js | 4 +- 7 files changed, 127 insertions(+), 99 deletions(-) create mode 100644 testing/web-platform/meta/IndexedDB/idb-explicit-commit-throw.any.js.ini create mode 100644 testing/web-platform/meta/IndexedDB/idb-explicit-commit.any.js.ini diff --git a/dom/indexedDB/ActorsChild.cpp b/dom/indexedDB/ActorsChild.cpp index a13dbc427df5..9f9e73165d24 100644 --- a/dom/indexedDB/ActorsChild.cpp +++ b/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(); } } } diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp index 86bc3c763496..53171f8ccab7 100644 --- a/dom/indexedDB/ActorsParent.cpp +++ b/dom/indexedDB/ActorsParent.cpp @@ -6288,7 +6288,6 @@ class TransactionBase { FlippedOnce mCommitOrAbortReceived; FlippedOnce mCommittedOrAborted; FlippedOnce 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( diff --git a/dom/indexedDB/IDBTransaction.cpp b/dom/indexedDB/IDBTransaction.cpp index 20dab73a981f..deaeeea7d331 100644 --- a/dom/indexedDB/IDBTransaction.cpp +++ b/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::Create( nsCOMPtr 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)", - LoggingSerialNumber(), requestSerialNumber, - aAutoCommit ? "automatically" : "explicitly"); + "All requests complete, committing transaction", "IDBTransaction commit", + LoggingSerialNumber(), requestSerialNumber); 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 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 mAbortedByScript; bool mNotedActiveTransaction; - FlippedOnce mSentCommitOrAbort; #ifdef DEBUG + FlippedOnce mSentCommitOrAbort; FlippedOnce mFiredCompleteOrAbort; - FlippedOnce mWasExplicitlyCommitted; #endif public: @@ -204,10 +205,6 @@ class IDBTransaction final return NS_FAILED(mAbortCode); } -#ifdef DEBUG - bool WasExplicitlyCommitted() const { return mWasExplicitlyCommitted; } -#endif - template 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 aGivenProto) override; @@ -362,7 +357,7 @@ class IDBTransaction final void AbortInternal(nsresult aAbortCode, RefPtr aError); - void SendCommit(bool aAutoCommit); + void SendCommit(); void SendAbort(nsresult aResultCode); diff --git a/testing/web-platform/meta/IndexedDB/idb-explicit-commit-throw.any.js.ini b/testing/web-platform/meta/IndexedDB/idb-explicit-commit-throw.any.js.ini new file mode 100644 index 000000000000..bfe98bc05d64 --- /dev/null +++ b/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 + diff --git a/testing/web-platform/meta/IndexedDB/idb-explicit-commit.any.js.ini b/testing/web-platform/meta/IndexedDB/idb-explicit-commit.any.js.ini new file mode 100644 index 000000000000..d32bd24f6039 --- /dev/null +++ b/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 + diff --git a/testing/web-platform/tests/IndexedDB/idb-explicit-commit.any.js b/testing/web-platform/tests/IndexedDB/idb-explicit-commit.any.js index 9fedb1c09928..2acd904ea6ab 100644 --- a/testing/web-platform/tests/IndexedDB/idb-explicit-commit.any.js +++ b/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'); - txn3.objectStore('not_books').getAllKeys(); + const txn3 = db.transaction(['not_books'], 'readwrite'); + txn3.objectStore('not_books').put({'title': 'not_title'}, 'key'); txn3.oncomplete = function() { releaseTxnFunction(); }