From 0dd630db14cd22f54cb3f89d0de6f6ced4a76bca Mon Sep 17 00:00:00 2001 From: Jan Varga Date: Tue, 16 Jul 2024 10:43:40 +0000 Subject: [PATCH] Bug 1906041 - Rename Ensure(Persistent|Temporary)OriginIsInitialized to Ensure(Persistent|Temporary)OriginIsInitializedInternal; r=dom-storage-reviewers,jari One of the goals of the asynchronous temporary storage initialization is to call Ensure(Persistent|Temporary)OriginIsInitialized only from Initialize(Persistent|Temporary)OriginOp. Calling from other places including quota clients will be disallowed by changing the method to a private method. The private nature of the method should be emphasized by adding the Internal suffix. Differential Revision: https://phabricator.services.mozilla.com/D192150 --- dom/cache/Context.cpp | 10 +++--- .../datamodel/FileSystemFileManager.cpp | 2 +- dom/indexedDB/ActorsParent.cpp | 24 +++++++------- dom/localstorage/ActorsParent.cpp | 24 +++++++------- dom/quota/ActorsParent.cpp | 33 ++++++++++--------- dom/quota/OriginOperations.cpp | 26 ++++++++------- dom/quota/QuotaManager.h | 14 ++++---- dom/quota/QuotaUsageRequestBase.cpp | 3 +- dom/quota/test/gtest/TestFileOutputStream.cpp | 4 +-- .../test/gtest/TestStorageConnection.cpp | 2 +- dom/simpledb/ActorsParent.cpp | 9 ++--- 11 files changed, 81 insertions(+), 70 deletions(-) diff --git a/dom/cache/Context.cpp b/dom/cache/Context.cpp index 91aa8642c5ba..3cb99b6878a0 100644 --- a/dom/cache/Context.cpp +++ b/dom/cache/Context.cpp @@ -403,11 +403,11 @@ Context::QuotaInitRunnable::Run() { QM_TRY(MOZ_TO_RESULT( quotaManager->EnsureTemporaryStorageIsInitializedInternal())); - QM_TRY_UNWRAP( - mDirectoryMetadata->mDir, - quotaManager - ->EnsureTemporaryOriginIsInitialized(*mDirectoryMetadata) - .map([](const auto& res) { return res.first; })); + QM_TRY_UNWRAP(mDirectoryMetadata->mDir, + quotaManager + ->EnsureTemporaryOriginIsInitializedInternal( + *mDirectoryMetadata) + .map([](const auto& res) { return res.first; })); auto* cacheQuotaClient = CacheQuotaClient::Get(); MOZ_DIAGNOSTIC_ASSERT(cacheQuotaClient); diff --git a/dom/fs/parent/datamodel/FileSystemFileManager.cpp b/dom/fs/parent/datamodel/FileSystemFileManager.cpp index 22261f1c8b47..b21c88a10ebd 100644 --- a/dom/fs/parent/datamodel/FileSystemFileManager.cpp +++ b/dom/fs/parent/datamodel/FileSystemFileManager.cpp @@ -175,7 +175,7 @@ nsresult EnsureFileSystemDirectory( QM_TRY_INSPECT( const auto& fileSystemDirectory, - quotaManager->EnsureTemporaryOriginIsInitialized(aOriginMetadata) + quotaManager->EnsureTemporaryOriginIsInitializedInternal(aOriginMetadata) .map([](const auto& aPair) { return aPair.first; })); QM_TRY(QM_TO_RESULT(fileSystemDirectory->AppendRelativePath( diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp index eadc74874524..9b508621a115 100644 --- a/dom/indexedDB/ActorsParent.cpp +++ b/dom/indexedDB/ActorsParent.cpp @@ -13240,14 +13240,15 @@ nsresult Maintenance::DirectoryWork() { QM_TRY_UNWRAP( const DebugOnly created, - quotaManager->EnsurePersistentOriginIsInitialized(metadata) + quotaManager + ->EnsurePersistentOriginIsInitializedInternal(metadata) .map([](const auto& res) { return res.second; }), // Not much we can do here... Ok{}); // We found this origin directory by traversing the repository, - // so EnsurePersistentOriginIsInitialized shouldn't report that - // a new directory has been created. + // so EnsurePersistentOriginIsInitializedInternal shouldn't + // report that a new directory has been created. MOZ_ASSERT(!created); } @@ -15169,14 +15170,15 @@ nsresult OpenDatabaseOp::DoDatabaseWork() { ([persistenceType, "aManager, this]() -> mozilla::Result, bool>, nsresult> { if (persistenceType == PERSISTENCE_TYPE_PERSISTENT) { - QM_TRY_RETURN(quotaManager->EnsurePersistentOriginIsInitialized( - mOriginMetadata)); + QM_TRY_RETURN( + quotaManager->EnsurePersistentOriginIsInitializedInternal( + mOriginMetadata)); } QM_TRY(MOZ_TO_RESULT( quotaManager->EnsureTemporaryStorageIsInitializedInternal())); - QM_TRY_RETURN( - quotaManager->EnsureTemporaryOriginIsInitialized(mOriginMetadata)); + QM_TRY_RETURN(quotaManager->EnsureTemporaryOriginIsInitializedInternal( + mOriginMetadata)); }() .map([](const auto& res) { return res.first; }))); @@ -16718,12 +16720,12 @@ nsresult GetDatabasesOp::DoDatabaseWork() { QM_TRY((["aManager, this]() -> mozilla::Result, bool>, nsresult> { if (mPersistenceType == PERSISTENCE_TYPE_PERSISTENT) { - QM_TRY_RETURN( - quotaManager->EnsurePersistentOriginIsInitialized(mOriginMetadata)); + QM_TRY_RETURN(quotaManager->EnsurePersistentOriginIsInitializedInternal( + mOriginMetadata)); } - QM_TRY_RETURN( - quotaManager->EnsureTemporaryOriginIsInitialized(mOriginMetadata)); + QM_TRY_RETURN(quotaManager->EnsureTemporaryOriginIsInitializedInternal( + mOriginMetadata)); }() .map([](const auto& res) { return Ok{}; }))); diff --git a/dom/localstorage/ActorsParent.cpp b/dom/localstorage/ActorsParent.cpp index e99860059f1b..401e5c46d6e5 100644 --- a/dom/localstorage/ActorsParent.cpp +++ b/dom/localstorage/ActorsParent.cpp @@ -1355,13 +1355,13 @@ class Connection final : public CachingDatabaseConnection { }; /** - * Helper to invoke EnsureTemporaryOriginIsInitialized on the QuotaManager IO - * thread from the LocalStorage connection thread when creating a database - * connection on demand. This is necessary because we attempt to defer the - * creation of the origin directory and the database until absolutely needed, - * but the directory creation and origin initialization must happen on the QM - * IO thread for invariant reasons. (We can't just use a mutex because there - * could be logic on the IO thread that also wants to deal with the same + * Helper to invoke EnsureTemporaryOriginIsInitializedInternal on the + * QuotaManager IO thread from the LocalStorage connection thread when creating + * a database connection on demand. This is necessary because we attempt to + * defer the creation of the origin directory and the database until absolutely + * needed, but the directory creation and origin initialization must happen on + * the QM IO thread for invariant reasons. (We can't just use a mutex because + * there could be logic on the IO thread that also wants to deal with the same * origin, so we need to queue a runnable and wait our turn.) */ class Connection::InitTemporaryOriginHelper final : public Runnable { @@ -4204,7 +4204,7 @@ nsresult Connection::InitTemporaryOriginHelper::RunOnIOThread() { QM_TRY_INSPECT( const auto& directoryEntry, - quotaManager->EnsureTemporaryOriginIsInitialized(mOriginMetadata) + quotaManager->EnsureTemporaryOriginIsInitializedInternal(mOriginMetadata) .map([](const auto& res) { return res.first; })); QM_TRY(MOZ_TO_RESULT(directoryEntry->GetPath(mOriginDirectoryPath))); @@ -6942,10 +6942,10 @@ nsresult PrepareDatastoreOp::DatabaseWork() { ([hasDataForMigration, "aManager, this]() -> mozilla::Result, nsresult> { if (hasDataForMigration) { - QM_TRY_RETURN( - quotaManager - ->EnsureTemporaryOriginIsInitialized(mOriginMetadata) - .map([](const auto& res) { return res.first; })); + QM_TRY_RETURN(quotaManager + ->EnsureTemporaryOriginIsInitializedInternal( + mOriginMetadata) + .map([](const auto& res) { return res.first; })); } MOZ_ASSERT(mOriginMetadata.mPersistenceType == diff --git a/dom/quota/ActorsParent.cpp b/dom/quota/ActorsParent.cpp index dcb8ebba2d87..a2bde0e6dd27 100644 --- a/dom/quota/ActorsParent.cpp +++ b/dom/quota/ActorsParent.cpp @@ -2788,12 +2788,13 @@ nsresult QuotaManager::LoadQuota() { Unused << lastAccessTimeUpdated; // We don't need to update the .metadata-v2 file on disk here, - // EnsureTemporaryOriginIsInitialized is responsible for doing that. - // We just need to use correct group and last access time before - // initializing quota for the given origin. (Note that calling + // EnsureTemporaryOriginIsInitializedInternal is responsible for + // doing that. We just need to use correct group and last access time + // before initializing quota for the given origin. (Note that calling // LoadFullOriginMetadataWithRestore below might update the group in // the metadata file, but only as a side-effect. The actual place we - // ensure consistency is in EnsureTemporaryOriginIsInitialized.) + // ensure consistency is in + // EnsureTemporaryOriginIsInitializedInternal.) if (accessed) { QM_TRY_INSPECT(const auto& directory, @@ -2811,7 +2812,8 @@ nsresult QuotaManager::LoadQuota() { // Calling LoadFullOriginMetadataWithRestore might update the group // in the metadata file, but only as a side-effect. The actual place - // we ensure consistency is in EnsureTemporaryOriginIsInitialized. + // we ensure consistency is in + // EnsureTemporaryOriginIsInitializedInternal. QM_TRY_INSPECT(const auto& metadata, LoadFullOriginMetadataWithRestore(directory)); @@ -5315,7 +5317,7 @@ RefPtr QuotaManager::CreateDirectoryLockInternal( } Result, bool>, nsresult> -QuotaManager::EnsurePersistentOriginIsInitialized( +QuotaManager::EnsurePersistentOriginIsInitializedInternal( const OriginMetadata& aOriginMetadata) { AssertIsOnIOThread(); MOZ_ASSERT(aOriginMetadata.mPersistenceType == PERSISTENCE_TYPE_PERSISTENT); @@ -5330,7 +5332,7 @@ QuotaManager::EnsurePersistentOriginIsInitialized( QM_TRY_UNWRAP(auto directory, GetOriginDirectory(aOriginMetadata)); - if (mInitializedOrigins.Contains(aOriginMetadata.mOrigin)) { + if (mInitializedOriginsInternal.Contains(aOriginMetadata.mOrigin)) { MOZ_ASSERT(firstInitializationAttempt.Recorded()); return std::pair(std::move(directory), false); } @@ -5365,7 +5367,7 @@ QuotaManager::EnsurePersistentOriginIsInitialized( aOriginMetadata, timestamp, /* aPersisted */ true, directory))); - mInitializedOrigins.AppendElement(aOriginMetadata.mOrigin); + mInitializedOriginsInternal.AppendElement(aOriginMetadata.mOrigin); return std::pair(std::move(directory), created); }; @@ -5376,7 +5378,7 @@ QuotaManager::EnsurePersistentOriginIsInitialized( innerFunc); } -bool QuotaManager::IsTemporaryOriginInitialized( +bool QuotaManager::IsTemporaryOriginInitializedInternal( const OriginMetadata& aOriginMetadata) const { AssertIsOnIOThread(); MOZ_ASSERT(aOriginMetadata.mPersistenceType != PERSISTENCE_TYPE_PERSISTENT); @@ -5390,7 +5392,7 @@ bool QuotaManager::IsTemporaryOriginInitialized( } Result, bool>, nsresult> -QuotaManager::EnsureTemporaryOriginIsInitialized( +QuotaManager::EnsureTemporaryOriginIsInitializedInternal( const OriginMetadata& aOriginMetadata) { AssertIsOnIOThread(); MOZ_ASSERT(aOriginMetadata.mPersistenceType != PERSISTENCE_TYPE_PERSISTENT); @@ -5453,7 +5455,8 @@ QuotaManager::EnsurePersistentClientIsInitialized( MOZ_ASSERT(aClientMetadata.mPersistenceType == PERSISTENCE_TYPE_PERSISTENT); MOZ_ASSERT(Client::IsValidType(aClientMetadata.mClientType)); MOZ_DIAGNOSTIC_ASSERT(IsStorageInitializedInternal()); - MOZ_DIAGNOSTIC_ASSERT(IsOriginInitialized(aClientMetadata.mOrigin)); + MOZ_DIAGNOSTIC_ASSERT( + IsPersistentOriginInitializedInternal(aClientMetadata.mOrigin)); QM_TRY_UNWRAP(auto directory, GetOriginDirectory(aClientMetadata)); @@ -5489,7 +5492,7 @@ QuotaManager::EnsureTemporaryClientIsInitialized( MOZ_ASSERT(Client::IsValidType(aClientMetadata.mClientType)); MOZ_DIAGNOSTIC_ASSERT(IsStorageInitializedInternal()); MOZ_DIAGNOSTIC_ASSERT(IsTemporaryStorageInitializedInternal()); - MOZ_DIAGNOSTIC_ASSERT(IsTemporaryOriginInitialized(aClientMetadata)); + MOZ_DIAGNOSTIC_ASSERT(IsTemporaryOriginInitializedInternal(aClientMetadata)); QM_TRY_UNWRAP(auto directory, GetOriginDirectory(aClientMetadata)); @@ -5729,7 +5732,7 @@ void QuotaManager::ShutdownStorageInternal() { if (mStorageConnection) { mInitializationInfo.ResetOriginInitializationInfos(); - mInitializedOrigins.Clear(); + mInitializedOriginsInternal.Clear(); if (mTemporaryStorageInitializedInternal) { if (mCacheUsable) { @@ -5802,7 +5805,7 @@ void QuotaManager::OriginClearCompleted( if (aClientType.IsNull()) { if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) { - mInitializedOrigins.RemoveElement(aOrigin); + mInitializedOriginsInternal.RemoveElement(aOrigin); } for (Client::Type type : AllClientTypes()) { @@ -5818,7 +5821,7 @@ void QuotaManager::RepositoryClearCompleted(PersistenceType aPersistenceType) { AssertIsOnIOThread(); if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) { - mInitializedOrigins.Clear(); + mInitializedOriginsInternal.Clear(); } for (Client::Type type : AllClientTypes()) { diff --git a/dom/quota/OriginOperations.cpp b/dom/quota/OriginOperations.cpp index 26707b068890..00e4a3e58884 100644 --- a/dom/quota/OriginOperations.cpp +++ b/dom/quota/OriginOperations.cpp @@ -1584,11 +1584,12 @@ nsresult InitializePersistentOriginOp::DoDirectoryWork( QM_TRY(OkIf(aQuotaManager.IsStorageInitializedInternal()), NS_ERROR_NOT_INITIALIZED); - QM_TRY_UNWRAP(mCreated, - (aQuotaManager - .EnsurePersistentOriginIsInitialized(OriginMetadata{ - mPrincipalMetadata, PERSISTENCE_TYPE_PERSISTENT}) - .map([](const auto& res) { return res.second; }))); + QM_TRY_UNWRAP( + mCreated, + (aQuotaManager + .EnsurePersistentOriginIsInitializedInternal( + OriginMetadata{mPrincipalMetadata, PERSISTENCE_TYPE_PERSISTENT}) + .map([](const auto& res) { return res.second; }))); return NS_OK; } @@ -1624,7 +1625,7 @@ nsresult InitializeTemporaryOriginOp::DoDirectoryWork( QM_TRY_UNWRAP(mCreated, (aQuotaManager - .EnsureTemporaryOriginIsInitialized( + .EnsureTemporaryOriginIsInitializedInternal( OriginMetadata{mPrincipalMetadata, mPersistenceType}) .map([](const auto& res) { return res.second; }))); @@ -1700,9 +1701,9 @@ nsresult InitializePersistentClientOp::DoDirectoryWork( QM_TRY(MOZ_TO_RESULT(aQuotaManager.IsStorageInitializedInternal()), NS_ERROR_FAILURE); - QM_TRY( - MOZ_TO_RESULT(aQuotaManager.IsOriginInitialized(mClientMetadata.mOrigin)), - NS_ERROR_FAILURE); + QM_TRY(MOZ_TO_RESULT(aQuotaManager.IsPersistentOriginInitializedInternal( + mClientMetadata.mOrigin)), + NS_ERROR_FAILURE); QM_TRY_UNWRAP( mCreated, @@ -1740,8 +1741,8 @@ nsresult InitializeTemporaryClientOp::DoDirectoryWork( QM_TRY(MOZ_TO_RESULT(aQuotaManager.IsTemporaryStorageInitializedInternal()), NS_ERROR_FAILURE); - QM_TRY(MOZ_TO_RESULT( - aQuotaManager.IsTemporaryOriginInitialized(mClientMetadata)), + QM_TRY(MOZ_TO_RESULT(aQuotaManager.IsTemporaryOriginInitializedInternal( + mClientMetadata)), NS_ERROR_FAILURE); QM_TRY_UNWRAP( @@ -2055,7 +2056,8 @@ void ClearRequestBase::DeleteFilesInternal( const bool initialized = aPersistenceType == PERSISTENCE_TYPE_PERSISTENT - ? aQuotaManager.IsOriginInitialized(metadata.mOrigin) + ? aQuotaManager.IsPersistentOriginInitializedInternal( + metadata.mOrigin) : aQuotaManager.IsTemporaryStorageInitializedInternal(); // If it hasn't been initialized, we don't need to update the diff --git a/dom/quota/QuotaManager.h b/dom/quota/QuotaManager.h index ae61b512ae0e..48e48d347eac 100644 --- a/dom/quota/QuotaManager.h +++ b/dom/quota/QuotaManager.h @@ -141,10 +141,10 @@ class QuotaManager final : public BackgroundThreadObject { void UnregisterNormalOriginOp(NormalOriginOperationBase& aNormalOriginOp); - bool IsOriginInitialized(const nsACString& aOrigin) const { + bool IsPersistentOriginInitializedInternal(const nsACString& aOrigin) const { AssertIsOnIOThread(); - return mInitializedOrigins.Contains(aOrigin); + return mInitializedOriginsInternal.Contains(aOrigin); } bool IsTemporaryStorageInitializedInternal() const { @@ -362,15 +362,17 @@ class QuotaManager final : public BackgroundThreadObject { // Returns a pair of an nsIFile object referring to the directory, and a bool // indicating whether the directory was newly created. Result, bool>, nsresult> - EnsurePersistentOriginIsInitialized(const OriginMetadata& aOriginMetadata); + EnsurePersistentOriginIsInitializedInternal( + const OriginMetadata& aOriginMetadata); - bool IsTemporaryOriginInitialized( + bool IsTemporaryOriginInitializedInternal( const OriginMetadata& aOriginMetadata) const; // Returns a pair of an nsIFile object referring to the directory, and a bool // indicating whether the directory was newly created. Result, bool>, nsresult> - EnsureTemporaryOriginIsInitialized(const OriginMetadata& aOriginMetadata); + EnsureTemporaryOriginIsInitializedInternal( + const OriginMetadata& aOriginMetadata); RefPtr InitializePersistentClient( const PrincipalInfo& aPrincipalInfo, Client::Type aClientType); @@ -770,7 +772,7 @@ class QuotaManager final : public BackgroundThreadObject { // A list of all successfully initialized persistent origins. This list isn't // protected by any mutex but it is only ever touched on the IO thread. - nsTArray mInitializedOrigins; + nsTArray mInitializedOriginsInternal; // A hash table that is used to cache origin parser results for given // sanitized origin strings. This hash table isn't protected by any mutex but diff --git a/dom/quota/QuotaUsageRequestBase.cpp b/dom/quota/QuotaUsageRequestBase.cpp index 17b37cbf290b..15d1550de629 100644 --- a/dom/quota/QuotaUsageRequestBase.cpp +++ b/dom/quota/QuotaUsageRequestBase.cpp @@ -39,7 +39,8 @@ Result QuotaUsageRequestBase::GetUsageForOrigin( bool initialized; if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) { - initialized = aQuotaManager.IsOriginInitialized(aOriginMetadata.mOrigin); + initialized = aQuotaManager.IsPersistentOriginInitializedInternal( + aOriginMetadata.mOrigin); } else { initialized = aQuotaManager.IsTemporaryStorageInitializedInternal(); } diff --git a/dom/quota/test/gtest/TestFileOutputStream.cpp b/dom/quota/test/gtest/TestFileOutputStream.cpp index fac2b6828385..94e8240ba611 100644 --- a/dom/quota/test/gtest/TestFileOutputStream.cpp +++ b/dom/quota/test/gtest/TestFileOutputStream.cpp @@ -61,8 +61,8 @@ TEST_F(TestFileOutputStream, extendFileStreamWithSetEOF) { ASSERT_NS_SUCCEEDED( quotaManager->EnsureTemporaryStorageIsInitializedInternal()); - auto res = - quotaManager->EnsureTemporaryOriginIsInitialized(originMetadata); + auto res = quotaManager->EnsureTemporaryOriginIsInitializedInternal( + originMetadata); ASSERT_TRUE(res.isOk()); } diff --git a/dom/quota/test/gtest/TestStorageConnection.cpp b/dom/quota/test/gtest/TestStorageConnection.cpp index 6693ccdb1ce6..0b3f2ddf7b75 100644 --- a/dom/quota/test/gtest/TestStorageConnection.cpp +++ b/dom/quota/test/gtest/TestStorageConnection.cpp @@ -38,7 +38,7 @@ void InitializeClientDirectory(const ClientMetadata& aClientMetadata) { QM_TRY_INSPECT( const auto& directory, - quotaManager->EnsureTemporaryOriginIsInitialized(aClientMetadata) + quotaManager->EnsureTemporaryOriginIsInitializedInternal(aClientMetadata) .map([](const auto& aPair) { return aPair.first; }), QM_TEST_FAIL); diff --git a/dom/simpledb/ActorsParent.cpp b/dom/simpledb/ActorsParent.cpp index eb55b6b8fdc6..a1eed6bc9c92 100644 --- a/dom/simpledb/ActorsParent.cpp +++ b/dom/simpledb/ActorsParent.cpp @@ -1185,14 +1185,15 @@ nsresult OpenOp::DatabaseWork() { this]() -> mozilla::Result, bool>, nsresult> { if (persistenceType == PERSISTENCE_TYPE_PERSISTENT) { - QM_TRY_RETURN(quotaManager->EnsurePersistentOriginIsInitialized( - mOriginMetadata)); + QM_TRY_RETURN( + quotaManager->EnsurePersistentOriginIsInitializedInternal( + mOriginMetadata)); } QM_TRY(MOZ_TO_RESULT( quotaManager->EnsureTemporaryStorageIsInitializedInternal())); - QM_TRY_RETURN( - quotaManager->EnsureTemporaryOriginIsInitialized(mOriginMetadata)); + QM_TRY_RETURN(quotaManager->EnsureTemporaryOriginIsInitializedInternal( + mOriginMetadata)); }() .map([](const auto& res) { return res.first; })));