Storage buckets: clear expired buckets during eviction rounds

Combine this with eviction in order to share the timing (polling) and
to make sure zombie expired buckets aren't using up cache and causing
us to evict non-expired buckets.

Bug: 1315392
Change-Id: Ib7a4ac2715aecba04fcbb8c119d99e0a7e97cb2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3997427
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Auto-Submit: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067334}

storage/browser/quota/quota_database.cc

@@ -768,6 +768,24 @@ QuotaErrorOr<std::set<BucketLocator>> QuotaDatabase::GetBucketsModifiedBetween(
   return buckets;
 }
 
+QuotaErrorOr<std::set<BucketInfo>> QuotaDatabase::GetExpiredBuckets() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  QuotaError open_error = EnsureOpened();
+  if (open_error != QuotaError::kNone)
+    return open_error;
+
+  // clang-format off
+  static constexpr char kSql[] =
+      "SELECT " BUCKET_INFO_FIELDS_SELECTOR
+        "FROM buckets "
+        "WHERE expiration > 0 AND expiration < ?";
+  // clang-format on
+
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindTime(0, GetNow());
+  return BucketInfosFromSqlStatement(statement);
+}
+
 bool QuotaDatabase::IsBootstrapped() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (EnsureOpened() != QuotaError::kNone)

storage/browser/quota/quota_database.h

@@ -202,6 +202,9 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaDatabase {
       base::Time begin,
       base::Time end);
 
+  // Returns a set of all expired buckets.
+  QuotaErrorOr<std::set<BucketInfo>> GetExpiredBuckets();
+
   base::FilePath GetStoragePath() const { return storage_directory_->path(); }
 
   // Returns false if SetIsBootstrapped() has never been called before, which

storage/browser/quota/quota_database_unittest.cc

