Strict Secure Cookies re-implemented to be priority aware

net/cookies/cookie_monster.cc

Index: net/cookies/cookie_monster.cc
diff --git a/net/cookies/cookie_monster.cc b/net/cookies/cookie_monster.cc
index e208d332882e58ecfd09c6d0c88baa6916168a88..e87ed2c661deba34a9f374394088956f651adc40 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -309,6 +309,24 @@ void RunAsync(scoped_refptr<base::TaskRunner> proxy,
   proxy->PostTask(FROM_HERE, base::Bind(callback, cookie, removed));
 }
 
+bool IsCookieEligibleForEviction(CookiePriority current_priority_level,
+                                 bool protect_secure_cookies,
+                                 const CanonicalCookie* cookie) {
+  if (!cookie->IsSecure() || !protect_secure_cookies)
+    return cookie->Priority() <= current_priority_level;
+
+  // Special consideration has to be given for low-priority secure cookies since
+  // they are given lower prority than non-secure medium-priority and non-secure
+  // high-priority cookies. Thus, low-priority secure cookies may be evicted at
+  // a medium and high value of |current_priority_level|. Put another way,
+  // low-priority secure cookies are only protected when the current priority
+  // level is low.
+  if (current_priority_level == COOKIE_PRIORITY_LOW)
+    return false;
+
+  return cookie->Priority() == COOKIE_PRIORITY_LOW;
+}

[Mike West, 2016/05/04 08:08:19]

This is clever. It wasn't at all how I'd considered approaching the problem;
it's better.

[jww, 2016/05/05 18:56:26]

Yay! Thanks.

