Bug 1711181 - Use QM_OR_ELSE_LOG in remaining places where QM_OR_ELSE_WARN would spam the reports; r=dom-storage-reviewers,jstutte

Differential Revision: https://phabricator.services.mozilla.com/D115159

dom/cache/FileUtils.cpp

@@ -437,9 +437,23 @@ Result<nsCOMPtr<nsIFile>, nsresult> GetMarkerFileHandle(
 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
+  // 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
+  // cleaned it up, but obviously we can crash and not clean it up, which is
+  // the whole point of the marker file. In that case, we'll realize the marker
+  // file exists in SetupAction::RunSyncWithDBOnTarget and do some cleanup, but
+  // we won't delete the marker file, so if we see this marker file, it is part
+  // 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_WARN(ToResult(marker->Create(nsIFile::NORMAL_FILE_TYPE, 0644)),
-                      MapAlreadyExistsToDefault));
+      QM_OR_ELSE_LOG(ToResult(marker->Create(nsIFile::NORMAL_FILE_TYPE, 0644)),
+                     MapAlreadyExistsToDefault));
 
   // Note, we don't need to fsync here.  We only care about actually
   // writing the marker if later modifications to the Cache are

dom/indexedDB/ActorsParent.cpp

@@ -5728,6 +5728,14 @@ nsresult DeleteFile(nsIFile& aFile, QuotaManager* const aQuotaManager,
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(!IsOnBackgroundThread());
 
+  // Callers which pass Idempotency::Yes call this function without checking if
+  // the file already exists (idempotent usage). QM_OR_ELSE_WARN is not used
+  // here since we just want to log NS_ERROR_FILE_NOT_FOUND and
+  // NS_ERROR_FILE_TARGET_DOES_NOT_EXIST results and not spam the reports.
+  // Theoretically, there should be no QM_OR_ELSE_(WARN|LOG) when a caller
+  // passes Idempotency::No, but it's simpler when the orElse function just
+  // always propagates the passed error.
+
   QM_TRY_INSPECT(
       const auto& fileSize,
       ([aQuotaManager, &aFile,
@@ -5735,7 +5743,7 @@ nsresult DeleteFile(nsIFile& aFile, QuotaManager* const aQuotaManager,
         if (aQuotaManager) {
           QM_TRY_INSPECT(
               const Maybe<int64_t>& fileSize,
-              QM_OR_ELSE_WARN(
+              QM_OR_ELSE_LOG(
                   MOZ_TO_RESULT_INVOKE(aFile, GetFileSize)
                       .map([](const int64_t val) { return Some(val); }),
                   MakeMaybeIdempotentFilter<int64_t>(aIdempotent)));
@@ -5756,8 +5764,8 @@ nsresult DeleteFile(nsIFile& aFile, QuotaManager* const aQuotaManager,
   }
 
   QM_TRY_INSPECT(const auto& didExist,
-                 QM_OR_ELSE_WARN(ToResult(aFile.Remove(false)).map(Some<Ok>),
-                                 MakeMaybeIdempotentFilter<Ok>(aIdempotent)));
+                 QM_OR_ELSE_LOG(ToResult(aFile.Remove(false)).map(Some<Ok>),
+                                MakeMaybeIdempotentFilter<Ok>(aIdempotent)));
 
   if (!didExist) {
     // XXX If we get here, this means that the file still existed when we
@@ -5825,7 +5833,24 @@ Result<nsCOMPtr<nsIFile>, nsresult> CreateMarkerFile(
       CloneFileAndAppend(aBaseDirectory,
                          kIdbDeletionMarkerFilePrefix + aDatabaseNameBase));
 
-  QM_TRY(QM_OR_ELSE_WARN(
+  // 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
+  // to log NS_ERROR_FILE_ALREADY_EXISTS result and not spam the reports.
+  //
+  // TODO: In theory if this file exists, then RemoveDatabaseFilesAndDirectory
+  // should have cleaned it up, but obviously we can crash and not clean it up,
+  // which is the whole point of the marker file. In that case, we'll realize
+  // the marker file exists in OpenDatabaseOp::DoDatabaseWork or
+  // GetUsageForOriginInternal and resume the removal by calling
+  // RemoveDatabaseFilesAndDirectory again, but we will also try to create the
+  // marker file again, so if we see this marker file, it is part
+  // of our standard operating procedure to redundantly try and create the
+  // marker here. We currently treat this as idempotent usage, but we could
+  // add an additional argument to RemoveDatabaseFilesAndDirectory which would
+  // indicate that we are resuming an unfinished removal, so the marker already
+  // exists and doesn't have to be created, and change QM_OR_ELSE_LOG to
+  // QM_OR_ELSE_WARN in the end.
+  QM_TRY(QM_OR_ELSE_LOG(
       ToResult(markerFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644)),
       ErrToDefaultOkOrErr<NS_ERROR_FILE_ALREADY_EXISTS>));
 
@@ -14608,7 +14633,9 @@ nsresult DatabaseOperationBase::InsertIndexTableRows(
     QM_TRY(aObjectStoreKey.BindToStatement(&*borrowedStmt,
                                            kStmtParamNameObjectDataKey));
 
-    QM_TRY(QM_OR_ELSE_WARN(
+    // QM_OR_ELSE_WARN is not used here since we just want to log the
+    // collision and not spam the reports.
+    QM_TRY(QM_OR_ELSE_LOG(
         ToResult(borrowedStmt->Execute()),
         ([&info, index, &aIndexValues](nsresult rv) -> Result<Ok, nsresult> {
           if (rv == NS_ERROR_STORAGE_CONSTRAINT && info.mUnique) {