Bug 1577202 - Defer closing of a database while CreateFileOp's are pending. r=janv draft
authorSimon Giesecke <sgiesecke@mozilla.com>
Wed, 04 Dec 2019 12:46:04 +0000
changeset 2514392 929b85763af4314ad853817585a57c32ed7c964d
parent 2514391 a12c414074043283b45ea59ef925a9d7dfe65613
child 2514393 18057afc752621c3431e722417657ecd4076cfa7
push id459904
push userreviewbot
push dateWed, 04 Dec 2019 12:46:21 +0000
treeherdertry@18057afc7526 [default view] [failures only]
reviewersjanv
bugs1577202
milestone73.0a1
Bug 1577202 - Defer closing of a database while CreateFileOp's are pending. r=janv Differential Revision: https://phabricator.services.mozilla.com/D44683 Differential Diff: PHID-DIFF-is7vzmb5jn2ggqbojdld
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -5793,16 +5793,17 @@ class Database final
   RefPtr<DatabaseConnection> mConnection;
   const PrincipalInfo mPrincipalInfo;
   const Maybe<ContentParentId> mOptionalContentParentId;
   const nsCString mGroup;
   const nsCString mOrigin;
   const nsCString mId;
   const nsString mFilePath;
   uint32_t mActiveMutableFileCount;
+  uint32_t mPendingCreateFileOpCount;
   int64_t mDirectoryLockId;
   const uint32_t mTelemetryId;
   const PersistenceType mPersistenceType;
   const bool mFileHandleDisabled;
   const bool mChromeWriteAccessAllowed;
   bool mClosed;
   bool mInvalidated;
   bool mActorWasAlive;
@@ -5888,16 +5889,20 @@ class Database final
   bool RegisterMutableFile(MutableFile* aMutableFile);
 
   void UnregisterMutableFile(MutableFile* aMutableFile);
 
   void NoteActiveMutableFile();
 
   void NoteInactiveMutableFile();
 
+  void NotePendingCreateFileOp();
+
+  void NoteCompletedCreateFileOp();
+
   void SetActorAlive();
 
   void MapBlob(const IPCBlob& aIPCBlob, FileInfo* aFileInfo);
 
   bool IsActorAlive() const {
     AssertIsOnBackgroundThread();
 
     return mActorWasAlive && !mActorDestroyed;
@@ -12862,16 +12867,17 @@ Database::Database(Factory* aFactory, co
       mDirectoryLock(std::move(aDirectoryLock)),
       mPrincipalInfo(aPrincipalInfo),
       mOptionalContentParentId(aOptionalContentParentId),
       mGroup(aGroup),
       mOrigin(aOrigin),
       mId(aMetadata->mDatabaseId),
       mFilePath(aMetadata->mFilePath),
       mActiveMutableFileCount(0),
+      mPendingCreateFileOpCount(0),
       mTelemetryId(aTelemetryId),
       mPersistenceType(aMetadata->mCommonMetadata.persistenceType()),
       mFileHandleDisabled(aFileHandleDisabled),
       mChromeWriteAccessAllowed(aChromeWriteAccessAllowed),
       mClosed(false),
       mInvalidated(false),
       mActorWasAlive(false),
       mActorDestroyed(false),
@@ -13030,28 +13036,18 @@ void Database::UnregisterTransaction(Tra
   mTransactions.RemoveEntry(aTransaction);
 
   MaybeCloseConnection();
 }
 
 bool Database::RegisterMutableFile(MutableFile* aMutableFile) {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aMutableFile);
-
-  // The lock might have been cleared already. See Bug 1577202.
-  //
-  // TODO: This might be reverted back to an assertion again if the behaviour of
-  // Close is changed to delay in case of a pending CreateFileOp. However, since
-  // support for mutable files is planned to be removed, it might not be
-  // worthwhile to implement this.
-  if (!mDirectoryLock) {
-    return false;
-  }
-
   MOZ_ASSERT(!mMutableFiles.GetEntry(aMutableFile));
+  MOZ_ASSERT(mDirectoryLock);
 
   if (NS_WARN_IF(!mMutableFiles.PutEntry(aMutableFile, fallible))) {
     return false;
   }
 
   return true;
 }
 
@@ -13075,16 +13071,33 @@ void Database::NoteInactiveMutableFile()
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(mActiveMutableFileCount > 0);
 
   --mActiveMutableFileCount;
 
   MaybeCloseConnection();
 }
 
