Bug 1609957 - Fix race condition in determining the last request before commit. r=dom-workers-and-storage-reviewers,janv

Differential Revision: https://phabricator.services.mozilla.com/D60642

--HG--
extra : moz-landing-system : lando

dom/indexedDB/ActorsParent.cpp

@@ -6306,7 +6306,9 @@ class TransactionBase {
   FlippedOnce<false> mCommitOrAbortReceived;
   FlippedOnce<false> mCommittedOrAborted;
   FlippedOnce<false> mForceAborted;
-  bool mHasFailedRequest = false;
+  InitializedOnce<const Maybe<int64_t>, LazyInit::Allow>
+      mLastRequestBeforeCommit;
+  Maybe<int64_t> mLastFailedRequest;
 
  public:
   void AssertIsOnConnectionThread() const {
@@ -6397,7 +6399,7 @@ class TransactionBase {
 
   void NoteActiveRequest();
 
-  void NoteFinishedRequest(nsresult aResultCode);
+  void NoteFinishedRequest(int64_t aRequestId, nsresult aResultCode);
 
   void Invalidate();
 
@@ -6417,11 +6419,11 @@ class TransactionBase {
   void FakeActorDestroyed() { mActorDestroyed.EnsureFlipped(); }
 #endif
 
-  bool RecvCommit();
+  bool RecvCommit(Maybe<int64_t> aLastRequest);
 
   bool RecvAbort(nsresult aResultCode);
 
-  void MaybeCommitOrAbort(const nsresult aResultCode) {
+  void MaybeCommitOrAbort() {
     AssertIsOnBackgroundThread();
 
     // If we've already committed or aborted then there's nothing else to do.
@@ -6442,13 +6444,6 @@ class TransactionBase {
       return;
     }
 
-    // Note failed request, but ignore requests that failed before we were
-    // committing (cf. https://w3c.github.io/IndexedDB/#async-execute-request
-    // step 5.3 vs. 5.4).
-    if (NS_FAILED(aResultCode)) {
-      mHasFailedRequest = true;
-    }
-
     CommitOrAbort();
   }
 
@@ -6548,7 +6543,8 @@ class NormalTransaction final : public TransactionBase,
 
   mozilla::ipc::IPCResult RecvDeleteMe() override;
 
-  mozilla::ipc::IPCResult RecvCommit() override;
+  mozilla::ipc::IPCResult RecvCommit(
+      const Maybe<int64_t>& aLastRequest) override;
 
   mozilla::ipc::IPCResult RecvAbort(const nsresult& aResultCode) override;
 
@@ -6608,7 +6604,8 @@ class VersionChangeTransaction final
 
   mozilla::ipc::IPCResult RecvDeleteMe() override;
 
-  mozilla::ipc::IPCResult RecvCommit() override;
+  mozilla::ipc::IPCResult RecvCommit(
+      const Maybe<int64_t>& aLastRequest) override;
 
   mozilla::ipc::IPCResult RecvAbort(const nsresult& aResultCode) override;
 
@@ -14081,10 +14078,10 @@ void TransactionBase::Abort(nsresult aResultCode, bool aForce) {
     mForceAborted.EnsureFlipped();
   }
 
-  MaybeCommitOrAbort(mResultCode);
+  MaybeCommitOrAbort();
 }
 
-bool TransactionBase::RecvCommit() {
+bool TransactionBase::RecvCommit(const Maybe<int64_t> aLastRequest) {
   AssertIsOnBackgroundThread();
 
   if (NS_WARN_IF(mCommitOrAbortReceived)) {
@@ -14093,8 +14090,9 @@ bool TransactionBase::RecvCommit() {
   }
 
   mCommitOrAbortReceived.Flip();
+  mLastRequestBeforeCommit.init(aLastRequest);
 
-  MaybeCommitOrAbort(NS_OK);
+  MaybeCommitOrAbort();
   return true;
 }
 
@@ -14132,6 +14130,17 @@ void TransactionBase::CommitOrAbort() {
     return;
   }
 
+  // In case of a failed request that was started after committing was
+  // initiated, abort (cf.
+  // https://w3c.github.io/IndexedDB/#async-execute-request step 5.3 vs. 5.4).
+  // Note this can only happen here when we are committing explicitly, otherwise
+  // the decision is made by the child.
+  if (NS_SUCCEEDED(mResultCode) && mLastFailedRequest &&
+      *mLastRequestBeforeCommit &&
+      *mLastFailedRequest >= **mLastRequestBeforeCommit) {
+    mResultCode = NS_ERROR_DOM_INDEXEDDB_ABORT_ERR;
+  }
+
   RefPtr<CommitOp> commitOp = new CommitOp(this, ClampResultCode(mResultCode));
 
   gConnectionPool->Finish(TransactionId(), commitOp);
@@ -14641,13 +14650,18 @@ void TransactionBase::NoteActiveRequest() {
   mActiveRequestCount++;
 }
 
-void TransactionBase::NoteFinishedRequest(const nsresult aResultCode) {
+void TransactionBase::NoteFinishedRequest(const int64_t aRequestId,
+                                          const nsresult aResultCode) {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(mActiveRequestCount);
 
   mActiveRequestCount--;
 
-  MaybeCommitOrAbort(aResultCode);
+  if (NS_FAILED(aResultCode)) {
+    mLastFailedRequest = Some(aRequestId);
+  }
+
+  MaybeCommitOrAbort();
 }
 
 void TransactionBase::Invalidate() {
@@ -14923,7 +14937,7 @@ void NormalTransaction::ActorDestroy(ActorDestroyReason aWhy) {
 
     mForceAborted.EnsureFlipped();
 
-    MaybeCommitOrAbort(mResultCode);
+    MaybeCommitOrAbort();
   }
 }
 
@@ -14938,10 +14952,11 @@ mozilla::ipc::IPCResult NormalTransaction::RecvDeleteMe() {
   return IPC_OK();
 }
 
-mozilla::ipc::IPCResult NormalTransaction::RecvCommit() {
+mozilla::ipc::IPCResult NormalTransaction::RecvCommit(
+    const Maybe<int64_t>& aLastRequest) {
   AssertIsOnBackgroundThread();
 
-  if (!TransactionBase::RecvCommit()) {
+  if (!TransactionBase::RecvCommit(aLastRequest)) {
     return IPC_FAIL_NO_REASON(this);
   }
   return IPC_OK();
@@ -15184,7 +15199,7 @@ void VersionChangeTransaction::ActorDestroy(ActorDestroyReason aWhy) {
 
     mForceAborted.EnsureFlipped();
 
-    MaybeCommitOrAbort(mResultCode);
+    MaybeCommitOrAbort();
   }
 }
 
@@ -15199,10 +15214,11 @@ mozilla::ipc::IPCResult VersionChangeTransaction::RecvDeleteMe() {
   return IPC_OK();
 }
 
-mozilla::ipc::IPCResult VersionChangeTransaction::RecvCommit() {
+mozilla::ipc::IPCResult VersionChangeTransaction::RecvCommit(
+    const Maybe<int64_t>& aLastRequest) {
   AssertIsOnBackgroundThread();
 
-  if (!TransactionBase::RecvCommit()) {
+  if (!TransactionBase::RecvCommit(aLastRequest)) {
     return IPC_FAIL_NO_REASON(this);
   }
   return IPC_OK();
@@ -22743,7 +22759,7 @@ void TransactionDatabaseOperationBase::SendPreprocessInfoOrResults(
     mWaitingForContinue = true;
   } else {
     if (mLoggingSerialNumber) {
-      (*mTransaction)->NoteFinishedRequest(ResultCode());
+      (*mTransaction)->NoteFinishedRequest(mLoggingSerialNumber, ResultCode());
     }
 
     Cleanup();
@@ -22926,10 +22942,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

@@ -386,7 +386,35 @@ void IDBTransaction::SendCommit(const bool aAutoCommit) {
       LoggingSerialNumber(), requestSerialNumber,
       aAutoCommit ? "automatically" : "explicitly");
 
-  DoWithTransactionChild([](auto& actor) { actor.SendCommit(); });
+  const auto lastRequestSerialNumber =
+      [this, aAutoCommit,
+       requestSerialNumber]() -> Maybe<decltype(requestSerialNumber)> {
+    if (aAutoCommit) {
+      return Nothing();
+    }
+
+    // In case of an explicit commit, we need to note the serial number of the
+    // last request to check if a request submitted before the commit request
+    // failed. If we are currently in an event handler for a request on this
+    // transaction, ignore this request. This is used to synchronize the
+    // transaction's committing state with the parent side, to abort the
+    // transaction in case of a request resulting in an error (see
+    // https://w3c.github.io/IndexedDB/#async-execute-request, step 5.3.). With
+    // automatic commit, this is not necessary, as the transaction's state will
+    // only be set to committing after the last request completed.
+    const bool dispatchingEventForThisTransaction =
+        BackgroundChildImpl::GetThreadLocalForCurrentThread()
+            ->mIndexedDBThreadLocal->GetCurrentTransaction() == this;
+
+    return Some(requestSerialNumber
+                    ? (requestSerialNumber -
+                       (dispatchingEventForThisTransaction ? 0 : 1))
+                    : 0);
+  }();
+
+  DoWithTransactionChild([lastRequestSerialNumber](auto& actor) {
+    actor.SendCommit(lastRequestSerialNumber);
+  });
 
   mSentCommitOrAbort.Flip();
 }

dom/indexedDB/PBackgroundIDBTransaction.ipdl

@@ -14,8 +14,7 @@ namespace mozilla {
 namespace dom {
 namespace indexedDB {
 
-protocol PBackgroundIDBTransaction
-{
+protocol PBackgroundIDBTransaction {
   manager PBackgroundIDBDatabase;
 
   manages PBackgroundIDBCursor;
@@ -24,7 +23,13 @@ protocol PBackgroundIDBTransaction
 parent:
   async DeleteMe();
 
-  async Commit();
+  // lastRequest is used with explicit commit to synchronize the
+  // transaction's committing state with the parent side, to abort the
+  // transaction in case of a request resulting in an error (see
+  // https://w3c.github.io/IndexedDB/#async-execute-request, step 5.3.). With
+  // automatic commit, this is not necessary, as the transaction's state will
+  // only be set to committing after the last request completed.
+  async Commit(int64_t? lastRequest);
   async Abort(nsresult resultCode);
 
   async PBackgroundIDBCursor(OpenCursorParams params);
@@ -37,6 +42,6 @@ child:
   async Complete(nsresult result);
 };
 
-} // namespace indexedDB
-} // namespace dom
-} // namespace mozilla
+}  // namespace indexedDB
+}  // namespace dom
+}  // namespace mozilla

dom/indexedDB/PBackgroundIDBVersionChangeTransaction.ipdl

@@ -7,10 +7,10 @@ include protocol PBackgroundIDBDatabase;
 include protocol PBackgroundIDBDatabaseFile;
 include protocol PBackgroundIDBRequest;
 include protocol PBackgroundMutableFile;
-include protocol PChildToParentStream; // FIXME: bug 792908
-include protocol PFileDescriptorSet; // FIXME: bug 792908
-include protocol PIPCBlobInputStream; // FIXME: bug 792908
-include protocol PParentToChildStream; // FIXME: bug 792908
+include protocol PChildToParentStream;  // FIXME: bug 792908
+include protocol PFileDescriptorSet;    // FIXME: bug 792908
+include protocol PIPCBlobInputStream;   // FIXME: bug 792908
+include protocol PParentToChildStream;  // FIXME: bug 792908
 
 include PBackgroundIDBSharedTypes;
 
@@ -18,8 +18,7 @@ namespace mozilla {
 namespace dom {
 namespace indexedDB {
 
-protocol PBackgroundIDBVersionChangeTransaction
-{
+protocol PBackgroundIDBVersionChangeTransaction {
   manager PBackgroundIDBDatabase;
 
   manages PBackgroundIDBCursor;
@@ -28,21 +27,16 @@ protocol PBackgroundIDBVersionChangeTransaction
 parent:
   async DeleteMe();
 
-  async Commit();
+  async Commit(int64_t? lastRequest);
   async Abort(nsresult resultCode);
 
   async CreateObjectStore(ObjectStoreMetadata metadata);
   async DeleteObjectStore(int64_t objectStoreId);
-  async RenameObjectStore(int64_t objectStoreId,
-                          nsString name);
+  async RenameObjectStore(int64_t objectStoreId, nsString name);
 
-  async CreateIndex(int64_t objectStoreId,
-                    IndexMetadata metadata);
-  async DeleteIndex(int64_t objectStoreId,
-                    int64_t indexId);
-  async RenameIndex(int64_t objectStoreId,
-                    int64_t indexId,
-                    nsString name);
+  async CreateIndex(int64_t objectStoreId, IndexMetadata metadata);
+  async DeleteIndex(int64_t objectStoreId, int64_t indexId);
+  async RenameIndex(int64_t objectStoreId, int64_t indexId, nsString name);
 
   async PBackgroundIDBCursor(OpenCursorParams params);
 
@@ -54,6 +48,6 @@ child:
   async Complete(nsresult result);
 };
 
-} // namespace indexedDB
-} // namespace dom
-} // namespace mozilla
+}  // namespace indexedDB
+}  // namespace dom
+}  // namespace mozilla