Quota: Remove UsageTracker::GetGlobalLimitedUsage().

ClientUsageTracker::GetGlobalLimitedUsage() has a bug, exposed by the
tests in https://crrev.com/c/2288502/1. Fortunately, it is only used to
implement UsageTracker::GetGlobalLimitedUsage(), which is only used in
tests.

This CL removes the unused code, and adds UsageTracker unit tests to
prevent bugs similar to the one discovered in GetGlobalLimitedUsage().

Bug: 1103426
Change-Id: Ifb44dbce66579ea9afc420fce0ee2482cafa127d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2288502
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786597}

storage/browser/quota/client_usage_tracker.cc

@@ -52,16 +52,15 @@ void RecordSkippedOriginHistogram(const InvalidOriginReason reason) {
   UMA_HISTOGRAM_ENUMERATION("Quota.SkippedInvalidOriginUsage", reason);
 }
 
-void DidGetGlobalClientUsageForLimitedGlobalClientUsage(
-    UsageCallback callback,
-    int64_t total_global_usage,
-    int64_t global_unlimited_usage) {
-  std::move(callback).Run(total_global_usage - global_unlimited_usage);
-}
-
 }  // namespace
 
 struct ClientUsageTracker::AccumulateInfo {
+  AccumulateInfo() = default;
+  ~AccumulateInfo() = default;
+
+  AccumulateInfo(const AccumulateInfo&) = delete;
+  AccumulateInfo& operator=(const AccumulateInfo&) = delete;
+
   size_t pending_jobs = 0;
   int64_t limited_usage = 0;
   int64_t unlimited_usage = 0;
@@ -89,35 +88,6 @@ ClientUsageTracker::~ClientUsageTracker() {
     special_storage_policy_->RemoveObserver(this);
 }
 
-void ClientUsageTracker::GetGlobalLimitedUsage(UsageCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!global_usage_retrieved_) {
-    GetGlobalUsage(
-        base::BindOnce(&DidGetGlobalClientUsageForLimitedGlobalClientUsage,
-                       std::move(callback)));
-    return;
-  }
-
-  if (non_cached_limited_origins_by_host_.empty()) {
-    std::move(callback).Run(global_limited_usage_);
-    return;
-  }
-
-  AccumulateInfo* info = new AccumulateInfo;
-  info->pending_jobs = non_cached_limited_origins_by_host_.size() + 1;
-  auto accumulator =
-      base::BindRepeating(&ClientUsageTracker::AccumulateLimitedOriginUsage,
-                          weak_factory_.GetWeakPtr(), base::Owned(info),
-                          AdaptCallbackForRepeating(std::move(callback)));
-
-  for (const auto& host_and_origins : non_cached_limited_origins_by_host_) {
-    for (const auto& origin : host_and_origins.second)
-      client_->GetOriginUsage(origin, type_, accumulator);
-  }
-
-  accumulator.Run(global_limited_usage_);
-}
-
 void ClientUsageTracker::GetGlobalUsage(GlobalUsageCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (global_usage_retrieved_ &&
@@ -257,18 +227,6 @@ void ClientUsageTracker::SetUsageCacheEnabled(const url::Origin& origin,
   }
 }
 
-void ClientUsageTracker::AccumulateLimitedOriginUsage(AccumulateInfo* info,
-                                                      UsageCallback callback,
-                                                      int64_t usage) {
-  DCHECK_GT(info->pending_jobs, 0U);
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  info->limited_usage += usage;
-  if (--info->pending_jobs)
-    return;
-
-  std::move(callback).Run(info->limited_usage);
-}
-
 void ClientUsageTracker::DidGetOriginsForGlobalUsage(
     GlobalUsageCallback callback,
     const std::set<url::Origin>& origins) {

storage/browser/quota/client_usage_tracker.h

@@ -57,7 +57,6 @@ class ClientUsageTracker : public SpecialStoragePolicy::Observer {
 
   ~ClientUsageTracker() override;
 
-  void GetGlobalLimitedUsage(UsageCallback callback);
   void GetGlobalUsage(GlobalUsageCallback callback);
   void GetHostUsage(const std::string& host, UsageCallback callback);
   void UpdateUsageCache(const url::Origin& origin, int64_t delta);
@@ -72,9 +71,6 @@ class ClientUsageTracker : public SpecialStoragePolicy::Observer {
 
   struct AccumulateInfo;
 
-  void AccumulateLimitedOriginUsage(AccumulateInfo* info,
-                                    UsageCallback callback,
-                                    int64_t usage);
   void DidGetOriginsForGlobalUsage(GlobalUsageCallback callback,
                                    const std::set<url::Origin>& origins);
   void AccumulateHostUsage(AccumulateInfo* info,

storage/browser/quota/usage_tracker.cc

@@ -16,16 +16,6 @@
 
 namespace storage {
 
-namespace {
-
-void DidGetGlobalUsageForLimitedGlobalUsage(UsageCallback callback,
-                                            int64_t total_global_usage,
-                                            int64_t global_unlimited_usage) {
-  std::move(callback).Run(total_global_usage - global_unlimited_usage);
-}
-
-}  // namespace
-
 struct UsageTracker::AccumulateInfo {
   AccumulateInfo() = default;
   ~AccumulateInfo() = default;
@@ -59,40 +49,6 @@ UsageTracker::~UsageTracker() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
-void UsageTracker::GetGlobalLimitedUsage(UsageCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!global_usage_callbacks_.empty()) {
-    global_usage_callbacks_.emplace_back(base::BindOnce(
-        &DidGetGlobalUsageForLimitedGlobalUsage, std::move(callback)));
-    return;
-  }
-
-  global_limited_usage_callbacks_.emplace_back(std::move(callback));
-  if (global_limited_usage_callbacks_.size() > 1)
-    return;
-
-  AccumulateInfo* info = new AccumulateInfo;
-  // Calling GetGlobalLimitedUsage(accumulator) may synchronously
-  // return if the usage is cached, which may in turn dispatch
-  // the completion callback before we finish looping over
-  // all clients (because info->pending_clients may reach 0
-  // during the loop).
-  // To avoid this, we add one more pending client as a sentinel
-  // and fire the sentinel callback at the end.
-  info->pending_clients = client_tracker_map_.size() + 1;
-  auto accumulator =
-      base::BindRepeating(&UsageTracker::AccumulateClientGlobalLimitedUsage,
-                          weak_factory_.GetWeakPtr(), base::Owned(info));
-
-  for (const auto& client_type_and_trackers : client_tracker_map_) {
-    for (const auto& client_tracker : client_type_and_trackers.second)
-      client_tracker->GetGlobalLimitedUsage(accumulator);
-  }
-
-  // Fire the sentinel as we've now called GetGlobalUsage for all clients.
-  accumulator.Run(0);
-}
-
 void UsageTracker::GetGlobalUsage(GlobalUsageCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   global_usage_callbacks_.emplace_back(std::move(callback));
@@ -217,22 +173,6 @@ void UsageTracker::SetUsageCacheEnabled(QuotaClientType client_type,
     client_tracker->SetUsageCacheEnabled(origin, enabled);
 }
 
-void UsageTracker::AccumulateClientGlobalLimitedUsage(AccumulateInfo* info,
-                                                      int64_t limited_usage) {
-  DCHECK_GT(info->pending_clients, 0U);
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  info->usage += limited_usage;
-  if (--info->pending_clients)
-    return;
-
-  // Moving callbacks out of the original vector handles the case where a
-  // callback makes a new quota call.
-  std::vector<UsageCallback> pending_callbacks;
-  pending_callbacks.swap(global_limited_usage_callbacks_);
-  for (auto& callback : pending_callbacks)
-    std::move(callback).Run(info->usage);
-}
-
 void UsageTracker::AccumulateClientGlobalUsage(AccumulateInfo* info,
                                                int64_t usage,
                                                int64_t unlimited_usage) {

storage/browser/quota/usage_tracker.h

@@ -55,7 +55,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) UsageTracker
     return type_;
   }
 
-  void GetGlobalLimitedUsage(UsageCallback callback);
   void GetGlobalUsage(GlobalUsageCallback callback);
   void GetHostUsageWithBreakdown(const std::string& host,
                                  UsageWithBreakdownCallback callback);
@@ -79,8 +78,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) UsageTracker
   struct AccumulateInfo;
   friend class ClientUsageTracker;
 
-  void AccumulateClientGlobalLimitedUsage(AccumulateInfo* info,
-                                          int64_t limited_usage);
   void AccumulateClientGlobalUsage(AccumulateInfo* info,
                                    int64_t usage,
                                    int64_t unlimited_usage);
@@ -98,7 +95,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) UsageTracker
       client_tracker_map_;
   size_t client_count_;
 
-  std::vector<UsageCallback> global_limited_usage_callbacks_;
   std::vector<GlobalUsageCallback> global_usage_callbacks_;
   std::map<std::string, std::vector<UsageWithBreakdownCallback>>
       host_usage_callbacks_;

storage/browser/quota/usage_tracker_unittest.cc

@@ -37,12 +37,6 @@ void DidGetGlobalUsage(bool* done,
   *unlimited_usage_out = unlimited_usage;
 }
 
-void DidGetUsage(bool* done, int64_t* usage_out, int64_t usage) {
-  EXPECT_FALSE(*done);
-  *done = true;
-  *usage_out = usage;
-}
-
 class UsageTrackerTestQuotaClient : public QuotaClient {
  public:
   UsageTrackerTestQuotaClient() = default;
@@ -159,15 +153,6 @@ class UsageTrackerTest : public testing::Test {
     quota_client_->UpdateUsage(origin, delta);
   }
 
-  void GetGlobalLimitedUsage(int64_t* limited_usage) {
-    bool done = false;
-    usage_tracker_.GetGlobalLimitedUsage(
-        base::BindOnce(&DidGetUsage, &done, limited_usage));
-    base::RunLoop().RunUntilIdle();
-
-    EXPECT_TRUE(done);
-  }
-
   void GetGlobalUsage(int64_t* usage, int64_t* unlimited_usage) {
     bool done = false;
     usage_tracker_.GetGlobalUsage(
@@ -329,7 +314,7 @@ TEST_F(UsageTrackerTest, CacheDisabledClientTest) {
   EXPECT_EQ(host_usage_breakdown_expected, host_usage_breakdown.second);
 }
 
-TEST_F(UsageTrackerTest, LimitedGlobalUsageTest) {
+TEST_F(UsageTrackerTest, GlobalUsageUnlimitedUncached) {
   const url::Origin kNormal = url::Origin::Create(GURL("http://normal"));
   const url::Origin kUnlimited = url::Origin::Create(GURL("http://unlimited"));
   const url::Origin kNonCached = url::Origin::Create(GURL("http://non_cached"));
@@ -347,24 +332,113 @@ TEST_F(UsageTrackerTest, LimitedGlobalUsageTest) {
   UpdateUsageWithoutNotification(kNonCached, 4);
   UpdateUsageWithoutNotification(kNonCachedUnlimited, 8);
 
-  int64_t limited_usage = 0;
   int64_t total_usage = 0;
   int64_t unlimited_usage = 0;
 
-  GetGlobalLimitedUsage(&limited_usage);
   GetGlobalUsage(&total_usage, &unlimited_usage);
-  EXPECT_EQ(1 + 4, limited_usage);
   EXPECT_EQ(1 + 2 + 4 + 8, total_usage);
   EXPECT_EQ(2 + 8, unlimited_usage);
 
   UpdateUsageWithoutNotification(kNonCached, 16 - 4);
   UpdateUsageWithoutNotification(kNonCachedUnlimited, 32 - 8);
 
-  GetGlobalLimitedUsage(&limited_usage);
   GetGlobalUsage(&total_usage, &unlimited_usage);
-  EXPECT_EQ(1 + 16, limited_usage);
   EXPECT_EQ(1 + 2 + 16 + 32, total_usage);
   EXPECT_EQ(2 + 32, unlimited_usage);
 }
 
+TEST_F(UsageTrackerTest, GlobalUsageMultipleOriginsPerHostCachedInit) {
+  const url::Origin kOrigin1 = url::Origin::Create(GURL("http://example.com"));
+  const url::Origin kOrigin2 =
+      url::Origin::Create(GURL("http://example.com:8080"));
+  ASSERT_EQ(kOrigin1.host(), kOrigin2.host())
+      << "The test assumes that the two origins have the same host";
+
+  UpdateUsageWithoutNotification(kOrigin1, 100);
+  UpdateUsageWithoutNotification(kOrigin2, 200);
+
+  int64_t total_usage = 0;
+  int64_t unlimited_usage = 0;
+  // GetGlobalUsage() takes different code paths on the first call and on
+  // subsequent calls. This test covers the code path used by the first call.
+  // Therefore, we introduce the origins before the first call.
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(100 + 200, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+}
+
+TEST_F(UsageTrackerTest, GlobalUsageMultipleOriginsPerHostCachedUpdate) {
+  const url::Origin kOrigin1 = url::Origin::Create(GURL("http://example.com"));
+  const url::Origin kOrigin2 =
+      url::Origin::Create(GURL("http://example.com:8080"));
+  ASSERT_EQ(kOrigin1.host(), kOrigin2.host())
+      << "The test assumes that the two origins have the same host";
+
+  int64_t total_usage = 0;
+  int64_t unlimited_usage = 0;
+  // GetGlobalUsage() takes different code paths on the first call and on
+  // subsequent calls. This test covers the code path used by subsequent calls.
+  // Therefore, we introduce the origins after the first call.
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(0, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+
+  UpdateUsage(kOrigin1, 100);
+  UpdateUsage(kOrigin2, 200);
+
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(100 + 200, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+}
+
+TEST_F(UsageTrackerTest, GlobalUsageMultipleOriginsPerHostUncachedInit) {
+  const url::Origin kOrigin1 = url::Origin::Create(GURL("http://example.com"));
+  const url::Origin kOrigin2 =
+      url::Origin::Create(GURL("http://example.com:8080"));
+  ASSERT_EQ(kOrigin1.host(), kOrigin2.host())
+      << "The test assumes that the two origins have the same host";
+
+  SetUsageCacheEnabled(kOrigin1, false);
+  SetUsageCacheEnabled(kOrigin2, false);
+
+  UpdateUsageWithoutNotification(kOrigin1, 100);
+  UpdateUsageWithoutNotification(kOrigin2, 200);
+
+  int64_t total_usage = 0;
+  int64_t unlimited_usage = 0;
+  // GetGlobalUsage() takes different code paths on the first call and on
+  // subsequent calls. This test covers the code path used by the first call.
+  // Therefore, we introduce the origins before the first call.
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(100 + 200, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+}
+
+TEST_F(UsageTrackerTest, GlobalUsageMultipleOriginsPerHostUncachedUpdate) {
+  const url::Origin kOrigin1 = url::Origin::Create(GURL("http://example.com"));
+  const url::Origin kOrigin2 =
+      url::Origin::Create(GURL("http://example.com:8080"));
+  ASSERT_EQ(kOrigin1.host(), kOrigin2.host())
+      << "The test assumes that the two origins have the same host";
+
+  int64_t total_usage = 0;
+  int64_t unlimited_usage = 0;
+  // GetGlobalUsage() takes different code paths on the first call and on
+  // subsequent calls. This test covers the code path used by subsequent calls.
+  // Therefore, we introduce the origins after the first call.
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(0, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+
+  SetUsageCacheEnabled(kOrigin1, false);
+  SetUsageCacheEnabled(kOrigin2, false);
+
+  UpdateUsageWithoutNotification(kOrigin1, 100);
+  UpdateUsageWithoutNotification(kOrigin2, 200);
+
+  GetGlobalUsage(&total_usage, &unlimited_usage);
+  EXPECT_EQ(100 + 200, total_usage);
+  EXPECT_EQ(0, unlimited_usage);
+}
+
 }  // namespace storage