tor-browser

commit 04ec36336f28d15dc660eab00e92f035529770de
parent bfc34eb4cd0058456bee2c30353dc89345f00923
Author: Jan Varga <Jan.Varga@gmail.com>
Date:   Mon, 27 Oct 2025 20:33:46 +0000

Bug 1988590 - QM: Refactor ClearOrigins to support custom checkers; r=asuth,dom-storage-reviewers,jari

This patch refactors QuotaManager::ClearOrigins to accept custom checkers via a
templated parameter. Callers can now provide assertions or predicates (e.g.
ensuring non-persisted or zero-usage origins) without hard-coding them in the
method body. This improves flexibility and keeps debug-only checks outside of
the core clearing logic.

Testing: This is exercised by existing quota manager tests.

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

Diffstat:
M
dom/quota/ActorsParent.cpp
 | 
55
++++++++++++++++++++++++++++++++++++++++++++-----------
M
dom/quota/Flatten.h
 | 
2
++
M
dom/quota/QuotaManager.h
 | 
14
++++++++++++++

3 files changed, 60 insertions(+), 11 deletions(-)
diff --git a/dom/quota/ActorsParent.cpp b/dom/quota/ActorsParent.cpp
@@ -8052,11 +8052,39 @@ QuotaManager::GetOriginInfosWithZeroUsage() const {
   return res;
 }
 
+// Based on discussions in
+// https://stackoverflow.com/questions/41847525/should-templated-functions-take-lambda-arguments-by-value-or-by-rvalue-reference
+// and
+// https://stackoverflow.com/questions/56988299/should-i-pass-a-lambda-by-const-reference,
+// passing a callable by universal (forwarding) reference is considered a more
+// flexible choice from an interface point of view.
+//
+// In future, we should also ensure this pattern is compatible with clang-tidy
+// checks such as modernize-pass-by-value and consider enabling
+// cppcoreguidelines-missing-std-forward to catch potential oversights.
+template <typename Checker>
 void QuotaManager::ClearOrigins(
-    const OriginInfosNestedTraversable& aDoomedOriginInfos,
+    const OriginInfosNestedTraversable& aDoomedOriginInfos, Checker&& aChecker,
     const Maybe<size_t>& aMaxOriginsToClear) {
   AssertIsOnIOThread();
 
+  if (QuotaManager::IsShuttingDown()) {
+    // Don't block shutdown.
+    return;
+  }
+
+  auto doomedOriginInfos =
+      Flatten<OriginInfosFlatTraversable::value_type>(aDoomedOriginInfos);
+
+  // If there's nothing to do, exit early.
+  if (doomedOriginInfos.begin() == doomedOriginInfos.end()) {
+    return;
+  }
+
+  // It's safer to take ownership of the checker and keep it outside the loop
+  // (the checker can have side effects or use move).
+  std::decay_t<Checker> checker = std::forward<Checker>(aChecker);
+
   // If we are in shutdown, we could break off early from clearing origins.
   // In such cases, we would like to track the ones that were already cleared
   // up, such that other essential cleanup could be performed on clearedOrigins.
@@ -8069,14 +8097,8 @@ void QuotaManager::ClearOrigins(
   nsTArray<OriginMetadata> clearedOrigins;
 
   // XXX Does this need to be done a) in order and/or b) sequentially?
-  for (const auto& doomedOriginInfo :
-       Flatten<OriginInfosFlatTraversable::value_type>(aDoomedOriginInfos)) {
-#ifdef DEBUG
-    {
-      MutexAutoLock lock(mQuotaMutex);
-      MOZ_ASSERT(!doomedOriginInfo->LockedPersisted());
-    }
-#endif
+  for (const auto& doomedOriginInfo : doomedOriginInfos) {
+    std::invoke(checker, doomedOriginInfo);
 
     //  TODO: We are currently only checking for this flag here which
     //  means that we cannot break off once we start cleaning an origin. It
@@ -8122,8 +8144,19 @@ void QuotaManager::CleanupTemporaryStorage() {
   // Evicting origins that exceed their group limit also affects the global
   // temporary storage usage, so these steps have to be taken sequentially.
   // Combining them doesn't seem worth the added complexity.
-  ClearOrigins(GetOriginInfosExceedingGroupLimit());
-  ClearOrigins(GetOriginInfosExceedingGlobalLimit());
+
+#ifdef DEBUG
+  auto nonPersistedChecker = [&self = *this](const auto& doomedOriginInfo) {
+    MutexAutoLock lock(self.mQuotaMutex);
+    MOZ_ASSERT(!doomedOriginInfo->LockedPersisted());
+  };
+#else
+  auto nonPersistedChecker = [](const auto&) {};
+#endif
+
+  ClearOrigins(GetOriginInfosExceedingGroupLimit(), nonPersistedChecker);
+  ClearOrigins(GetOriginInfosExceedingGlobalLimit(),
+               std::move(nonPersistedChecker));
 
   if (mTemporaryStorageUsage > mTemporaryStorageLimit) {
     // If disk space is still low after origin clear, notify storage pressure.
diff --git a/dom/quota/Flatten.h b/dom/quota/Flatten.h
@@ -63,6 +63,8 @@ struct FlatIter {
            (mOuterIter != mOuterEnd && mInnerIter != aOther.mInnerIter);
   }
 
+  bool operator==(const FlatIter& aOther) const { return !(*this != aOther); }
+
  private:
   void InitInner() {
     while (mOuterIter != mOuterEnd) {
diff --git a/dom/quota/QuotaManager.h b/dom/quota/QuotaManager.h
@@ -862,7 +862,21 @@ class QuotaManager final : public BackgroundThreadObject {
 
   OriginInfosNestedTraversable GetOriginInfosWithZeroUsage() const;
 
+  /**
+   * Clears the given set of origins.
+   *
+   * @param aDoomedOriginInfos
+   *   Origins to be cleared.
+   * @param aChecker
+   *   A callable invoked for each origin before clearing. Typically used to
+   *   enforce or assert invariants at the call site.
+   * @param aMaxOriginsToClear
+   *   Optional cap on the number of origins cleared in a single run. If
+   *   Nothing(), all doomed origins are cleared.
+   */
+  template <typename Checker>
   void ClearOrigins(const OriginInfosNestedTraversable& aDoomedOriginInfos,
+                    Checker&& aChecker,
                     const Maybe<size_t>& aMaxOriginsToClear = Nothing());
 
   void CleanupTemporaryStorage();