Making cookies eviction quotas match spec

net/cookies/cookie_monster.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 291 matching lines @@
     // DELETE_COOKIE_LAST_ENTRY
     {CookieMonsterDelegate::CHANGE_COOKIE_EXPLICIT, false}};
 
 void RunAsync(scoped_refptr<base::TaskRunner> proxy,
               const CookieStore::CookieChangedCallback& callback,
               const CanonicalCookie& cookie,
               bool removed) {
   proxy->PostTask(FROM_HERE, base::Bind(callback, cookie, removed));
 }
 
-size_t CountProtectedSecureCookiesAtPriority(
-    CookiePriority priority,
-    CookieMonster::CookieItVector* cookies) {
-  size_t num_protected_secure_cookies = 0;
-  for (const auto& cookie : *cookies) {
-    if (!cookie->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 = cookie->second->Priority();
-    switch (cookie_priority) {
-      case COOKIE_PRIORITY_LOW:
-        if (priority == COOKIE_PRIORITY_LOW)
-          num_protected_secure_cookies++;
-        break;
-      case COOKIE_PRIORITY_MEDIUM:
-      case COOKIE_PRIORITY_HIGH:
-        if (cookie_priority <= priority)
-          num_protected_secure_cookies++;
-        break;
-    }
-  }
-
-  return num_protected_secure_cookies;
-}
-
 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;
+  if (cookie->Priority() == current_priority_level && protect_secure_cookies)
+    return !cookie->IsSecure();
 
-  // 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() == current_priority_level;
+}
 
-  return cookie->Priority() == COOKIE_PRIORITY_LOW;
+size_t CountCookiesForPossibleDeletion(
+    CookiePriority priority,
+    const CookieMonster::CookieItVector* cookies,
+    bool protect_secure_cookies = false) {

[jww, 2016/05/26 03:46:39]

Please do not use a default argument here; it caused me a bit of confusion
below, especially since there are only two call sites, so this doesn't help with
an exceptional case (just pass in protect_secure_cookies at the two call sites
and it will be much clearer).

[maksims (do not use this acc), 2016/05/26 11:30:20]

Done.

+  size_t cookies_count = 0U;
+  for (const auto& cookie : *cookies) {
+    if (cookie->second->Priority() == priority && !protect_secure_cookies)

[jww, 2016/05/26 03:46:39]

Please simplify this conditional. I'm finding the two paths to cookies_count++ a
bit hard to follow. Maybe something like:

if (cookie->second->Priority() == priority) {
  if (!protect_secure_cookies || cookie->second->IsSecure())
    cookies_count++;
}

Also, as a style note, you should have braces on both branches of the
conditional. According to the style guide, braces on one conditional means
braces on all branches.

[maksims (do not use this acc), 2016/05/26 11:30:20]

Then it will be incorrect if I will pass original |protect_secure_cookies| in
two calls. I just want to count the number of secure and non-secure cookies of a
certain priority all together. Thus, |protect_secure_cookies| must always be
false, see the first caller.

+      cookies_count++;
+    else if (cookie->second->Priority() == priority && protect_secure_cookies &&
+             cookie->second->IsSecure()) {
+      cookies_count++;
+    }
+  }
+  return cookies_count;
 }
 
 }  // namespace
 
 CookieMonster::CookieMonster(PersistentCookieStore* store,
                              CookieMonsterDelegate* delegate)
     : CookieMonster(
           store,
           delegate,
           base::TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds)) {}
