Quota: Remove available disk space from host quota calculation

This change alters how per host quota is calculated. Specifically, there is a case where users have low disk space availability where the quota is based off available disk space. This change ignores that branch of logic for users that have the StaticHostQuota Finch study enabled. The study is disabled by default, but will be enabled for 100% of users, starting with Dev/Canary and moving up the channels to Stable. Keeping this behind a Finch study allows us to disable this change with a single line change to Finch.

Bug: 965710
Change-Id: Icd5ed59c1c592ccd9554395427f6707210376271
Signed-off-by: Jarryd Goodman <jarrydg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625744Reviewed-by: Nik Bhagat <nikunjb@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663795}

storage/browser/quota/quota_features.cc

@@ -27,5 +27,13 @@ constexpr base::FeatureParam<double> kExperimentalPoolSizeRatio{
 
 constexpr base::FeatureParam<double> kPerHostRatio{&kQuotaExpandPoolSize,
                                                    "PerHostRatio", 0.2};
+
+// StaticHostQuota enables a simpler per-host quota model, where the quota is
+// only based on disk capacity (partition size). When the flag is disabled, the
+// quota computation takes into account free disk space, in addition to the
+// disk's total capacity.
+const base::Feature kStaticHostQuota{"StaticHostQuota",
+                                     base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace storage

storage/browser/quota/quota_features.h

@@ -18,6 +18,9 @@ extern const base::Feature kQuotaExpandPoolSize;
 extern const base::FeatureParam<double> kExperimentalPoolSizeRatio;
 extern const base::FeatureParam<double> kPerHostRatio;
 
+COMPONENT_EXPORT(STORAGE_BROWSER)
+extern const base::Feature kStaticHostQuota;
+
 }  // namespace features
 
 }  // namespace storage

storage/browser/quota/quota_manager.cc

@@ -17,6 +17,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/numerics/safe_conversions.h"
@@ -33,6 +34,7 @@
 #include "base/trace_event/trace_event.h"
 #include "net/base/url_util.h"
 #include "storage/browser/quota/client_usage_tracker.h"
+#include "storage/browser/quota/quota_features.h"
 #include "storage/browser/quota/quota_macros.h"
 #include "storage/browser/quota/quota_manager_proxy.h"
 #include "storage/browser/quota/quota_temporary_storage_evictor.h"