+void Database::NotePendingCreateFileOp() {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mDirectoryLock);
+  MOZ_ASSERT(mPendingCreateFileOpCount < UINT32_MAX);
+
+  ++mPendingCreateFileOpCount;
+}
+
+void Database::NoteCompletedCreateFileOp() {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mPendingCreateFileOpCount > 0);
+
+  --mPendingCreateFileOpCount;
+
+  MaybeCloseConnection();
+}
+
 void Database::SetActorAlive() {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(!mActorWasAlive);
   MOZ_ASSERT(!mActorDestroyed);
 
   mActorWasAlive = true;
 
   // This reference will be absorbed by IPDL and released when the actor is
@@ -13220,33 +13233,34 @@ bool Database::CloseInternal() {
   MaybeCloseConnection();
 
   return true;
 }
 
 void Database::MaybeCloseConnection() {
   AssertIsOnBackgroundThread();
 
-  if (!mTransactions.Count() && !mActiveMutableFileCount && IsClosed() &&
-      mDirectoryLock) {
+  if (!mTransactions.Count() && !mActiveMutableFileCount &&
+      !mPendingCreateFileOpCount && IsClosed() && mDirectoryLock) {
     nsCOMPtr<nsIRunnable> callback =
         NewRunnableMethod("dom::indexedDB::Database::ConnectionClosedCallback",
                           this, &Database::ConnectionClosedCallback);
 
     RefPtr<WaitForTransactionsHelper> helper =
         new WaitForTransactionsHelper(Id(), callback);
     helper->WaitForTransactions();
   }
 }
 
 void Database::ConnectionClosedCallback() {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(mClosed);
   MOZ_ASSERT(!mTransactions.Count());
   MOZ_ASSERT(!mActiveMutableFileCount);
+  MOZ_ASSERT(!mPendingCreateFileOpCount);
 
   mDirectoryLock = nullptr;
 
   CleanupMetadata();
 
   UnmapAllBlobs();
 
   if (IsInvalidated() && IsActorAlive()) {
@@ -13349,16 +13363,18 @@ bool Database::DeallocPBackgroundIDBData
 }
 
 PBackgroundIDBDatabaseRequestParent*
 Database::AllocPBackgroundIDBDatabaseRequestParent(
     const DatabaseRequestParams& aParams) {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aParams.type() != DatabaseRequestParams::T__None);
 
+  // TODO: Check here that the database has not been closed?
+
 #ifdef DEBUG
   // Always verify parameters in DEBUG builds!
   bool trustParams = false;
 #else
   PBackgroundParent* backgroundActor = GetBackgroundParent();
   MOZ_ASSERT(backgroundActor);
 
   bool trustParams = !BackgroundParent::IsOtherProcessActor(backgroundActor);
@@ -13369,16 +13385,18 @@ Database::AllocPBackgroundIDBDatabaseReq
     return nullptr;
   }
 
   RefPtr<DatabaseOp> actor;
 
   switch (aParams.type()) {
     case DatabaseRequestParams::TCreateFileParams: {
       actor = new CreateFileOp(this, aParams);
+
+      NotePendingCreateFileOp();
       break;
     }
 
     default:
       MOZ_CRASH("Should never get here!");
   }
 
   MOZ_ASSERT(actor);
@@ -22882,16 +22900,22 @@ void CreateFileOp::SendResults() {
     } else {
       response = ClampResultCode(mResultCode);
     }
 
     Unused << PBackgroundIDBDatabaseRequestParent::Send__delete__(this,
                                                                   response);
   }
 
+  // XXX: "Complete" in CompletedCreateFileOp and State::Completed mean
+  // different things, and State::Completed should only be reached after
+  // notifying the database. Either should probably be renamed to avoid
+  // confusion.
+  mDatabase->NoteCompletedCreateFileOp();
+
   mState = State::Completed;
 }
 
 nsresult VersionChangeTransactionOp::SendSuccessResult() {
   AssertIsOnOwningThread();
 
   // Nothing to send here, the API assumes that this request always succeeds.
   return NS_OK;