@@ skipping 1552 matching lines @@
 
     if (cookie_its->size() > kDomainMaxCookies) {
       VLOG(kVlogGarbageCollection) << "Deep Garbage Collect domain.";
       size_t purge_goal =
           cookie_its->size() - (kDomainMaxCookies - kDomainPurgeCookies);
       DCHECK(purge_goal > kDomainPurgeCookies);
 
       // Sort the cookies by access date, from least-recent to most-recent.
       std::sort(cookie_its->begin(), cookie_its->end(), LRACookieSorter);
 
-      size_t additional_quota_low = kDomainCookiesQuotaLow;
-      size_t additional_quota_medium = kDomainCookiesQuotaMedium;
-      size_t additional_quota_high = kDomainCookiesQuotaHigh;
-
       // Remove all but the kDomainCookiesQuotaLow most-recently accessed
       // cookies with low-priority. Then, if cookies still need to be removed,
       // 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.
       //
       // 1.  Low-priority non-secure cookies.
       // 2.  Low-priority secure cookies.
       // 3.  Medium-priority non-secure cookies.
       // 4.  High-priority non-secure cookies.
@@ skipping 21 matching lines @@
       for (const auto& purge_round : purge_rounds) {
         // Only observe the non-secure purge rounds if strict secure cookies is
         // enabled.
         if (!enforce_strict_secure && purge_round.protect_secure_cookies)
           continue;
 
         // Only adjust the quota if the round is executing, otherwise it is
         // necesary to delay quota adjustments until a later round. This is
         // because if the high priority, non-secure round is skipped, its quota
         // should not count until the later high priority, full round later.
-        size_t* additional_quota = nullptr;
         switch (purge_round.priority) {
           case COOKIE_PRIORITY_LOW:
-            additional_quota = &additional_quota_low;
+            quota = kDomainCookiesQuotaLow;
             break;
           case COOKIE_PRIORITY_MEDIUM:
-            additional_quota = &additional_quota_medium;
+            quota = kDomainCookiesQuotaMedium;
             break;
           case COOKIE_PRIORITY_HIGH:
-            additional_quota = &additional_quota_high;
+            quota = kDomainCookiesQuotaHigh;
             break;
         }
-        quota += *additional_quota;
-        *additional_quota = 0u;
         size_t just_deleted = 0u;
-
         // 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_round.priority, quota, purge_goal,
               purge_round.protect_secure_cookies);
           DCHECK_LE(just_deleted, purge_goal);
@@ skipping 49 matching lines @@
   return num_deleted;
 }
 
 size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies,
                                               CookiePriority priority,
                                               size_t to_protect,
                                               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.
+  // 1. Count number of the cookies at |priority|
+  size_t cookies_count_possibly_to_be_deleted =
+      CountCookiesForPossibleDeletion(priority, cookies);
+
+  // 2. If |cookies_count| at |priority| is less than or equal to_protect, skip.
+  // This involves secure and non-secure cookies at |priority|.
+  if (cookies_count_possibly_to_be_deleted <= to_protect)
+    return 0u;
+
+  // 3. Calculate number of secure cookies at |priority|
+  // and Count number of cookies at |priority| that can possibly be deleted.
+  // It is guaranteed we do not delete more than |purge_goal| even if
+  // |priority_cookies_might_be_deleted| is higher.
+  size_t secure_cookies = 0u;
   if (protect_secure_cookies) {
-    to_protect -= std::min(
-        to_protect, CountProtectedSecureCookiesAtPriority(priority, cookies));
+    secure_cookies = CountCookiesForPossibleDeletion(priority, cookies,
+                                                     protect_secure_cookies);
+    cookies_count_possibly_to_be_deleted -= secure_cookies;
+  } else {
+    cookies_count_possibly_to_be_deleted -= to_protect;
   }
 
-  size_t protection_boundary = cookies->size();
-  while (to_protect > 0 && protection_boundary > 0) {
-    protection_boundary--;
-    if (cookies->at(protection_boundary)->second->Priority() <= priority)
-      to_protect--;
-  }
-
-  // Now, walk up from the beginning of the list (least-recently accessed) until
-  // |purge_goal| cookies are removed, or the iterator hits
-  // |protection_boundary|.
-  size_t removed = 0;
-  size_t current = 0;
-  while (removed < purge_goal && current < protection_boundary) {
+  size_t removed = 0u;
+  size_t current = 0u;
+  while ((removed < purge_goal && current < cookies->size()) &&
+         cookies_count_possibly_to_be_deleted > 0) {
     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.
+    // Only delete the current cookie if the priority is equal to
+    // the current level.
     if (IsCookieEligibleForEviction(priority, protect_secure_cookies,
                                     current_cookie)) {
       InternalDeleteCookie(cookies->at(current), true,
                            DELETE_COOKIE_EVICTED_DOMAIN);
       cookies->erase(cookies->begin() + current);
       removed++;
-
+      cookies_count_possibly_to_be_deleted--;
       // The call to 'erase' above shifts the contents of the vector, but
       // doesn't shift |protection_boundary|. Decrement that here to ensure that
-      // the correct set of cookies is protected.
-      protection_boundary--;
+      // the correct set of cookies is protected.;
     } else {
       current++;
     }
   }
   return removed;
 }
 
 size_t CookieMonster::GarbageCollectExpired(const Time& current,
                                             const CookieMapItPair& itpair,
                                             CookieItVector* cookie_its) {
@@ skipping 317 matching lines @@
        it != hook_map_.end(); ++it) {
     std::pair<GURL, std::string> key = it->first;
     if (cookie.IncludeForRequestURL(key.first, opts) &&
         cookie.Name() == key.second) {
       it->second->Notify(cookie, removed);
     }
   }
 }
 
 }  // namespace net