@@ -272,14 +274,18 @@ class QuotaManager::UsageAndQuotaInfoGatherer : public QuotaTask {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     weak_factory_.InvalidateWeakPtrs();
 
-    // Constrain the desired |host_quota| to something that fits.
-    // If available space is too low, cap usage at current levels.
-    // If it's close to being too low, cap growth to avoid it getting too low.
-    int64_t host_quota =
-        std::min(desired_host_quota_,
-                 host_usage_ +
-                     std::max(INT64_C(0), available_space_ -
-                                              settings_.must_remain_available));
+    int64_t host_quota = desired_host_quota_;
+
+    if (!base::FeatureList::IsEnabled(features::kStaticHostQuota)) {
+      // Constrain the desired |host_quota| to something that fits.
+      // If available space is too low, cap usage at current levels.
+      // If it's close to being too low, cap growth to avoid it getting too low.
+      int64_t temp_pool_free_space =
+          std::max(static_cast<int64_t>(0),
+                   available_space_ - settings_.must_remain_available);
+      host_quota = std::min(host_quota, temp_pool_free_space + host_usage_);
+    }
+
     std::move(callback_).Run(blink::mojom::QuotaStatusCode::kOk, host_usage_,
                              host_quota, std::move(host_usage_breakdown_));
     if (type_ == StorageType::kTemporary && !is_incognito_ && !is_unlimited_) {

storage/browser/quota/quota_manager_unittest.cc

@@ -22,10 +22,12 @@
 #include "base/stl_util.h"
 #include "base/system/sys_info.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "storage/browser/quota/quota_database.h"
+#include "storage/browser/quota/quota_features.h"
 #include "storage/browser/quota/quota_manager.h"
 #include "storage/browser/quota/quota_manager_proxy.h"
 #include "storage/browser/test/mock_special_storage_policy.h"
@@ -480,6 +482,7 @@ class QuotaManagerTest : public testing::Test {
   void reset_status_callback_count() { status_callback_count_ = 0; }
 
  protected:
+  base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
  private:
@@ -1218,6 +1221,69 @@ TEST_F(QuotaManagerTest, GetAndSetPersistentUsageAndQuota) {
   EXPECT_EQ(QuotaManager::kNoLimit, quota());
 }
 
+TEST_F(QuotaManagerTest, GetQuotaLowAvailableDiskSpace) {
+  static const MockOriginData kData[] = {
+      {"http://foo.com/", kTemp, 100000},
+      {"http://unlimited/", kTemp, 4000000},
+  };
+
+  MockStorageClient* client =
+      CreateClient(kData, base::size(kData), QuotaClient::kFileSystem);
+  RegisterClient(client);
+
+  const int kPoolSize = 10000000;
+  const int kPerHostQuota = kPoolSize / 5;
+
+  // Simulating a low available disk space scenario by making
+  // kMustRemainAvailable 64KB less than GetAvailableDiskSpaceForTest(), which
+  // means there is 64KB of storage quota that can be used before triggering
+  // the low available space logic branch in quota_manager.cc. From the
+  // perspective of QuotaManager, there are 64KB of free space in the temporary
+  // pool, so it should return (64KB + usage) as quota since the sum is less
+  // than the default host quota.
+  const int kMustRemainAvailable =
+      static_cast<int>(GetAvailableDiskSpaceForTest() - 65536);
+  SetQuotaSettings(kPoolSize, kPerHostQuota, kMustRemainAvailable);
+
+  GetUsageAndQuotaForWebApps(ToOrigin("http://foo.com/"), kTemp);
+  scoped_task_environment_.RunUntilIdle();
+  EXPECT_EQ(QuotaStatusCode::kOk, status());
+  EXPECT_EQ(100000, usage());
+  EXPECT_GT(kPerHostQuota, quota());
+  EXPECT_EQ(65536 + usage(), quota());
+}
+
+TEST_F(QuotaManagerTest, GetStaticQuotaLowAvailableDiskSpace) {
+  // This test is the same as the previous but with the kStaticHostQuota Finch
+  // feature enabled. In here, we expect the low available space logic branch
+  // to be ignored. Doing so should have QuotaManager return the same per host
+  // quota as what is set in QuotaSettings, despite being in a state of low
+  // available space. Notice the different expectation in the last line of
+  // each test.
+  scoped_feature_list_.InitAndEnableFeature(
+      storage::features::kStaticHostQuota);
+  static const MockOriginData kData[] = {
+      {"http://foo.com/", kTemp, 100000},
+      {"http://unlimited/", kTemp, 4000000},
+  };
+
+  MockStorageClient* client =
+      CreateClient(kData, base::size(kData), QuotaClient::kFileSystem);
+  RegisterClient(client);
+
+  const int kPoolSize = 10000000;
+  const int kPerHostQuota = kPoolSize / 5;
+  const int kMustRemainAvailable =
+      static_cast<int>(GetAvailableDiskSpaceForTest() - 65536);
+  SetQuotaSettings(kPoolSize, kPerHostQuota, kMustRemainAvailable);
+
+  GetUsageAndQuotaForWebApps(ToOrigin("http://foo.com/"), kTemp);
+  scoped_task_environment_.RunUntilIdle();
+  EXPECT_EQ(QuotaStatusCode::kOk, status());
+  EXPECT_EQ(100000, usage());
+  EXPECT_EQ(kPerHostQuota, quota());
+}
+
 TEST_F(QuotaManagerTest, GetSyncableQuota) {
   RegisterClient(CreateClient(nullptr, 0, QuotaClient::kFileSystem));
 

testing/variations/fieldtrial_testing_config.json

@@ -5036,6 +5036,25 @@
             ]
         }
     ],
+    "StaticHostQuota": [
+        {
+            "platforms": [
+                "android",
+                "chromeos",
+                "linux",
+                "mac",
+                "windows"
+            ],
+            "experiments": [
+                {
+                    "name": "Enabled",
+                    "enable_features": [
+                        "StaticHostQuota"
+                    ]
+                }
+            ]
+        }
+    ],
     "StopLoadingInBackground": [
         {
             "platforms": [