Quota: Fix client accounting in QuotaManager helpers.

https://crrev.com/c/2241955 refactored how QuotaManager reasons about
QuotaClients. The new setup maintains separate lists of clients for each
StorageType (kTemporary, kPersistent, kSyncable).  Unfortunately, the CL
did not correctly migrate all the QuotaManager helpers to iterate over
the per-StorageType lists instead of the global list.

This CL migrates QuotaManager helpers, and adds testing coverage to
prevent future regressions.

Bug: 1100260
Change-Id: Ie788fc08ffc24cb1bb9d2b83524c21a9d9e296d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299466
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788871}

storage/browser/quota/quota_manager.cc

@@ -532,9 +532,9 @@ class QuotaManager::OriginDataDeleter : public QuotaTask {
 
  protected:
   void Run() override {
-    error_count_ = 0;
-    remaining_clients_ = manager()->clients_.size();
     DCHECK(manager()->client_types_.contains(type_));
+    remaining_clients_ = manager()->client_types_[type_].size();
+
     for (const auto& client_and_type : manager()->client_types_[type_]) {
       QuotaClient* client = client_and_type.first;
       QuotaClientType client_type = client_and_type.second;
@@ -549,10 +549,12 @@ class QuotaManager::OriginDataDeleter : public QuotaTask {
                            weak_factory_.GetWeakPtr(), tracing_id));
       } else {
         ++skipped_clients_;
-        if (--remaining_clients_ == 0)
-          CallCompleted();
+        --remaining_clients_;
       }
     }
