Bug 1788986 - Part 4: Add a counter for nsIFile r=xpcom-reviewers,nika,barret,janv

This adds `count` out parameter for recursive file remove calls to report the number of the removed entries.

  Having a lot of files removed by `::Remove(recursive=true)` has been a problem, as a slow disk can cause a hang with such call. A counter feature will help us knowing the situation better via telemetry.

  The use of out parameter here is to mark it optional and prevent unwanted changes in existing callers (because a return value can't be optional).

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

dom/indexedDB/ActorsParent.cpp

@@ -5976,8 +5976,7 @@ Result<Ok, nsresult> DeleteFileManagerDirectory(
                        aOriginMetadata, Idempotency::Yes)));
       }));
 
-  QM_TRY_RETURN(
-      MOZ_TO_RESULT_INVOKE_MEMBER(aFileManagerDirectory, Remove, false));
+  QM_TRY_RETURN(MOZ_TO_RESULT(aFileManagerDirectory.Remove(false)));
 }
 
 // Idempotently delete all the parts of an IndexedDB database including its

xpcom/idl-parser/xpidl/header.py

@@ -194,7 +194,7 @@ def paramlistAsNative(m, empty="void", return_param=True):
         while (
             paramIter >= 0
             and m.params[paramIter].optional
-            and m.params[paramIter].paramtype == "out"
+            and "out" in m.params[paramIter].paramtype
         ):
             t = m.params[paramIter].type
             # Strings can't be optional, so this shouldn't happen, but let's make sure:

xpcom/io/FileDescriptorFile.cpp

@@ -274,7 +274,9 @@ FileDescriptorFile::RenameToNative(nsIFile* aNewParentDir,
 }
 
 NS_IMETHODIMP
