From 2792c7214057e6351c96ebf6096beffed296bce5 Mon Sep 17 00:00:00 2001 From: Jan Varga Date: Sun, 30 May 2021 11:08:02 +0000 Subject: [PATCH] Bug 1708643 - CACHE: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=dom-storage-reviewers,asuth Differential Revision: https://phabricator.services.mozilla.com/D115150 --- dom/cache/DBAction.cpp | 21 ++++----- dom/cache/DBSchema.cpp | 8 +++- dom/cache/FileUtils.cpp | 86 ++++++++++++++++--------------------- dom/cache/QuotaClient.cpp | 17 +++----- dom/cache/QuotaClientImpl.h | 4 ++ 5 files changed, 62 insertions(+), 74 deletions(-) diff --git a/dom/cache/DBAction.cpp b/dom/cache/DBAction.cpp index 13ef88582a69..de20e9c6809f 100644 --- a/dom/cache/DBAction.cpp +++ b/dom/cache/DBAction.cpp @@ -171,26 +171,23 @@ Result, nsresult> OpenDBConnection( QM_TRY_UNWRAP( auto conn, - QM_OR_ELSE_WARN( + QM_OR_ELSE_WARN_IF( MOZ_TO_RESULT_INVOKE_TYPED(nsCOMPtr, storageService, OpenDatabaseWithFileURL, dbFileUrl, ""_ns), + IsDatabaseCorruptionError, ([&aQuotaInfo, &aDBFile, &storageService, &dbFileUrl](const nsresult rv) -> Result, nsresult> { - if (IsDatabaseCorruptionError(rv)) { - NS_WARNING( - "Cache database corrupted. Recreating empty database."); + NS_WARNING("Cache database corrupted. Recreating empty database."); - // There is nothing else we can do to recover. Also, this data - // can be deleted by QuotaManager at any time anyways. - QM_TRY(WipeDatabase(aQuotaInfo, aDBFile)); + // There is nothing else we can do to recover. Also, this data + // can be deleted by QuotaManager at any time anyways. + QM_TRY(WipeDatabase(aQuotaInfo, aDBFile)); - QM_TRY_RETURN(MOZ_TO_RESULT_INVOKE_TYPED( - nsCOMPtr, storageService, - OpenDatabaseWithFileURL, dbFileUrl, ""_ns)); - } - return Err(rv); + QM_TRY_RETURN(MOZ_TO_RESULT_INVOKE_TYPED( + nsCOMPtr, storageService, + OpenDatabaseWithFileURL, dbFileUrl, ""_ns)); }))); // Check the schema to make sure it is not too old. diff --git a/dom/cache/DBSchema.cpp b/dom/cache/DBSchema.cpp index 91ed8fc653f4..4b4b7b7a0ffe 100644 --- a/dom/cache/DBSchema.cpp +++ b/dom/cache/DBSchema.cpp @@ -520,6 +520,9 @@ nsresult CreateOrMigrateSchema(mozIStorageConnection& aConn) { // if a new migration is incorrect by fast failing on the corruption. // Unfortunately, this must be performed outside of the transaction. + // XXX Currently if both VACUUM and IntegrityCheck fail, we will propagate + // an error result from IntegrityCheck which is wrong. This should rather + // use a cleanup function instead of QM_OR_ELSE_WARN. QM_TRY( QM_OR_ELSE_WARN(ToResult(aConn.ExecuteSimpleSQL("VACUUM"_ns)), ([&aConn](const nsresult rv) -> Result { @@ -554,8 +557,9 @@ nsresult InitializeConnection(mozIStorageConnection& aConn) { kPageSize))); // Limit fragmentation by growing the database by many pages at once. - QM_TRY(QM_OR_ELSE_WARN(ToResult(aConn.SetGrowthIncrement(kGrowthSize, ""_ns)), - ErrToDefaultOkOrErr)); + QM_TRY(QM_OR_ELSE_WARN_IF( + ToResult(aConn.SetGrowthIncrement(kGrowthSize, ""_ns)), + IsSpecificError, ErrToDefaultOk<>)); // Enable WAL journaling. This must be performed in a separate transaction // after changing the page_size and enabling auto_vacuum. diff --git a/dom/cache/FileUtils.cpp b/dom/cache/FileUtils.cpp index ea92a2cb1a40..ba63d7d622e3 100644 --- a/dom/cache/FileUtils.cpp +++ b/dom/cache/FileUtils.cpp @@ -64,18 +64,9 @@ nsresult DirectoryPaddingWrite(nsIFile& aBaseDir, const auto kMorgueDirectory = u"morgue"_ns; -template -Result MapNotFoundToDefault(const nsresult aRv) { - return (aRv == NS_ERROR_FILE_NOT_FOUND || - aRv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) - ? Result{std::in_place} - : Err(aRv); -} - -Result MapAlreadyExistsToDefault(const nsresult aRv) { - return (aRv == NS_ERROR_FILE_ALREADY_EXISTS) - ? Result{std::in_place} - : Err(aRv); +bool IsFileNotFoundError(const nsresult aRv) { + return aRv == NS_ERROR_FILE_NOT_FOUND || + aRv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST; } Result>, nsresult> BodyGetCacheDir(nsIFile& aBaseDir, @@ -89,11 +80,12 @@ Result>, nsresult> BodyGetCacheDir(nsIFile& aBaseDir, QM_TRY(cacheDir->Append(IntToString(aId.m3[7]))); // Callers call this function without checking if the directory already - // exists (idempotent usage). QM_OR_ELSE_WARN is not used here since we just - // want to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the reports. - QM_TRY( - QM_OR_ELSE_LOG(ToResult(cacheDir->Create(nsIFile::DIRECTORY_TYPE, 0755)), - (ErrToDefaultOkOrErr))); + // exists (idempotent usage). QM_OR_ELSE_WARN_IF is not used here since we + // just want to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the + // reports. + QM_TRY(QM_OR_ELSE_LOG_IF( + ToResult(cacheDir->Create(nsIFile::DIRECTORY_TYPE, 0755)), + IsSpecificError, ErrToDefaultOk<>)); return WrapNotNullUnchecked(std::move(cacheDir)); } @@ -105,11 +97,12 @@ nsresult BodyCreateDir(nsIFile& aBaseDir) { CloneFileAndAppend(aBaseDir, kMorgueDirectory)); // Callers call this function without checking if the directory already - // exists (idempotent usage). QM_OR_ELSE_WARN is not used here since we just - // want to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the reports. - QM_TRY( - QM_OR_ELSE_LOG(ToResult(bodyDir->Create(nsIFile::DIRECTORY_TYPE, 0755)), - (ErrToDefaultOkOrErr))); + // exists (idempotent usage). QM_OR_ELSE_WARN_IF is not used here since we + // just want to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the + // reports. + QM_TRY(QM_OR_ELSE_LOG_IF( + ToResult(bodyDir->Create(nsIFile::DIRECTORY_TYPE, 0755)), + IsSpecificError, ErrToDefaultOk<>)); return NS_OK; } @@ -382,23 +375,18 @@ nsresult BodyDeleteOrphanedFiles(const QuotaInfo& aQuotaInfo, nsIFile& aBaseDir, return false; }; - // QM_OR_ELSE_WARN is not used here since we just want to log + // QM_OR_ELSE_WARN_IF is not used here since we just want to log // NS_ERROR_FILE_FS_CORRUPTED result and not spam the reports (even // a warning in the reports is not desired). - QM_TRY(QM_OR_ELSE_LOG( + QM_TRY(QM_OR_ELSE_LOG_IF( ToResult(BodyTraverseFiles(aQuotaInfo, *subdir, removeOrphanedFiles, /* aCanRemoveFiles */ true, /* aTrackQuota */ true)), - ([](const nsresult rv) -> Result { - // We treat NS_ERROR_FILE_FS_CORRUPTED as if the - // directory did not exist at all. - if (rv == NS_ERROR_FILE_FS_CORRUPTED) { - return Ok{}; - } - - return Err(rv); - }))); + IsSpecificError, + // We treat NS_ERROR_FILE_FS_CORRUPTED as if the + // directory did not exist at all. + ErrToDefaultOk<>)); break; } @@ -438,7 +426,7 @@ nsresult CreateMarkerFile(const QuotaInfo& aQuotaInfo) { QM_TRY_INSPECT(const auto& marker, GetMarkerFileHandle(aQuotaInfo)); // Callers call this function without checking if the file already exists - // (idempotent usage). QM_OR_ELSE_WARN is not used here since we just want + // (idempotent usage). QM_OR_ELSE_WARN_IF is not used here since we just want // to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the reports. // // TODO: In theory if this file exists, then Context::~Context should have @@ -449,11 +437,11 @@ nsresult CreateMarkerFile(const QuotaInfo& aQuotaInfo) { // of our standard operating procedure to redundantly try and create the // marker here. We currently treat this as idempotent usage, but we could // make sure to delete the marker file when handling the existing marker - // file in SetupAction::RunSyncWithDBOnTarget and change QM_OR_ELSE_LOG to - // QM_OR_ELSE_WARN in the end. - QM_TRY( - QM_OR_ELSE_LOG(ToResult(marker->Create(nsIFile::NORMAL_FILE_TYPE, 0644)), - MapAlreadyExistsToDefault)); + // file in SetupAction::RunSyncWithDBOnTarget and change QM_OR_ELSE_LOG_IF to + // QM_OR_ELSE_WARN_IF in the end. + QM_TRY(QM_OR_ELSE_LOG_IF( + ToResult(marker->Create(nsIFile::NORMAL_FILE_TYPE, 0644)), + IsSpecificError, ErrToDefaultOk<>)); // Note, we don't need to fsync here. We only care about actually // writing the marker if later modifications to the Cache are @@ -525,9 +513,9 @@ nsresult RemoveNsIFile(const QuotaInfo& aQuotaInfo, nsIFile& aFile, if (aTrackQuota) { QM_TRY_INSPECT( const auto& maybeFileSize, - QM_OR_ELSE_WARN( + QM_OR_ELSE_WARN_IF( MOZ_TO_RESULT_INVOKE(aFile, GetFileSize).map(Some), - MapNotFoundToDefault>)); + IsFileNotFoundError, ErrToDefaultOk>)); if (!maybeFileSize) { return NS_OK; @@ -536,8 +524,8 @@ nsresult RemoveNsIFile(const QuotaInfo& aQuotaInfo, nsIFile& aFile, fileSize = *maybeFileSize; } - QM_TRY(QM_OR_ELSE_WARN(ToResult(aFile.Remove(/* recursive */ false)), - MapNotFoundToDefault<>)); + QM_TRY(QM_OR_ELSE_WARN_IF(ToResult(aFile.Remove(/* recursive */ false)), + IsFileNotFoundError, ErrToDefaultOk<>)); if (fileSize > 0) { MOZ_ASSERT(aTrackQuota); @@ -608,10 +596,10 @@ nsresult UpdateDirectoryPaddingFile(nsIFile& aBaseDir, const auto directoryPaddingGetResult = aTemporaryFileExist ? Maybe{} : [&aBaseDir] { - QM_TRY_RETURN( - QM_OR_ELSE_WARN(DirectoryPaddingGet(aBaseDir).map(Some), - MapNotFoundToDefault>), - Maybe{}); + QM_TRY_RETURN(QM_OR_ELSE_WARN_IF( + DirectoryPaddingGet(aBaseDir).map(Some), + IsFileNotFoundError, ErrToDefaultOk>), + Maybe{}); }(); QM_TRY_INSPECT( @@ -724,8 +712,8 @@ nsresult DirectoryPaddingDeleteFile(nsIFile& aBaseDir, ? nsLiteralString(PADDING_TMP_FILE_NAME) : nsLiteralString(PADDING_FILE_NAME))); - QM_TRY(QM_OR_ELSE_WARN(ToResult(file->Remove(/* recursive */ false)), - MapNotFoundToDefault<>)); + QM_TRY(QM_OR_ELSE_WARN_IF(ToResult(file->Remove(/* recursive */ false)), + IsFileNotFoundError, ErrToDefaultOk<>)); return NS_OK; } diff --git a/dom/cache/QuotaClient.cpp b/dom/cache/QuotaClient.cpp index c0bd788f1689..f13bd2e2504a 100644 --- a/dom/cache/QuotaClient.cpp +++ b/dom/cache/QuotaClient.cpp @@ -103,22 +103,17 @@ Result GetBodyUsage(nsIFile& aMorgueDir, return false; }; - // QM_OR_ELSE_WARN is not used here since we just want to log + // QM_OR_ELSE_WARN_IF is not used here since we just want to log // NS_ERROR_FILE_FS_CORRUPTED result and not spam the reports (even a // warning in the reports is not desired). - QM_TRY(QM_OR_ELSE_LOG( + QM_TRY(QM_OR_ELSE_LOG_IF( ToResult(BodyTraverseFiles(QuotaInfo{}, *bodyDir, getUsage, /* aCanRemoveFiles */ true, /* aTrackQuota */ false)), - ([](const nsresult rv) -> Result { - // We treat NS_ERROR_FILE_FS_CORRUPTED as if the - // directory did not exist at all. - if (rv == NS_ERROR_FILE_FS_CORRUPTED) { - return Ok{}; - } - - return Err(rv); - }))); + IsSpecificError, + // We treat NS_ERROR_FILE_FS_CORRUPTED as if the + // directory did not exist at all. + ErrToDefaultOk<>)); return usageInfo; })); } diff --git a/dom/cache/QuotaClientImpl.h b/dom/cache/QuotaClientImpl.h index f6dc34b3139a..10670d79eeb1 100644 --- a/dom/cache/QuotaClientImpl.h +++ b/dom/cache/QuotaClientImpl.h @@ -91,10 +91,14 @@ class CacheQuotaClient final : public quota::Client { // next action recalculate the padding size. QM_TRY(aCommitHook()); + // XXX Replace QM_TRY(QM_OR_ELSE_WARN(...)) with QM_WARNONLY_TRY(..) QM_TRY(QM_OR_ELSE_WARN( ToResult(DirectoryPaddingFinalizeWrite(aBaseDir)), ([&aBaseDir](const nsresult) -> Result { // Force restore file next time. + // XXX QM_TRY failures from DirectoryPaddingDeleteFile are not + // propagated here, but there's no warning which would close the + // error stack. Unused << DirectoryPaddingDeleteFile(aBaseDir, DirPaddingFile::FILE); // Ensure that we are able to force the padding file