+
+    if (remaining_clients_ == 0)
+      CallCompleted();
   }
 
   void Completed() override {
@@ -622,10 +624,11 @@ class QuotaManager::HostDataDeleter : public QuotaTask {
 
  protected:
   void Run() override {
-    error_count_ = 0;
-    remaining_clients_ = manager()->clients_.size();
-    for (const auto& client : manager()->clients_) {
-      client->GetOriginsForHost(
+    DCHECK(manager()->client_types_.contains(type_));
+    remaining_clients_ = manager()->client_types_[type_].size();
+
+    for (const auto& client_and_type : manager()->client_types_[type_]) {
+      client_and_type.first->GetOriginsForHost(
           type_, host_,
           base::BindOnce(&HostDataDeleter::DidGetOriginsForHost,
                          weak_factory_.GetWeakPtr()));
@@ -715,14 +718,14 @@ class QuotaManager::StorageCleanupHelper : public QuotaTask {
 
  protected:
   void Run() override {
+    DCHECK(manager()->client_types_.contains(type_));
     base::RepeatingClosure barrier = base::BarrierClosure(
-        manager()->clients_.size(),
+        manager()->client_types_[type_].size(),
         base::BindOnce(&StorageCleanupHelper::CallCompleted,
                        weak_factory_.GetWeakPtr()));
 
     // This may synchronously trigger |callback_| at the end of the for loop,
     // make sure we do nothing after this block.
-    DCHECK(manager()->client_types_.contains(type_));
     for (const auto& client_and_type : manager()->client_types_[type_]) {
       QuotaClient* client = client_and_type.first;
       QuotaClientType client_type = client_and_type.second;
@@ -1060,7 +1063,9 @@ void QuotaManager::DeleteHostData(const std::string& host,
                                   StatusCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   LazyInitialize();
-  if (host.empty() || clients_.empty()) {
+
+  DCHECK(client_types_.contains(type));
+  if (host.empty() || client_types_[type].empty()) {
     std::move(callback).Run(blink::mojom::QuotaStatusCode::kOk);
     return;
   }
@@ -1217,8 +1222,12 @@ bool QuotaManager::ResetUsageTracker(StorageType type) {
 
 QuotaManager::~QuotaManager() {
   proxy_->manager_ = nullptr;
-  for (const auto& client : clients_)
+
+  // Iterating over |clients_for_ownership_| is correct here because we want to
+  // call OnQuotaManagerDestroyed() once per QuotaClient.
+  for (const auto& client : clients_for_ownership_)
     client->OnQuotaManagerDestroyed();
+
   if (database_)
     db_runner_->DeleteSoon(FROM_HERE, database_.release());
 }
@@ -1307,7 +1316,7 @@ void QuotaManager::RegisterClient(
 
   for (blink::mojom::StorageType storage_type : storage_types)
     client_types_[storage_type].insert({client.get(), client_type});
-  clients_.push_back(std::move(client));
+  clients_for_ownership_.push_back(std::move(client));
 }
 
 UsageTracker* QuotaManager::GetUsageTracker(StorageType type) const {
@@ -1446,11 +1455,6 @@ void QuotaManager::DeleteOriginDataInternal(const url::Origin& origin,
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   LazyInitialize();
 
-  if (clients_.empty()) {
-    std::move(callback).Run(blink::mojom::QuotaStatusCode::kOk);
-    return;
-  }
-
   OriginDataDeleter* deleter =
       new OriginDataDeleter(this, origin, type, std::move(quota_client_types),
                             is_eviction, std::move(callback));

storage/browser/quota/quota_manager.h

@@ -472,11 +472,14 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) QuotaManager
   std::set<url::Origin> access_notified_origins_;
 
   // Owns the QuotaClient instances registered via RegisterClient().
-  std::vector<scoped_refptr<QuotaClient>> clients_;
+  //
+  // Iterating over this list is almost always incorrect. Most algorithms should
+  // iterate over an entry in |client_types_|.
+  std::vector<scoped_refptr<QuotaClient>> clients_for_ownership_;
   // Maps QuotaClient instances to client types.
   //
   // The QuotaClient instances pointed to by the map keys are guaranteed to be
-  // alive, because they are owned by |clients_|.
+  // alive, because they are owned by |clients_for_ownership_|.
   base::flat_map<blink::mojom::StorageType,
                  base::flat_map<QuotaClient*, QuotaClientType>>
       client_types_;

storage/browser/quota/quota_manager_unittest.cc

@@ -1724,6 +1724,12 @@ TEST_F(QuotaManagerTest, GetEvictionRoundInfo) {
   EXPECT_LE(0, available_space());
 }
 
+TEST_F(QuotaManagerTest, DeleteHostDataNoClients) {
+  DeleteHostData(std::string(), kTemp, AllQuotaClientTypes());
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(QuotaStatusCode::kOk, status());
+}
+
 TEST_F(QuotaManagerTest, DeleteHostDataSimple) {
   static const MockOriginData kData[] = {
     { "http://foo.com/",   kTemp,     1 },
@@ -1861,6 +1867,101 @@ TEST_F(QuotaManagerTest, DeleteHostDataMultiple) {
   EXPECT_EQ(predelete_bar_pers, usage());
 }
 
+TEST_F(QuotaManagerTest, DeleteHostDataMultipleClientsDifferentTypes) {
+  static const MockOriginData kData1[] = {
+      {"http://foo.com/", kPerm, 1},
+      {"http://foo.com:1/", kPerm, 10},
+      {"http://foo.com/", kTemp, 100},
+      {"http://bar.com/", kPerm, 1000},
+  };
+  static const MockOriginData kData2[] = {
+      {"http://foo.com/", kTemp, 10000},
+      {"http://foo.com:1/", kTemp, 100000},
+      {"https://foo.com/", kTemp, 1000000},
+      {"http://bar.com/", kTemp, 10000000},
+  };
+  CreateAndRegisterClient(kData1, QuotaClientType::kFileSystem,
+                          {blink::mojom::StorageType::kTemporary,
+                           blink::mojom::StorageType::kPersistent});
+  CreateAndRegisterClient(kData2, QuotaClientType::kDatabase,
+                          {blink::mojom::StorageType::kTemporary});
+
+  GetGlobalUsage(kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_global_tmp = usage();
+
+  GetHostUsageWithBreakdown("foo.com", kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_foo_tmp = usage();
+
+  GetHostUsageWithBreakdown("bar.com", kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_bar_tmp = usage();
+
+  GetGlobalUsage(kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_global_pers = usage();
+
+  GetHostUsageWithBreakdown("foo.com", kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_foo_pers = usage();
+
+  GetHostUsageWithBreakdown("bar.com", kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_bar_pers = usage();
+
+  reset_status_callback_count();
+  DeleteHostData("foo.com", kPerm, AllQuotaClientTypes());
+  DeleteHostData("bar.com", kPerm, AllQuotaClientTypes());
+  task_environment_.RunUntilIdle();
+
+  EXPECT_EQ(2, status_callback_count());
+
+  DumpOriginInfoTable();
+  task_environment_.RunUntilIdle();
+
+  for (const auto& entry : origin_info_entries()) {
+    if (entry.type != kTemp)
+      continue;
+
+    EXPECT_NE(std::string("http://foo.com/"), entry.origin.GetURL().spec());
+    EXPECT_NE(std::string("http://foo.com:1/"), entry.origin.GetURL().spec());
+    EXPECT_NE(std::string("https://foo.com/"), entry.origin.GetURL().spec());
+    EXPECT_NE(std::string("http://bar.com/"), entry.origin.GetURL().spec());
+  }
+
+  GetGlobalUsage(kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_global_tmp, usage());
+
+  GetHostUsageWithBreakdown("foo.com", kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_foo_tmp, usage());
+
+  GetHostUsageWithBreakdown("bar.com", kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_bar_tmp, usage());
+
+  GetGlobalUsage(kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_global_pers - (1 + 10 + 1000), usage());
+
+  GetHostUsageWithBreakdown("foo.com", kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_foo_pers - (1 + 10), usage());
+
+  GetHostUsageWithBreakdown("bar.com", kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_bar_pers - 1000, usage());
+}
+
+TEST_F(QuotaManagerTest, DeleteOriginDataNoClients) {
+  DeleteOriginData(url::Origin::Create(GURL("http://foo.com/")), kTemp,
+                   AllQuotaClientTypes());
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(QuotaStatusCode::kOk, status());
+}
+
 // Single-run DeleteOriginData cases must be well covered by
 // EvictOriginData tests.
 TEST_F(QuotaManagerTest, DeleteOriginDataMultiple) {
@@ -1954,6 +2055,102 @@ TEST_F(QuotaManagerTest, DeleteOriginDataMultiple) {
   EXPECT_EQ(predelete_bar_pers, usage());
 }
 
+TEST_F(QuotaManagerTest, DeleteOriginDataMultipleClientsDifferentTypes) {
+  static const MockOriginData kData1[] = {
+      {"http://foo.com/", kPerm, 1},
+      {"http://foo.com:1/", kPerm, 10},
+      {"http://foo.com/", kTemp, 100},
+      {"http://bar.com/", kPerm, 1000},
+  };
+  static const MockOriginData kData2[] = {
+      {"http://foo.com/", kTemp, 10000},
+      {"http://foo.com:1/", kTemp, 100000},
+      {"https://foo.com/", kTemp, 1000000},
+      {"http://bar.com/", kTemp, 10000000},
+  };
+  CreateAndRegisterClient(kData1, QuotaClientType::kFileSystem,
+                          {blink::mojom::StorageType::kTemporary,
+                           blink::mojom::StorageType::kPersistent});
+  CreateAndRegisterClient(kData2, QuotaClientType::kDatabase,
+                          {blink::mojom::StorageType::kTemporary});
+
+  GetGlobalUsage(kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_global_tmp = usage();
+
+  GetHostUsageWithBreakdown("foo.com", kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_foo_tmp = usage();
+
+  GetHostUsageWithBreakdown("bar.com", kTemp);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_bar_tmp = usage();
+
+  GetGlobalUsage(kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_global_pers = usage();
+
+  GetHostUsageWithBreakdown("foo.com", kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_foo_pers = usage();
+
+  GetHostUsageWithBreakdown("bar.com", kPerm);
+  task_environment_.RunUntilIdle();
+  const int64_t predelete_bar_pers = usage();
+
+  for (const MockOriginData& data : kData1) {
+    quota_manager()->NotifyStorageAccessed(
+        url::Origin::Create(GURL(data.origin)), data.type);
+  }
+  for (const MockOriginData& data : kData2) {
+    quota_manager()->NotifyStorageAccessed(
+        url::Origin::Create(GURL(data.origin)), data.type);
+  }
+  task_environment_.RunUntilIdle();
+
+  reset_status_callback_count();
+  DeleteOriginData(ToOrigin("http://foo.com/"), kPerm, AllQuotaClientTypes());
+  DeleteOriginData(ToOrigin("http://bar.com/"), kPerm, AllQuotaClientTypes());
+  task_environment_.RunUntilIdle();
+
+  EXPECT_EQ(2, status_callback_count());
+
+  DumpOriginInfoTable();
+  task_environment_.RunUntilIdle();
+
+  for (const auto& entry : origin_info_entries()) {
+    if (entry.type != kPerm)
+      continue;
+
+    EXPECT_NE(std::string("http://foo.com/"), entry.origin.GetURL().spec());
+    EXPECT_NE(std::string("http://bar.com/"), entry.origin.GetURL().spec());
+  }
+
+  GetGlobalUsage(kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_global_tmp, usage());
+
+  GetHostUsageWithBreakdown("foo.com", kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_foo_tmp, usage());
+
+  GetHostUsageWithBreakdown("bar.com", kTemp);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_bar_tmp, usage());
+
+  GetGlobalUsage(kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_global_pers - (1 + 1000), usage());
+
+  GetHostUsageWithBreakdown("foo.com", kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_foo_pers - 1, usage());
+
+  GetHostUsageWithBreakdown("bar.com", kPerm);
+  task_environment_.RunUntilIdle();
+  EXPECT_EQ(predelete_bar_pers - 1000, usage());
+}
+
 TEST_F(QuotaManagerTest, GetCachedOrigins) {
   static const MockOriginData kData[] = {
     { "http://a.com/",   kTemp,       1 },