+
 }  // namespace
 
 CookieMonster::CookieMonster(PersistentCookieStore* store,
@@ -1869,15 +1887,6 @@ size_t CookieMonster::GarbageCollect(const Time& current,
     num_deleted +=
         GarbageCollectExpired(current, cookies_.equal_range(key), cookie_its);
 
-    // TODO(mkwst): Soften this.
-    CookieItVector secure_cookie_its;
-    if (enforce_strict_secure && cookie_its->size() > kDomainMaxCookies) {
-      VLOG(kVlogGarbageCollection) << "Garbage collecting non-Secure cookies.";
-      num_deleted +=
-          GarbageCollectNonSecure(non_expired_cookie_its, &secure_cookie_its);

[Mike West, 2016/05/04 08:08:19]

Nit: I think this was the only callsite to `GarbageCollectNonSecure()`, right?
Can we remove the method entirely?

[jww, 2016/05/05 18:56:26]

Done.

-      cookie_its = &secure_cookie_its;
-    }
-
     if (cookie_its->size() > kDomainMaxCookies) {
       VLOG(kVlogGarbageCollection) << "Deep Garbage Collect domain.";
       size_t purge_goal =
@@ -1892,17 +1901,55 @@ size_t CookieMonster::GarbageCollect(const Time& current,
       // bump the quota and remove low- and medium-priority. Then, if cookies
       // _still_ need to be removed, bump the quota and remove cookies with
       // any priority.
-      const size_t kQuotas[3] = {kDomainCookiesQuotaLow,
-                                 kDomainCookiesQuotaMedium,
-                                 kDomainCookiesQuotaHigh};
+      //
+      // 1.  Low-priority non-secure cookies.
+      // 2.  Low-priority secure cookies.
+      // 3.  Medium-priority non-secure cookies.
+      // 4.  High-priority non-secure cookies.
+      // 5.  Medium-priority secure cookies.
+      // 6.  High-priority secure cookies.
+      const static struct {
+        CookiePriority priority;
+        size_t quota;

[Mike West, 2016/05/04 08:08:19]

Nit: Maybe "additionalQuota"? I missed the `+=` bit at first, and was confused
about why the secure rounds had `0u` as their quota.

[jww, 2016/05/05 18:56:26]

Done.

+        bool protect_secure_cookies;
+      } purge_rounds[] = {
+          // 1.  Low-priority non-secure cookies.
+          {COOKIE_PRIORITY_LOW, kDomainCookiesQuotaLow, true},
+          // 2.  Low-priority secure cookies.
+          {COOKIE_PRIORITY_LOW, 0U, false},

[Mike West, 2016/05/04 08:08:19]

Tiny nit: lowercase 'u', here and below.

[jww, 2016/05/05 18:56:26]

Done.

+          // 3.  Medium-priority non-secure cookies.
+          {COOKIE_PRIORITY_MEDIUM, kDomainCookiesQuotaMedium, true},
+          // 4.  High-priority non-secure cookies.
+          {COOKIE_PRIORITY_HIGH, kDomainCookiesQuotaHigh, true},
+          // 5.  Medium-priority secure cookies.
+          {COOKIE_PRIORITY_MEDIUM, 0U, false},
+          // 6.  High-priority secure cookies.
+          {COOKIE_PRIORITY_HIGH, 0U, false},
+      };
+
       size_t quota = 0;
-      for (size_t i = 0; i < arraysize(kQuotas) && purge_goal > 0; i++) {
-        quota += kQuotas[i];
-        size_t just_deleted = PurgeLeastRecentMatches(
-            cookie_its, static_cast<CookiePriority>(i), quota, purge_goal);
-        DCHECK_LE(just_deleted, purge_goal);
-        purge_goal -= just_deleted;
-        num_deleted += just_deleted;
+      for (size_t i = 0; i < arraysize(purge_rounds) && purge_goal > 0; i++) {

[Mike West, 2016/05/04 08:08:19]

Nit: C++11 `for`? We'd used a regular `for` loop in the previous iteration
because we needed `i` to cast into a `CookiePriority`. Now that the priority is
part of the struct, can we be all fancy and modern? :)

The `purge_goal > 0` bit should be covered by the DCHECK_LE on 1949, and the
DCHECK on 1955, right?

[jww, 2016/05/05 18:56:26]

Done.

+        quota += purge_rounds[i].quota;
+        size_t just_deleted = 0u;
+
+        // Only observe the non-secure purge rounds if strict secure cookies is
+        // enabled.
+        if (!enforce_strict_secure && purge_rounds[i].protect_secure_cookies)
+          continue;
+
+        // Purge up to |purge_goal| for all cookies at the given priority. This
+        // path will always execute if strict secure cookies is disabled since
+        // |purge_goal| must be positive because of the for-loop guard. If
+        // strict secure cookies is enabled, this path will be taken only if the
+        // initial non-secure purge did not evict enough cookies.
+        if (purge_goal > 0) {
+          just_deleted = PurgeLeastRecentMatches(
+              cookie_its, purge_rounds[i].priority, quota, purge_goal,
+              purge_rounds[i].protect_secure_cookies);
+          DCHECK_LE(just_deleted, purge_goal);
+          purge_goal -= just_deleted;
+          num_deleted += just_deleted;
+        }
       }
 
       DCHECK_EQ(0U, purge_goal);
@@ -1955,12 +2002,43 @@ size_t CookieMonster::GarbageCollect(const Time& current,
 size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies,
                                               CookiePriority priority,
                                               size_t to_protect,
-                                              size_t purge_goal) {
+                                              size_t purge_goal,
+                                              bool protect_secure_cookies) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Find the first protected cookie by walking down from the end of the list
   // cookie list (most-recently accessed) until |to_protect| cookies that match
   // |priority| are found.
+  //
+  // If |protect_secure_cookies| is true, do a first pass that counts eligible
+  // secure cookies at the specified priority as protected.
+  if (protect_secure_cookies) {
+    for (size_t i = 0; to_protect > 0 && i < cookies->size(); i++) {
+      if (!cookies->at(i)->second->IsSecure())
+        continue;
+      // 1) At low-priority, only low-priority secure cookies are protected as
+      //    part of the quota.
+      // 2) At medium-priority, only medium-priority secure cookies are
+      //    protected as part of the quota (low-priority secure cookies may be
+      //    deleted).
+      // 3) At high-priority, medium-priority and high-priority secure cookies
+      //    are protected as part of the quota (low-priority secure cookies may
+      //    be deleted).
+      CookiePriority cookie_priority = cookies->at(i)->second->Priority();
+      switch (cookie_priority) {
+        case COOKIE_PRIORITY_LOW:
+          if (priority == COOKIE_PRIORITY_LOW)
+            to_protect--;
+          break;
+        case COOKIE_PRIORITY_MEDIUM:
+        case COOKIE_PRIORITY_HIGH:
+          if (cookie_priority <= priority)
+            to_protect--;
+          break;
+      }
+    }
+  }

[Mike West, 2016/05/04 08:08:19]

Nit: This took me longer than I care to admit to wrap my head around. I think
this might be clearer if you broke it out into a helper function named something
like `CountProtectedSecureCookiesForPriority`, and did something like:

```
if (protect_secure_cookies)
  to_protect -= std::min(to_protect,
CountProtectedSecureCookiesForPriority(cookies, priority));
```

[jww, 2016/05/05 18:56:26]

Done.

+
   size_t protection_boundary = cookies->size();
   while (to_protect > 0 && protection_boundary > 0) {
     protection_boundary--;
@@ -1974,7 +2052,12 @@ size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies,
   size_t removed = 0;
   size_t current = 0;
   while (removed < purge_goal && current < protection_boundary) {
-    if (cookies->at(current)->second->Priority() <= priority) {
+    const CanonicalCookie* current_cookie = cookies->at(current)->second;
+    // Only delete the current cookie if the priority is less than or equal to
+    // the current level. If it is equal to the current level, and secure
+    // cookies are protected, only delete it if it is not secure.
+    if (IsCookieEligibleForEviction(priority, protect_secure_cookies,
+                                    current_cookie)) {
       InternalDeleteCookie(cookies->at(current), true,
                            DELETE_COOKIE_EVICTED_DOMAIN);
       cookies->erase(cookies->begin() + current);