-FileDescriptorFile::Remove(bool aRecursive) { return NS_ERROR_NOT_IMPLEMENTED; }
+FileDescriptorFile::Remove(bool aRecursive, uint32_t* aRemoveCount) {
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
 
 NS_IMETHODIMP
 FileDescriptorFile::GetPermissions(uint32_t* aPermissions) {

xpcom/io/nsIFile.idl

@@ -232,9 +232,13 @@ interface nsIFile : nsISupports
      *  This will try to delete this file.  The 'recursive' flag
      *  must be PR_TRUE to delete directories which are not empty.
      *
+     *  If passed, 'removeCount' will be incremented by the total number of files
+     *  and/or directories removed. Will be 1 unless the 'recursive' flag is
+     *  set. The parameter must be initialized beforehand.
+     *
      *  This will not resolve any symlinks.
      */
-    void remove(in boolean recursive);
+    void remove(in boolean recursive, [optional] inout uint32_t removeCount);
 
     /**
      *  Attributes of nsIFile.

xpcom/io/nsLocalFileUnix.cpp

@@ -1101,7 +1101,7 @@ nsLocalFile::MoveToFollowingLinksNative(nsIFile* aNewParent,
 }
 
 NS_IMETHODIMP
-nsLocalFile::Remove(bool aRecursive) {
+nsLocalFile::Remove(bool aRecursive, uint32_t* aRemoveCount) {
   CHECK_mPath();
   ENSURE_STAT_CACHE();
 
@@ -1113,7 +1113,11 @@ nsLocalFile::Remove(bool aRecursive) {
   }
 
   if (isSymLink || !S_ISDIR(mCachedStat.st_mode)) {
-    return NSRESULT_FOR_RETURN(unlink(mPath.get()));
+    rv = NSRESULT_FOR_RETURN(unlink(mPath.get()));
+    if (NS_SUCCEEDED(rv) && aRemoveCount) {
+      *aRemoveCount += 1;
+    }
+    return rv;
   }
 
   if (aRecursive) {
@@ -1138,7 +1142,9 @@ nsLocalFile::Remove(bool aRecursive) {
       if (NS_FAILED(rv)) {
         return NS_ERROR_FAILURE;
       }
-      rv = file->Remove(aRecursive);
+      // XXX: We care the result of the removal here while
+      // nsLocalFileWin does not. We should align the behavior. (bug 1779696)
+      rv = file->Remove(aRecursive, aRemoveCount);
 
 #ifdef ANDROID
       // See bug 580434 - Bionic gives us just deleted files
@@ -1152,7 +1158,11 @@ nsLocalFile::Remove(bool aRecursive) {
     }
   }
 
-  return NSRESULT_FOR_RETURN(rmdir(mPath.get()));
+  rv = NSRESULT_FOR_RETURN(rmdir(mPath.get()));
+  if (NS_SUCCEEDED(rv) && aRemoveCount) {
+    *aRemoveCount += 1;
+  }
+  return rv;
 }
 
 nsresult nsLocalFile::GetTimeImpl(PRTime* aTime,

xpcom/io/nsLocalFileWin.cpp

@@ -2287,7 +2287,7 @@ nsLocalFile::Load(PRLibrary** aResult) {
 }
 
 NS_IMETHODIMP
-nsLocalFile::Remove(bool aRecursive) {
+nsLocalFile::Remove(bool aRecursive, uint32_t* aRemoveCount) {
   // NOTE:
   //
   // if the working path points to a shortcut, then we will only
@@ -2340,9 +2340,11 @@ nsLocalFile::Remove(bool aRecursive) {
         return rv;
       }
 
+      // XXX: We are ignoring the result of the removal here while
+      // nsLocalFileUnix does not. We should align the behavior. (bug 1779696)
       nsCOMPtr<nsIFile> file;
       while (NS_SUCCEEDED(dirEnum->GetNextFile(getter_AddRefs(file))) && file) {
-        file->Remove(aRecursive);
+        file->Remove(aRecursive, aRemoveCount);
       }
     }
     if (RemoveDirectoryW(mWorkingPath.get()) == 0) {
@@ -2354,6 +2356,10 @@ nsLocalFile::Remove(bool aRecursive) {
     }
   }
 
+  if (aRemoveCount) {
+    *aRemoveCount += 1;
+  }
+
   MakeDirty();
   return rv;
 }

xpcom/tests/gtest/TestFile.cpp

@@ -201,7 +201,8 @@ static bool TestDeleteOnClose(nsIFile* aBase, const char* aName, int32_t aFlags,
 }
 
 // Test nsIFile::Remove, verifying that the file does not exist and did before
-static bool TestRemove(nsIFile* aBase, const char* aName, bool aRecursive) {
+static bool TestRemove(nsIFile* aBase, const char* aName, bool aRecursive,
+                       uint32_t aExpectedRemoveCount = 1) {
   nsCOMPtr<nsIFile> file = NewFile(aBase);
   if (!file) return false;
 
@@ -217,8 +218,10 @@ static bool TestRemove(nsIFile* aBase, const char* aName, bool aRecursive) {
     return false;
   }
 
-  rv = file->Remove(aRecursive);
+  uint32_t removeCount = 0;
+  rv = file->Remove(aRecursive, &removeCount);
   if (!VerifyResult(rv, "Remove")) return false;
+  EXPECT_EQ(removeCount, aExpectedRemoveCount) << "Removal count was wrong";
 
   rv = file->Exists(&exists);
   if (!VerifyResult(rv, "Exists (after)")) return false;
@@ -489,7 +492,7 @@ static void SetupAndTestFunctions(const nsAString& aDirName,
   // Test moving across directories and renaming at the same time
   ASSERT_TRUE(TestMove(subdir, base, "file2.txt", "file4.txt"));
 
-  // Test copying across directoreis
+  // Test copying across directories
   ASSERT_TRUE(TestCopy(base, subdir, "file4.txt", "file5.txt"));
 
   if (aTestNormalize) {
@@ -498,7 +501,7 @@ static void SetupAndTestFunctions(const nsAString& aDirName,
   }
 
   // Test recursive directory removal
-  ASSERT_TRUE(TestRemove(base, "subdir", true));
+  ASSERT_TRUE(TestRemove(base, "subdir", true, 2));
 
   if (aTestCreateUnique) {
     ASSERT_TRUE(TestCreateUnique(base, "foo", nsIFile::NORMAL_FILE_TYPE, 0600));