@@ -1217,6 +1217,10 @@ TEST_P(QuotaDatabaseTest, Expiration) {
   ASSERT_TRUE(result.ok());
   EXPECT_TRUE(result->expiration.is_null());
 
+  QuotaErrorOr<std::set<BucketInfo>> expired_buckets = db.GetExpiredBuckets();
+  ASSERT_TRUE(expired_buckets.ok());
+  EXPECT_EQ(0U, expired_buckets->size());
+
   // Non-default `expiration` value.
   BucketInitParams params2(
       StorageKey::CreateFromStringForTesting("http://example/"),
@@ -1227,10 +1231,24 @@ TEST_P(QuotaDatabaseTest, Expiration) {
   EXPECT_EQ(params2.expiration, result->expiration);
 
   // Update `expiration` value.
-  const base::Time updated_time = base::Time::Now() + base::Days(1);
+  base::Time updated_time = base::Time::Now() + base::Days(1);
   result = db.UpdateBucketExpiration(result->id, updated_time);
   ASSERT_TRUE(result.ok());
   EXPECT_EQ(updated_time, result->expiration);
+
+  expired_buckets = db.GetExpiredBuckets();
+  ASSERT_TRUE(expired_buckets.ok());
+  EXPECT_EQ(0U, expired_buckets->size());
+
+  // Set expiration to the past.
+  updated_time = base::Time::Now() - base::Days(1);
+  result = db.UpdateBucketExpiration(result->id, updated_time);
+  ASSERT_TRUE(result.ok());
+  EXPECT_EQ(updated_time, result->expiration);
+
+  expired_buckets = db.GetExpiredBuckets();
+  ASSERT_TRUE(expired_buckets.ok());
+  EXPECT_EQ(1U, expired_buckets->size());
 }
 
 TEST_P(QuotaDatabaseTest, Persistent) {

storage/browser/quota/quota_manager_impl.cc

@@ -791,44 +791,38 @@ class QuotaManagerImpl::BucketDataDeleter {
       GUARDED_BY_CONTEXT(sequence_checker_){this};
 };
 
-// Retrieves all buckets for `host` from QuotaDatabase and calls
-// BucketDataDeleter for all registered QuotaClientTypes.
-class QuotaManagerImpl::HostDataDeleter {
+// Deletes a set of buckets.
+class QuotaManagerImpl::BucketSetDataDeleter {
  public:
   // `callback` will be run to report the status of the deletion on task
-  // completion. `callback` will only be called while this HostDataDeleter
-  // instance is alive. `callback` may destroy this HostDataDeleter instance.
-  HostDataDeleter(
+  // completion. `callback` will only be called while this BucketSetDataDeleter
+  // instance is alive. `callback` may destroy this BucketSetDataDeleter
+  // instance.
+  BucketSetDataDeleter(
       QuotaManagerImpl* manager,
-      const std::string& host,
-      StorageType type,
-      base::OnceCallback<void(HostDataDeleter*, blink::mojom::QuotaStatusCode)>
-          callback)
-      : manager_(manager),
-        host_(host),
-        type_(type),
-        callback_(std::move(callback)) {
+      base::OnceCallback<void(BucketSetDataDeleter*,
+                              blink::mojom::QuotaStatusCode)> callback)
+      : manager_(manager), callback_(std::move(callback)) {
     DCHECK(manager_);
     DCHECK(callback_);
   }
 
-  ~HostDataDeleter() {
+  ~BucketSetDataDeleter() {
     if (callback_) {
       std::move(callback_).Run(this,
                                blink::mojom::QuotaStatusCode::kErrorAbort);
     }
   }
 
-  void Run() {
+  base::OnceCallback<void(QuotaErrorOr<std::set<BucketInfo>>)>
+  GetBucketDeletionCallback() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #if DCHECK_IS_ON()
-    DCHECK(!run_called_) << __func__ << " already called";
-    run_called_ = true;
+    DCHECK(!started_) << __func__ << " already called";
+    started_ = true;
 #endif  // DCHECK_IS_ON()
-    manager_->GetBucketsForHost(
-        host_, type_,
-        base::BindOnce(&HostDataDeleter::DidGetBucketsForHost,
-                       weak_factory_.GetWeakPtr()));
+    return base::BindOnce(&BucketSetDataDeleter::DidGetBuckets,
+                          weak_factory_.GetWeakPtr());
   }
 
   bool completed() {
@@ -837,7 +831,7 @@ class QuotaManagerImpl::HostDataDeleter {
   }
 
  private:
-  void DidGetBucketsForHost(QuotaErrorOr<std::set<BucketInfo>> result) {
+  void DidGetBuckets(QuotaErrorOr<std::set<BucketInfo>> result) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     if (!result.ok()) {
       Complete(/*success=*/false);
@@ -861,7 +855,7 @@ class QuotaManagerImpl::HostDataDeleter {
       // callback when it's alive.
       auto bucket_deleter = std::make_unique<BucketDataDeleter>(
           manager_, bucket, AllQuotaClientTypes(), /*commit_immediately=*/false,
-          base::BindOnce(&HostDataDeleter::DidDeleteBucketData,
+          base::BindOnce(&BucketSetDataDeleter::DidDeleteBucketData,
                          base::Unretained(this)));
       auto* bucket_deleter_ptr = bucket_deleter.get();
       bucket_deleters_[bucket_deleter_ptr] = std::move(bucket_deleter);
@@ -898,20 +892,18 @@ class QuotaManagerImpl::HostDataDeleter {
   SEQUENCE_CHECKER(sequence_checker_);
   const raw_ptr<QuotaManagerImpl> manager_
       GUARDED_BY_CONTEXT(sequence_checker_);
-  const std::string host_;
-  const StorageType type_;
   std::map<BucketDataDeleter*, std::unique_ptr<BucketDataDeleter>>
       bucket_deleters_;
   std::set<BucketLocator> buckets_ GUARDED_BY_CONTEXT(sequence_checker_);
   int error_count_ GUARDED_BY_CONTEXT(sequence_checker_) = 0;
-  base::OnceCallback<void(HostDataDeleter*, blink::mojom::QuotaStatusCode)>
+  base::OnceCallback<void(BucketSetDataDeleter*, blink::mojom::QuotaStatusCode)>
       callback_ GUARDED_BY_CONTEXT(sequence_checker_);
 
 #if DCHECK_IS_ON()
-  bool run_called_ GUARDED_BY_CONTEXT(sequence_checker_) = false;
+  bool started_ GUARDED_BY_CONTEXT(sequence_checker_) = false;
 #endif  // DCHECK_IS_ON()
 
-  base::WeakPtrFactory<HostDataDeleter> weak_factory_{this};
+  base::WeakPtrFactory<BucketSetDataDeleter> weak_factory_{this};
 };
 
 class QuotaManagerImpl::StorageCleanupHelper : public QuotaTask {
@@ -1498,29 +1490,29 @@ void QuotaManagerImpl::DeleteHostData(const std::string& host,
     std::move(callback).Run(blink::mojom::QuotaStatusCode::kOk);
     return;
   }
-  auto host_deleter = std::make_unique<HostDataDeleter>(
-      this, host, type,
-      base::BindOnce(&QuotaManagerImpl::DidDeleteHostData,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
-  auto* host_deleter_ptr = host_deleter.get();
-  host_data_deleters_[host_deleter_ptr] = std::move(host_deleter);
-  host_deleter_ptr->Run();
+  auto buckets_deleter = std::make_unique<BucketSetDataDeleter>(
+      this, base::BindOnce(&QuotaManagerImpl::DidDeleteBuckets,
+                           weak_factory_.GetWeakPtr(), std::move(callback)));
+  auto* buckets_deleter_ptr = buckets_deleter.get();
+  bucket_set_data_deleters_[buckets_deleter_ptr] = std::move(buckets_deleter);
+  GetBucketsForHost(host, type,
+                    buckets_deleter_ptr->GetBucketDeletionCallback());
 }
 
 // static
-void QuotaManagerImpl::DidDeleteHostData(
+void QuotaManagerImpl::DidDeleteBuckets(
     base::WeakPtr<QuotaManagerImpl> quota_manager,
-    StatusCallback delete_host_data_callback,
-    HostDataDeleter* deleter,
+    StatusCallback callback,
+    BucketSetDataDeleter* deleter,
     blink::mojom::QuotaStatusCode status_code) {
-  DCHECK(delete_host_data_callback);
+  DCHECK(callback);
   DCHECK(deleter);
   DCHECK(deleter->completed());
 
   if (quota_manager)
-    quota_manager->host_data_deleters_.erase(deleter);
+    quota_manager->bucket_set_data_deleters_.erase(deleter);
 
-  std::move(delete_host_data_callback).Run(status_code);
+  std::move(callback).Run(status_code);
 }
 
 void QuotaManagerImpl::BindInternalsHandler(
@@ -1802,6 +1794,10 @@ bool QuotaManagerImpl::ResetUsageTracker(StorageType type) {
 }
 
 QuotaManagerImpl::~QuotaManagerImpl() {
+  // Delete this now because otherwise it may call back into `this` after the
+  // `sequence_checker_` has been destroyed.
+  temporary_storage_evictor_.reset();
+
   proxy_->InvalidateQuotaManagerImpl(base::PassKey<QuotaManagerImpl>());
 
   if (database_)
@@ -2514,6 +2510,30 @@ void QuotaManagerImpl::GetEvictionBucket(StorageType type,
                            weak_factory_.GetWeakPtr(), std::move(callback)));
 }
 
+void QuotaManagerImpl::EvictExpiredBuckets(StatusCallback done) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(done);
+  EnsureDatabaseOpened();
+
+  if (db_disabled_) {
+    std::move(done).Run(blink::mojom::QuotaStatusCode::kUnknown);
+    return;
+  }
+
+  auto buckets_deleter = std::make_unique<BucketSetDataDeleter>(
+      this, base::BindOnce(&QuotaManagerImpl::DidDeleteBuckets,
+                           weak_factory_.GetWeakPtr(), std::move(done)));
+  auto* buckets_deleter_ptr = buckets_deleter.get();
+  bucket_set_data_deleters_[buckets_deleter_ptr] = std::move(buckets_deleter);
+
+  PostTaskAndReplyWithResultForDBThread(
+      base::BindOnce([](QuotaDatabase* database) {
+        DCHECK(database);
+        return database->GetExpiredBuckets();
+      }),
+      buckets_deleter_ptr->GetBucketDeletionCallback());
+}
+
 void QuotaManagerImpl::EvictBucketData(const BucketLocator& bucket,
                                        StatusCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

storage/browser/quota/quota_manager_impl.h

@@ -71,6 +71,9 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaEvictionHandler {
                               int64_t global_usage,
                               bool global_usage_is_complete)>;
 
+  // Deletes all buckets that have explicit expiration dates which have passed.
+  virtual void EvictExpiredBuckets(StatusCallback done) = 0;
+
   // Called at the beginning of an eviction round to gather the info about
   // the current settings, capacity, and usage.
   virtual void GetEvictionRoundInfo(EvictionRoundInfoCallback callback) = 0;
@@ -392,6 +395,14 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl
       storage::mojom::StorageType storage_type,
       GetGlobalUsageForInternalsCallback callback) override;
 
+  // QuotaEvictionHandler.
+  void EvictExpiredBuckets(StatusCallback done) override;
+  void GetEvictionBucket(blink::mojom::StorageType type,
+                         GetBucketCallback callback) override;
+  void EvictBucketData(const BucketLocator& bucket,
+                       StatusCallback callback) override;
+  void GetEvictionRoundInfo(EvictionRoundInfoCallback callback) override;
+
   // Called by UI and internal modules.
   void GetPersistentHostQuota(const std::string& host, QuotaCallback callback);
   void SetPersistentHostQuota(const std::string& host,
@@ -515,7 +526,7 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl
   class GetUsageInfoTask;
   class StorageKeyGathererTask;
   class BucketDataDeleter;
-  class HostDataDeleter;
+  class BucketSetDataDeleter;
   class DumpBucketTableHelper;
   class StorageCleanupHelper;
 
@@ -604,14 +615,14 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl
                                 QuotaClientTypes quota_client_types,
                                 StatusCallback callback);
 
-  // Removes the HostDataDeleter that completed its work.
+  // Removes the BucketSetDataDeleter that completed its work.
   //
-  // This method is static because it must call `delete_host_data_callback` even
-  // if the QuotaManagerImpl was destroyed.
-  static void DidDeleteHostData(base::WeakPtr<QuotaManagerImpl> quota_manager,
-                                StatusCallback delete_host_data_callback,
-                                HostDataDeleter* deleter,
-                                blink::mojom::QuotaStatusCode status_code);
+  // This method is static because it must call `callback` even if the
+  // QuotaManagerImpl was destroyed.
+  static void DidDeleteBuckets(base::WeakPtr<QuotaManagerImpl> quota_manager,
+                               StatusCallback callback,
+                               BucketSetDataDeleter* deleter,
+                               blink::mojom::QuotaStatusCode status_code);
 
   // Removes the BucketDataDeleter that completed its work.
   //
@@ -654,13 +665,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl
   void DidGetEvictionBucket(GetBucketCallback callback,
                             const absl::optional<BucketLocator>& bucket);
 
-  // QuotaEvictionHandler.
-  void GetEvictionBucket(blink::mojom::StorageType type,
-                         GetBucketCallback callback) override;
-  void EvictBucketData(const BucketLocator& bucket,
-                       StatusCallback callback) override;
-  void GetEvictionRoundInfo(EvictionRoundInfoCallback callback) override;
-
   void DidGetBucketInfoForEviction(
       const BucketLocator& bucket,
       QuotaErrorOr<mojom::BucketTableEntryPtr> result);
@@ -849,8 +853,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManagerImpl
   GetVolumeInfoFn get_volume_info_fn_;
 
   std::unique_ptr<EvictionRoundInfoHelper> eviction_helper_;
-  std::map<HostDataDeleter*, std::unique_ptr<HostDataDeleter>>
-      host_data_deleters_;
+  std::map<BucketSetDataDeleter*, std::unique_ptr<BucketSetDataDeleter>>
+      bucket_set_data_deleters_;
   std::map<BucketDataDeleter*, std::unique_ptr<BucketDataDeleter>>
       bucket_data_deleters_;
   std::unique_ptr<StorageKeyGathererTask> storage_key_gatherer_;

storage/browser/quota/quota_manager_unittest.cc

@@ -838,6 +838,41 @@ TEST_F(QuotaManagerImplTest, UpdateOrCreateBucket_Expiration) {
   QuotaDatabase::SetClockForTesting(nullptr);
 }
 
+// Make sure `EvictExpiredBuckets` deletes expired buckets.
+TEST_F(QuotaManagerImplTest, EvictExpiredBuckets) {
+  auto clock = std::make_unique<base::SimpleTestClock>();
+  QuotaDatabase::SetClockForTesting(clock.get());
+  clock->SetNow(base::Time::Now());
+
+  BucketInitParams params(ToStorageKey("http://a.com/"), "bucket_a");
+  params.expiration = clock->Now() + base::Days(1);
+  auto bucket = UpdateOrCreateBucket(params);
+  ASSERT_TRUE(bucket.ok());
+
+  BucketInitParams params_b(ToStorageKey("http://b.com/"), "bucket_b");
+  params_b.expiration = clock->Now() + base::Days(10);
+  auto bucket_b = UpdateOrCreateBucket(params_b);
+  ASSERT_TRUE(bucket_b.ok());
+
+  // No specified expiration.
+  BucketInitParams params_c(ToStorageKey("http://c.com/"), "bucket_c");
+  auto bucket_c = UpdateOrCreateBucket(params_c);
+  ASSERT_TRUE(bucket_c.ok());
+
+  clock->Advance(base::Days(5));
+
+  // Evict expired buckets.
+  base::test::TestFuture<QuotaStatusCode> future;
+  quota_manager_impl_->EvictExpiredBuckets(future.GetCallback());
+  EXPECT_EQ(QuotaStatusCode::kOk, future.Get());
+
+  EXPECT_FALSE(GetBucketById(bucket->id).ok());
+  EXPECT_TRUE(GetBucketById(bucket_b->id).ok());
+  EXPECT_TRUE(GetBucketById(bucket_c->id).ok());
+
+  QuotaDatabase::SetClockForTesting(nullptr);
+}
+
 TEST_F(QuotaManagerImplTest, GetOrCreateBucketSync) {
   base::RunLoop loop;
   // Post the function call on a different thread to ensure that the