Use bytes instead of no. of partitioned cookies for per-domain limit

After the discussion on privacycg/CHIPS#48 we have decided to change the way we limit domains' partitioned cookies in a single partition.

Prior to this change, we restricted domains to 10 cookies per partition.

After this change, domains' cookies' names and values may only occupy 10 kilobytes and we impose an additional 180 cookie limit.

Bug: 1373515
Change-Id: Id3169404ea8d62dea71999ad702ca89d0ad10577
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3946918
Commit-Queue: Dylan Cutler <[email protected]>
Reviewed-by: Chris Fredrickson <[email protected]>
Reviewed-by: Maks Orlovich <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1060253}

Author: DCtheTall

net/cookies/cookie_monster.cc

@@ -162,13 +162,33 @@ bool IncludeUnpartitionedCookies(
   return false;
 }
 
-size_t NameValueSizeBytes(const std::string& name, const std::string& value) {
-  base::CheckedNumeric<size_t> name_value_pair_size = name.size();
-  name_value_pair_size += value.size();
+size_t NameValueSizeBytes(const net::CanonicalCookie& cc) {
+  base::CheckedNumeric<size_t> name_value_pair_size = cc.Name().size();
+  name_value_pair_size += cc.Value().size();
   DCHECK(name_value_pair_size.IsValid());
   return name_value_pair_size.ValueOrDie();
 }
 
+size_t NumBytesInCookieMapForKey(
+    const net::CookieMonster::CookieMap& cookie_map,
+    const std::string& key) {
+  size_t result = 0;
+  auto range = cookie_map.equal_range(key);
+  for (auto it = range.first; it != range.second; ++it) {
+    result += NameValueSizeBytes(*it->second);
+  }
+  return result;
+}
+
+size_t NumBytesInCookieItVector(
+    const net::CookieMonster::CookieItVector& cookie_its) {
+  size_t result = 0;
+  for (const auto& it : cookie_its) {
+    result += NameValueSizeBytes(*it->second);
+  }
+  return result;
+}
+
 }  // namespace
 
 namespace net {
@@ -182,7 +202,8 @@ const size_t CookieMonster::kPurgeCookies = 300;
 
 const size_t CookieMonster::kMaxDomainPurgedKeys = 100;
 
-const size_t CookieMonster::kPerPartitionDomainMaxCookies = 10;
+const size_t CookieMonster::kPerPartitionDomainMaxCookieBytes = 10240;
+const size_t CookieMonster::kPerPartitionDomainMaxCookies = 180;
 
 const size_t CookieMonster::kDomainCookiesQuotaLow = 30;
 const size_t CookieMonster::kDomainCookiesQuotaMedium = 50;
@@ -1560,7 +1581,7 @@ void CookieMonster::SetCanonicalCookie(
 
     if (cc->IsEffectivelySameSiteNone()) {
       UMA_HISTOGRAM_COUNTS_10000("Cookie.SameSiteNoneSizeBytes",
-                                 NameValueSizeBytes(cc->Name(), cc->Value()));
+                                 NameValueSizeBytes(*cc));
     }
 
     bool is_partitioned_cookie = cc->IsPartitioned();
@@ -1981,26 +2002,30 @@ size_t CookieMonster::GarbageCollectPartitionedCookies(
   if (cookie_partition_it == partitioned_cookies_.end())
     return num_deleted;
 
-  if (cookie_partition_it->second->count(key) > kPerPartitionDomainMaxCookies) {
+  if (NumBytesInCookieMapForKey(*cookie_partition_it->second.get(), key) >
+          kPerPartitionDomainMaxCookieBytes ||
+      cookie_partition_it->second->count(key) > kPerPartitionDomainMaxCookies) {
     // TODO(crbug.com/1225444): Log garbage collection for partitioned cookies.
 
     CookieItVector non_expired_cookie_its;
     num_deleted += GarbageCollectExpiredPartitionedCookies(
         current, cookie_partition_it,
         cookie_partition_it->second->equal_range(key), &non_expired_cookie_its);
 
-    if (non_expired_cookie_its.size() > kPerPartitionDomainMaxCookies) {
+    size_t bytes_used = NumBytesInCookieItVector(non_expired_cookie_its);
+
+    if (bytes_used > kPerPartitionDomainMaxCookieBytes ||
+        non_expired_cookie_its.size() > kPerPartitionDomainMaxCookies) {
       // TODO(crbug.com/1225444): Log deep garbage collection for partitioned
       // cookies.
-
-      // For now, just delete the least recently accessed partition cookies
-      // until we are under the per-partition domain limit. All partitioned
-      // cookies are Secure since they require the __Host- prefix.
       std::sort(non_expired_cookie_its.begin(), non_expired_cookie_its.end(),
                 LRACookieSorter);
+
       for (size_t i = 0;
-           i < (non_expired_cookie_its.size() - kPerPartitionDomainMaxCookies);
+           bytes_used > kPerPartitionDomainMaxCookieBytes ||
+           non_expired_cookie_its.size() - i > kPerPartitionDomainMaxCookies;
            ++i) {
+        bytes_used -= NameValueSizeBytes(*non_expired_cookie_its[i]->second);
         InternalDeletePartitionedCookie(
             cookie_partition_it, non_expired_cookie_its[i], true,
             DELETE_COOKIE_EVICTED_PER_PARTITION_DOMAIN);

net/cookies/cookie_monster.h

@@ -139,6 +139,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   static const size_t kMaxDomainPurgedKeys;
 
   // Partitioned cookie garbage collection thresholds.
+  static const size_t kPerPartitionDomainMaxCookieBytes;
   static const size_t kPerPartitionDomainMaxCookies;
   // TODO(crbug.com/1225444): Add global limit to number of partitioned cookies.
 

net/cookies/cookie_monster_unittest.cc

@@ -923,29 +923,6 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
                            70U);
   }
 
-  // Test enforcement of the per-partition domain limit on partitioned cookies.
-  void TestPartitionedCookiesGarbageCollectionHelper() {
-    DCHECK_EQ(10u, CookieMonster::kPerPartitionDomainMaxCookies);
-    int max_cookies = CookieMonster::kPerPartitionDomainMaxCookies;
-    auto cm = std::make_unique<CookieMonster>(nullptr, net::NetLog::Get());
-
-    auto cookie_partition_key =
-        CookiePartitionKey::FromURLForTesting(GURL("https://toplevelsite.com"));
-    for (int i = 0; i < max_cookies + 5; ++i) {
-      std::string cookie = base::StringPrintf("__Host-a%02d=b", i);
-      EXPECT_TRUE(SetCookie(cm.get(), https_www_foo_.url(),
-                            cookie + "; secure; path=/; partitioned",
-                            cookie_partition_key));
-      std::string cookies =
-          this->GetCookies(cm.get(), https_www_foo_.url(),
-                           CookiePartitionKeyCollection(cookie_partition_key));
-      EXPECT_NE(cookies.find(cookie), std::string::npos);
-      EXPECT_LE(CountInString(cookies, '='), max_cookies);
-    }
-    // TODO(crbug.com/1225444): Test recording stats for deleting partitioned
-    // cookies.
-  }
-
   // Function for creating a CM with a number of cookies in it,
   // no store (and hence no ability to affect access time).
   std::unique_ptr<CookieMonster> CreateMonsterForGC(int num_cookies) {
@@ -1780,8 +1757,64 @@ TEST_F(CookieMonsterTest, TestPriorityAwareGarbageCollectionMixed) {
   TestPriorityAwareGarbageCollectHelperMixed();
 }
 
-TEST_F(CookieMonsterTest, TestPartitionedCookiesGarbageCollection) {
-  TestPartitionedCookiesGarbageCollectionHelper();
+TEST_F(CookieMonsterTest, TestPartitionedCookiesGarbageCollection_Memory) {
+  // Limit should be 10 KB.
+  DCHECK_EQ(1024u * 10u, CookieMonster::kPerPartitionDomainMaxCookieBytes);
+
+  auto cm = std::make_unique<CookieMonster>(nullptr, net::NetLog::Get());
+  auto cookie_partition_key =
+      CookiePartitionKey::FromURLForTesting(GURL("https://toplevelsite1.com"));
+
+  for (size_t i = 0; i < 41; ++i) {
+    std::string cookie_value((10240 / 40) - (i < 10 ? 1 : 2), '0');
+    std::string cookie =
+        base::StrCat({base::NumberToString(i), "=", cookie_value});
+    EXPECT_TRUE(SetCookie(cm.get(), https_www_foo_.url(),
+                          cookie + "; secure; path=/; partitioned",
+                          cookie_partition_key))
+        << "Failed to set cookie " << i;
+  }
+
+  std::string cookies =
+      this->GetCookies(cm.get(), https_www_foo_.url(),
+                       CookiePartitionKeyCollection(cookie_partition_key));
+
+  EXPECT_THAT(cookies, CookieStringIs(
+                           testing::Not(testing::Contains(testing::Key("0")))));
+  for (size_t i = 1; i < 41; ++i) {
+    EXPECT_THAT(cookies, CookieStringIs(testing::Contains(
+                             testing::Key(base::NumberToString(i)))))
+        << "Failed to find cookie " << i;
+  }
+}
+
+TEST_F(CookieMonsterTest, TestPartitionedCookiesGarbageCollection_MaxCookies) {
+  // Partitioned cookies also limit domains to 180 cookies per partition.
+  DCHECK_EQ(180u, CookieMonster::kPerPartitionDomainMaxCookies);
+
+  auto cm = std::make_unique<CookieMonster>(nullptr, net::NetLog::Get());
+  auto cookie_partition_key =
+      CookiePartitionKey::FromURLForTesting(GURL("https://toplevelsite.com"));
+
+  for (size_t i = 0; i < 181; ++i) {
+    std::string cookie = base::StrCat({base::NumberToString(i), "=0"});
+    EXPECT_TRUE(SetCookie(cm.get(), https_www_foo_.url(),
+                          cookie + "; secure; path=/; partitioned",
+                          cookie_partition_key))
+        << "Failed to set cookie " << i;
+  }
+
+  std::string cookies =
+      this->GetCookies(cm.get(), https_www_foo_.url(),
+                       CookiePartitionKeyCollection(cookie_partition_key));
+  EXPECT_THAT(cookies, CookieStringIs(
+                           testing::Not(testing::Contains(testing::Key("0")))));
+  for (size_t i = 1; i < 181; ++i) {
+    std::string cookie = base::StrCat({base::NumberToString(i), "=0"});
+    EXPECT_THAT(cookies, CookieStringIs(testing::Contains(
+                             testing::Key(base::NumberToString(i)))))
+        << "Failed to find cookie " << i;
+  }
 }
 
 TEST_F(CookieMonsterTest, SetCookieableSchemes) {