Bug 1714573: Ensure profile deletion takes place before the profile is unlocked. r=mossop,dom-storage-reviewers,janv

We want to make our best effort to delete all files we can before we unlock the profile. This includes a retry loop hoping that whoever else locked those files did so intermittently.
And once the profile is unlocked, we want to reduce as much as possible the possibility to interfere with a starting instance on the same profile directory.

Until bug 1716291 lands, we need to get back here to finish the work, but this patch is already supposed to mitigate some of the problems we might see in the wild.

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

Author: jensstutte

toolkit/profile/nsProfileLock.cpp

@@ -399,18 +399,29 @@ nsresult nsProfileLock::GetReplacedLockTime(PRTime* aResult) {
   return NS_OK;
 }
 
-nsresult nsProfileLock::Lock(nsIFile* aProfileDir,
-                             nsIProfileUnlocker** aUnlocker) {
 #if defined(XP_MACOSX)
-  constexpr auto LOCKFILE_NAME = u".parentlock"_ns;
-  constexpr auto OLD_LOCKFILE_NAME = u"parent.lock"_ns;
+constexpr auto LOCKFILE_NAME = u".parentlock"_ns;
+constexpr auto OLD_LOCKFILE_NAME = u"parent.lock"_ns;
 #elif defined(XP_UNIX)
-  constexpr auto OLD_LOCKFILE_NAME = u"lock"_ns;
-  constexpr auto LOCKFILE_NAME = u".parentlock"_ns;
+constexpr auto OLD_LOCKFILE_NAME = u"lock"_ns;
+constexpr auto LOCKFILE_NAME = u".parentlock"_ns;
 #else
-  constexpr auto LOCKFILE_NAME = u"parent.lock"_ns;
+constexpr auto LOCKFILE_NAME = u"parent.lock"_ns;
+#endif
+
+bool nsProfileLock::IsMaybeLockFile(nsIFile* aFile) {
+  nsAutoString tmp;
+  if (NS_SUCCEEDED(aFile->GetLeafName(tmp))) {
+    if (tmp.Equals(LOCKFILE_NAME)) return true;
+#if (defined(XP_MACOSX) || defined(XP_UNIX))
+    if (tmp.Equals(OLD_LOCKFILE_NAME)) return true;
 #endif
+  }
+  return false;
+}
 
+nsresult nsProfileLock::Lock(nsIFile* aProfileDir,
+                             nsIProfileUnlocker** aUnlocker) {
   nsresult rv;
   if (aUnlocker) *aUnlocker = nullptr;
 

toolkit/profile/nsProfileLock.h

@@ -50,6 +50,12 @@ class nsProfileLock
    */
   nsresult Unlock(bool aFatalSignal = false);
 
+  /**
+   * Checks, if the given file has a name that matches potential lock file
+   * names. It does not check, if it actually is the currently active lock.
+   */
+  static bool IsMaybeLockFile(nsIFile* aFile);
+
   /**
    * Clean up any left over files in the directory.
    */

toolkit/profile/nsToolkitProfileService.cpp

@@ -4,9 +4,11 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/ArrayUtils.h"
+#include "mozilla/ScopeExit.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/UniquePtrExtensions.h"
 #include "mozilla/WidgetUtils.h"
+#include "nsProfileLock.h"
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -50,6 +52,8 @@
 #include "nsIToolkitShellService.h"
 #include "mozilla/Telemetry.h"
 #include "nsProxyRelease.h"
+#include "prinrval.h"
+#include "prthread.h"
 
 using namespace mozilla;
 
@@ -86,41 +90,130 @@ nsTArray<UniquePtr<KeyValue>> GetSectionStrings(nsINIParser* aParser,
   return result;
 }
 
+void RemoveProfileRecursion(const nsCOMPtr<nsIFile>& aDirectoryOrFile,
+                            bool aIsIgnoreRoot, bool aIsIgnoreLockfile,
+                            nsTArray<nsCOMPtr<nsIFile>>& aOutUndeletedFiles) {
+  auto guardDeletion = MakeScopeExit(
+      [&] { aOutUndeletedFiles.AppendElement(aDirectoryOrFile); });
+
+  // We actually would not expect to see links in our profiles, but still.
+  bool isLink = false;
+  NS_ENSURE_SUCCESS_VOID(aDirectoryOrFile->IsSymlink(&isLink));
+
+  // Only check to see if we have a directory if it isn't a link.
+  bool isDir = false;
+  if (!isLink) {
+    NS_ENSURE_SUCCESS_VOID(aDirectoryOrFile->IsDirectory(&isDir));
+  }
+
+  if (isDir) {
+    nsCOMPtr<nsIDirectoryEnumerator> dirEnum;
+    NS_ENSURE_SUCCESS_VOID(
+        aDirectoryOrFile->GetDirectoryEntries(getter_AddRefs(dirEnum)));
+
+    bool more = false;
+    while (NS_SUCCEEDED(dirEnum->HasMoreElements(&more)) && more) {
+      nsCOMPtr<nsISupports> item;
+      dirEnum->GetNext(getter_AddRefs(item));
+      nsCOMPtr<nsIFile> file = do_QueryInterface(item);
+      if (file) {
+        // Do not delete the profile lock.
+        if (aIsIgnoreLockfile && nsProfileLock::IsMaybeLockFile(file)) continue;
+        // If some children's remove fails, we still continue the loop.
+        RemoveProfileRecursion(file, false, false, aOutUndeletedFiles);
+      }
+    }
+  }
+  // Do not delete the root directory (yet).
+  if (!aIsIgnoreRoot) {
+    NS_ENSURE_SUCCESS_VOID(aDirectoryOrFile->Remove(false));
+  }
+  guardDeletion.release();
+}
+
 void RemoveProfileFiles(nsIToolkitProfile* aProfile, bool aInBackground) {
   nsCOMPtr<nsIFile> rootDir;
   aProfile->GetRootDir(getter_AddRefs(rootDir));
   nsCOMPtr<nsIFile> localDir;
   aProfile->GetLocalDir(getter_AddRefs(localDir));
 
+  // XXX If we get here with an active quota manager,
+  // something went very wrong. We want to assert this.
+
   // Just lock the directories, don't mark the profile as locked or the lock
   // will attempt to release its reference to the profile on the background
   // thread which will assert.
   nsCOMPtr<nsIProfileLock> lock;
-  nsresult rv =
-      NS_LockProfilePath(rootDir, localDir, nullptr, getter_AddRefs(lock));
-  NS_ENSURE_SUCCESS_VOID(rv);
+  NS_ENSURE_SUCCESS_VOID(
+      NS_LockProfilePath(rootDir, localDir, nullptr, getter_AddRefs(lock)));
 
   nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableFunction(
       "nsToolkitProfile::RemoveProfileFiles",
       [rootDir, localDir, lock]() mutable {
+        // We try to remove every single file and directory and collect
+        // those whose removal failed.
+        nsTArray<nsCOMPtr<nsIFile>> undeletedFiles;
+        // The root dir might contain the temp dir, so remove the temp dir
+        // first.
         bool equals;
         nsresult rv = rootDir->Equals(localDir, &equals);
-        // The root dir might contain the temp dir, so remove
-        // the temp dir first.
         if (NS_SUCCEEDED(rv) && !equals) {
-          localDir->Remove(true);
+          RemoveProfileRecursion(localDir,
+                                 /* aIsIgnoreRoot  */ false,
+                                 /* aIsIgnoreLockfile */ false, undeletedFiles);
         }
+        // Now remove the content of the profile dir (except lockfile)
+        RemoveProfileRecursion(rootDir,
+                               /* aIsIgnoreRoot  */ true,
+                               /* aIsIgnoreLockfile */ true, undeletedFiles);
+
+        // Retry loop if something was not deleted
+        if (undeletedFiles.Length() > 0) {
+          uint32_t retries = 1;
+          // XXX: Until bug 1716291 is fixed we just make one retry
+          while (undeletedFiles.Length() > 0 && retries <= 1) {
+            Unused << PR_Sleep(PR_MillisecondsToInterval(10 * retries));
+            for (auto&& file :
+                 std::exchange(undeletedFiles, nsTArray<nsCOMPtr<nsIFile>>{})) {
+              RemoveProfileRecursion(file,
+                                     /* aIsIgnoreRoot */ false,
+                                     /* aIsIgnoreLockfile */ true,
+                                     undeletedFiles);
+            }
+            retries++;
+          }
+        }
+
+#ifdef DEBUG
+        // XXX: Until bug 1716291 is fixed, we do not want to spam release
+        if (undeletedFiles.Length() > 0) {
+          NS_WARNING("Unable to remove all files from the profile directory:");
+          // Log the file names of those we could not remove
+          for (auto&& file : undeletedFiles) {
+            nsAutoString leafName;
+            if (NS_SUCCEEDED(file->GetLeafName(leafName))) {
+              NS_WARNING(NS_LossyConvertUTF16toASCII(leafName).get());
+            }
+          }
+        }
+#endif
+        // XXX: Activate this assert once bug 1716291 is fixed
+        // MOZ_ASSERT(undeletedFiles.Length() == 0);
 
-        // Ideally we'd unlock after deleting but since the lock is a file
-        // in the profile we must unlock before removing.
+        // Now we can unlock the profile safely.
         lock->Unlock();
         // nsIProfileLock is not threadsafe so release our reference to it on
         // the main thread.
         NS_ReleaseOnMainThread("nsToolkitProfile::RemoveProfileFiles::Unlock",
                                lock.forget());
 
-        rv = rootDir->Remove(true);
-        NS_ENSURE_SUCCESS_VOID(rv);
+        if (undeletedFiles.Length() == 0) {
+          // We can safely remove the (empty) remaining profile directory
+          // and lockfile, no other files are here.
+          // As we do this only if we had no other blockers, this is as safe
+          // as deleting the lockfile explicitely after unlocking.
+          Unused << rootDir->Remove(true);
+        }
       });
 
   if (aInBackground) {
@@ -350,6 +443,9 @@ nsToolkitProfileLock::Unlock() {
     return NS_ERROR_UNEXPECTED;
   }
 
+  // XXX If we get here with an active quota manager,
+  // something went very wrong. We want to assert this.
+
   mLock.Unlock();
 
   if (mProfile) {

toolkit/profile/test/test_create_profile.xhtml

@@ -106,6 +106,8 @@ function createProfile(profileName) {
 
   // Clean up the profile before local profile test.
   profile.remove(true);
+  ok(!jsonFile.exists(), "Times file was removed");
+  ok(!profileDir.exists(), "Profile dir was removed");
 
   // Create with non-null aRootDir
   profile = gProfileService.createProfile(profileDir, profileName);
@@ -116,6 +118,7 @@ function createProfile(profileName) {
 
   // Clean up the profile.
   profile.remove(true);
+  ok(!profileDir.exists(), "Profile dir was removed");
 }
 
   ]]>