From 4ae1cba5204bef4ac4e8692a986cab3ffe019964 Mon Sep 17 00:00:00 2001 From: Ponchale Date: Fri, 25 Oct 2024 13:09:58 -0500 Subject: [PATCH] Bug 1914599 - Fix a potential race between OpenDatabaseOp and GetDatabasesOp; r=dom-storage-reviewers,jari a=RyanVM --- dom/indexedDB/ActorsParent.cpp | 111 +++++++++++++++++++++++++--- dom/indexedDB/DatabaseFileManager.h | 6 +- dom/indexedDB/SchemaUpgrades.cpp | 3 +- 3 files changed, 104 insertions(+), 16 deletions(-) diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp index 838d01f5c01..fd38d62da21 100644 --- a/dom/indexedDB/ActorsParent.cpp +++ b/dom/indexedDB/ActorsParent.cpp @@ -2962,10 +2962,15 @@ class FactoryOp // Waiting to do/doing work on the "work thread". This involves waiting for // the VersionChangeOp (OpenDatabaseOp and DeleteDatabaseOp each have a - // different implementation) to do its work. Eventually the state will - // transition to SendingResults. + // different implementation) to do its work. If the VersionChangeOp is + // OpenDatabaseOp and it succeeded then the next state is + // DatabaseWorkVersionUpdate. Otherwise the next step is SendingResults. DatabaseWorkVersionChange, + // Waiting to do/doing finalization work on the QuotaManager IO thread. + // Eventually the state will transition to SendingResults. + DatabaseWorkVersionUpdate, + // Waiting to send/sending results on the PBackground thread. Next step is // Completed. SendingResults, @@ -3087,6 +3092,8 @@ class FactoryOp virtual nsresult DispatchToWorkThread() = 0; + virtual nsresult DoVersionUpdate() = 0; + // Should only be called by Run(). virtual void SendResults() = 0; @@ -3166,6 +3173,8 @@ class OpenDatabaseOp final : public FactoryRequestOp { // cycles. VersionChangeOp* mVersionChangeOp; + MoveOnlyFunction mCompleteCallback; + uint32_t mTelemetryId; public: @@ -3210,6 +3219,8 @@ class OpenDatabaseOp final : public FactoryRequestOp { nsresult DispatchToWorkThread() override; + nsresult DoVersionUpdate() override; + void SendResults() override; static nsresult UpdateLocaleAwareIndex(mozIStorageConnection& aConnection, @@ -3281,6 +3292,8 @@ class DeleteDatabaseOp final : public FactoryRequestOp { nsresult DispatchToWorkThread() override; + nsresult DoVersionUpdate() override; + void SendResults() override; }; @@ -3341,6 +3354,8 @@ class GetDatabasesOp final : public FactoryOp { nsresult DispatchToWorkThread() override; + nsresult DoVersionUpdate() override; + void SendResults() override; }; @@ -10699,6 +10714,7 @@ void VersionChangeTransaction::UpdateMetadata(nsresult aResult) { void VersionChangeTransaction::SendCompleteNotification(nsresult aResult) { AssertIsOnBackgroundThread(); MOZ_ASSERT(mOpenDatabaseOp); + MOZ_ASSERT(!mOpenDatabaseOp->mCompleteCallback); MOZ_ASSERT_IF(!mActorWasAlive, mOpenDatabaseOp->HasFailed()); MOZ_ASSERT_IF(!mActorWasAlive, mOpenDatabaseOp->mState > OpenDatabaseOp::State::SendingResults); @@ -10709,21 +10725,40 @@ void VersionChangeTransaction::SendCompleteNotification(nsresult aResult) { return; } + openDatabaseOp->mCompleteCallback = + [self = SafeRefPtr{this, AcquireStrongRefFromRawPtr{}}, aResult]() { + if (!self->IsActorDestroyed()) { + Unused << self->SendComplete(aResult); + } + }; + + auto handleError = [openDatabaseOp](const nsresult rv) { + openDatabaseOp->SetFailureCodeIfUnset(NS_ERROR_DOM_INDEXEDDB_ABORT_ERR); + + openDatabaseOp->mState = OpenDatabaseOp::State::SendingResults; + + MOZ_ALWAYS_SUCCEEDS(openDatabaseOp->Run()); + }; + if (NS_FAILED(aResult)) { // 3.3.1 Opening a database: // "If the upgrade transaction was aborted, run the steps for closing a // database connection with connection, create and return a new AbortError // exception and abort these steps." - openDatabaseOp->SetFailureCodeIfUnset(NS_ERROR_DOM_INDEXEDDB_ABORT_ERR); + handleError(NS_ERROR_DOM_INDEXEDDB_ABORT_ERR); + return; } - openDatabaseOp->mState = OpenDatabaseOp::State::SendingResults; + openDatabaseOp->mState = OpenDatabaseOp::State::DatabaseWorkVersionUpdate; - if (!IsActorDestroyed()) { - Unused << SendComplete(aResult); - } + QuotaManager* const quotaManager = QuotaManager::Get(); + MOZ_ASSERT(quotaManager); - MOZ_ALWAYS_SUCCEEDS(openDatabaseOp->Run()); + QM_TRY(MOZ_TO_RESULT(quotaManager->IOThread()->Dispatch(openDatabaseOp, + NS_DISPATCH_NORMAL)) + .mapErr( + [](const auto) { return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; }), + QM_VOID, handleError); } void VersionChangeTransaction::ActorDestroy(ActorDestroyReason aWhy) { @@ -11536,7 +11571,20 @@ DatabaseFileManager::DatabaseFileManager( mEnforcingQuota(aEnforcingQuota), mIsInPrivateBrowsingMode(aIsInPrivateBrowsingMode) {} +uint64_t DatabaseFileManager::DatabaseVersion() const { + AssertIsOnIOThread(); + + return mDatabaseVersion; +} + +void DatabaseFileManager::UpdateDatabaseVersion(uint64_t aDatabaseVersion) { + AssertIsOnIOThread(); + + mDatabaseVersion = aDatabaseVersion; +} + nsresult DatabaseFileManager::Init(nsIFile* aDirectory, + const uint64_t aDatabaseVersion, mozIStorageConnection& aConnection) { AssertIsOnIOThread(); MOZ_ASSERT(aDirectory); @@ -11571,6 +11619,8 @@ nsresult DatabaseFileManager::Init(nsIFile* aDirectory, mJournalDirectoryPath.init(std::move(path)); } + mDatabaseVersion = aDatabaseVersion; + QM_TRY_INSPECT(const auto& stmt, MOZ_TO_RESULT_INVOKE_MEMBER_TYPED( nsCOMPtr, aConnection, @@ -14930,6 +14980,10 @@ FactoryOp::Run() { QM_WARNONLY_TRY(MOZ_TO_RESULT(DispatchToWorkThread()), handleError); break; + case State::DatabaseWorkVersionUpdate: + QM_WARNONLY_TRY(MOZ_TO_RESULT(DoVersionUpdate()), handleError); + break; + case State::SendingResults: SendResults(); break; @@ -15168,7 +15222,8 @@ nsresult OpenDatabaseOp::DoDatabaseWork() { NS_ERROR_DOM_INDEXEDDB_VERSION_ERR); if (!fileManager->Initialized()) { - QM_TRY(MOZ_TO_RESULT(fileManager->Init(fmDirectory, *connection))); + QM_TRY(MOZ_TO_RESULT(fileManager->Init( + fmDirectory, mMetadata->mCommonMetadata.version(), *connection))); idm->AddFileManager(fileManager.clonePtr()); } @@ -15706,12 +15761,40 @@ nsresult OpenDatabaseOp::SendUpgradeNeeded() { return NS_OK; } +nsresult OpenDatabaseOp::DoVersionUpdate() { + AssertIsOnIOThread(); + MOZ_ASSERT(mState == State::DatabaseWorkVersionUpdate); + MOZ_ASSERT(!HasFailed()); + + AUTO_PROFILER_LABEL("OpenDatabaseOp::DoVersionUpdate", DOM); + + if (NS_WARN_IF(QuotaClient::IsShuttingDownOnNonBackgroundThread()) || + !OperationMayProceed()) { + IDB_REPORT_INTERNAL_ERR(); + return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; + } + + mFileManager->UpdateDatabaseVersion(mRequestedVersion); + + mState = State::SendingResults; + + QM_TRY(MOZ_TO_RESULT( + DispatchThisAfterProcessingCurrentEvent(mOwningEventTarget))); + + return NS_OK; +} + void OpenDatabaseOp::SendResults() { AssertIsOnOwningThread(); MOZ_ASSERT(mState == State::SendingResults); MOZ_ASSERT(mMaybeBlockedDatabases.IsEmpty()); MOZ_ASSERT_IF(!HasFailed(), !mVersionChangeTransaction); + if (mCompleteCallback) { + auto completeCallback = std::move(mCompleteCallback); + completeCallback(); + } + DebugOnly info = nullptr; MOZ_ASSERT_IF(mDatabaseId.isSome() && gLiveDatabaseHashtable && gLiveDatabaseHashtable->Get(mDatabaseId.ref(), &info), @@ -15734,8 +15817,6 @@ void OpenDatabaseOp::SendResults() { // need to update the version in our metadata. mMetadata->mCommonMetadata.version() = mRequestedVersion; - mFileManager->UpdateDatabaseVersion(mRequestedVersion); - nsresult rv = EnsureDatabaseActorIsAlive(); if (NS_SUCCEEDED(rv)) { // We successfully opened a database so use its actor as the success @@ -16356,6 +16437,10 @@ void DeleteDatabaseOp::SendBlockedNotification() { } } +nsresult DeleteDatabaseOp::DoVersionUpdate() { + MOZ_CRASH("Not implemented because this should be unreachable."); +} + void DeleteDatabaseOp::SendResults() { AssertIsOnOwningThread(); MOZ_ASSERT(mState == State::SendingResults); @@ -16726,6 +16811,10 @@ nsresult GetDatabasesOp::DispatchToWorkThread() { MOZ_CRASH("Not implemented because this should be unreachable."); } +nsresult GetDatabasesOp::DoVersionUpdate() { + MOZ_CRASH("Not implemented because this should be unreachable."); +} + void GetDatabasesOp::SendResults() { AssertIsOnOwningThread(); MOZ_ASSERT(mState == State::SendingResults); diff --git a/dom/indexedDB/DatabaseFileManager.h b/dom/indexedDB/DatabaseFileManager.h index fdfea06ba11..c8d6c65f6c3 100644 --- a/dom/indexedDB/DatabaseFileManager.h +++ b/dom/indexedDB/DatabaseFileManager.h @@ -89,11 +89,9 @@ class DatabaseFileManager final const nsAString& DatabaseFilePath() const { return mDatabaseFilePath; } - uint64_t DatabaseVersion() const { return mDatabaseVersion; } + uint64_t DatabaseVersion() const; - void UpdateDatabaseVersion(uint64_t aDatabaseVersion) { - mDatabaseVersion = aDatabaseVersion; - } + void UpdateDatabaseVersion(uint64_t aDatabaseVersion); IndexedDBCipherKeyManager& MutableCipherKeyManagerRef() const { MOZ_ASSERT(mIsInPrivateBrowsingMode); diff --git a/dom/indexedDB/SchemaUpgrades.cpp b/dom/indexedDB/SchemaUpgrades.cpp index 17230e247f5..a4e307dcb8a 100644 --- a/dom/indexedDB/SchemaUpgrades.cpp +++ b/dom/indexedDB/SchemaUpgrades.cpp @@ -2866,7 +2866,8 @@ nsresult UpgradeFileIdsFunction::Init(nsIFile* aFMDirectory, /* aDatabaseFilePath */ u""_ns, /* aEnforcingQuota */ false, /* aIsInPrivateBrowsingMode */ false); - nsresult rv = fileManager->Init(aFMDirectory, aConnection); + nsresult rv = + fileManager->Init(aFMDirectory, /* aDatabaseVersion */ 0, aConnection); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }