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

@@ -171,26 +171,23 @@ Result<nsCOMPtr<mozIStorageConnection>, nsresult> OpenDBConnection(
 
   QM_TRY_UNWRAP(
       auto conn,
-      QM_OR_ELSE_WARN(
+      QM_OR_ELSE_WARN_IF(
           MOZ_TO_RESULT_INVOKE_TYPED(nsCOMPtr<mozIStorageConnection>,
                                      storageService, OpenDatabaseWithFileURL,
                                      dbFileUrl, ""_ns),
+          IsDatabaseCorruptionError,
           ([&aQuotaInfo, &aDBFile, &storageService,
             &dbFileUrl](const nsresult rv)
                -> Result<nsCOMPtr<mozIStorageConnection>, 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<mozIStorageConnection>, storageService,
-                  OpenDatabaseWithFileURL, dbFileUrl, ""_ns));
-            }
-            return Err(rv);
+            QM_TRY_RETURN(MOZ_TO_RESULT_INVOKE_TYPED(
+                nsCOMPtr<mozIStorageConnection>, storageService,
+                OpenDatabaseWithFileURL, dbFileUrl, ""_ns));
           })));
 
   // Check the schema to make sure it is not too old.

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<Ok, nsresult> {
@@ -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<NS_ERROR_FILE_TOO_BIG>));
+  QM_TRY(QM_OR_ELSE_WARN_IF(
+      ToResult(aConn.SetGrowthIncrement(kGrowthSize, ""_ns)),
+      IsSpecificError<NS_ERROR_FILE_TOO_BIG>, ErrToDefaultOk<>));
 
   // Enable WAL journaling.  This must be performed in a separate transaction
   // after changing the page_size and enabling auto_vacuum.

dom/cache/FileUtils.cpp

@@ -64,18 +64,9 @@ nsresult DirectoryPaddingWrite(nsIFile& aBaseDir,
 
 const auto kMorgueDirectory = u"morgue"_ns;
 
-template <typename SuccessType = Ok>
-Result<SuccessType, nsresult> MapNotFoundToDefault(const nsresult aRv) {
-  return (aRv == NS_ERROR_FILE_NOT_FOUND ||
-          aRv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)
-             ? Result<SuccessType, nsresult>{std::in_place}
-             : Err(aRv);
-}
-
-Result<Ok, nsresult> MapAlreadyExistsToDefault(const nsresult aRv) {
-  return (aRv == NS_ERROR_FILE_ALREADY_EXISTS)
-             ? Result<Ok, nsresult>{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<NotNull<nsCOMPtr<nsIFile>>, nsresult> BodyGetCacheDir(nsIFile& aBaseDir,
@@ -89,11 +80,12 @@ Result<NotNull<nsCOMPtr<nsIFile>>, 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<NS_ERROR_FILE_ALREADY_EXISTS>)));
+  // 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<NS_ERROR_FILE_ALREADY_EXISTS>, 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<NS_ERROR_FILE_ALREADY_EXISTS>)));
+  // 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<NS_ERROR_FILE_ALREADY_EXISTS>, 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<Ok, nsresult> {
-                  // 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<NS_ERROR_FILE_FS_CORRUPTED>,
+                // 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<NS_ERROR_FILE_ALREADY_EXISTS>, 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<int64_t>),
-            MapNotFoundToDefault<Maybe<int64_t>>));
+            IsFileNotFoundError, ErrToDefaultOk<Maybe<int64_t>>));
 
     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<int64_t>{} : [&aBaseDir] {
-        QM_TRY_RETURN(
-            QM_OR_ELSE_WARN(DirectoryPaddingGet(aBaseDir).map(Some<int64_t>),
-                            MapNotFoundToDefault<Maybe<int64_t>>),
-            Maybe<int64_t>{});
+        QM_TRY_RETURN(QM_OR_ELSE_WARN_IF(
+                          DirectoryPaddingGet(aBaseDir).map(Some<int64_t>),
+                          IsFileNotFoundError, ErrToDefaultOk<Maybe<int64_t>>),
+                      Maybe<int64_t>{});
       }();
 
   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;
 }

dom/cache/QuotaClient.cpp

@@ -103,22 +103,17 @@ Result<UsageInfo, nsresult> 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<Ok, nsresult> {
-              // 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<NS_ERROR_FILE_FS_CORRUPTED>,
+            // We treat NS_ERROR_FILE_FS_CORRUPTED as if the
+            // directory did not exist at all.
+            ErrToDefaultOk<>));
         return usageInfo;
       }));
 }

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<Ok, nsresult> {
           // 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