Evict non-secure cookies less agressively.

net/cookies/cookie_monster_unittest.cc

Index: net/cookies/cookie_monster_unittest.cc
diff --git a/net/cookies/cookie_monster_unittest.cc b/net/cookies/cookie_monster_unittest.cc
index 5a485992d8ad56ea6e8533afae25e4c75e8f15df..8c38de507a980097ba7143fc9f9c3f092762f322 100644
--- a/net/cookies/cookie_monster_unittest.cc
+++ b/net/cookies/cookie_monster_unittest.cc
@@ -353,11 +353,12 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
   // Instantiates a CookieMonster, adds multiple cookies (to http_www_google_)
   // with priorities specified by |coded_priority_str|, and tests priority-aware
   // domain cookie eviction.
-  // |coded_priority_str| specifies a run-length-encoded string of priorities.
-  // Example: "2M 3L M 4H" means "MMLLLMHHHH", and speicifies sequential (i.e.,
-  // from least- to most-recently accessed) insertion of 2 medium-priority
-  // cookies, 3 low-priority cookies, 1 medium-priority cookie, and 4
-  // high-priority cookies.
+  //
+  // Example: |coded_priority_string| of "2MN 3LS MN 4HN" specifies sequential
+  // (i.e., from least- to most-recently accessed) insertion of 2
+  // medium-priority non-secure cookies, 3 low-priority secure cookies, 1
+  // medium-priority non-secure cookie, and 4 high-priority non-secure cookies.
+  //
   // Within each priority, only the least-accessed cookies should be evicted.
   // Thus, to describe expected suriving cookies, it suffices to specify the
   // expected population of surviving cookies per priority, i.e.,
@@ -366,41 +367,57 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
                               const std::string& coded_priority_str,
                               size_t expected_low_count,
                               size_t expected_medium_count,
-                              size_t expected_high_count) {
+                              size_t expected_high_count,
+                              size_t expected_nonsecure,
+                              size_t expected_secure) {
+    SCOPED_TRACE(coded_priority_str.c_str());
     this->DeleteAll(cm);
     int next_cookie_id = 0;
-    std::vector<CookiePriority> priority_list;
-    std::vector<int> id_list[3];  // Indexed by CookiePriority.
+    // A list of cookie IDs, indexed by secure status, then by priority.
+    std::vector<std::vector<std::vector<int>>> id_list{{{}, {}, {}},
+                                                       {{}, {}, {}}};
+    // A list of all the cookies stored, along with their properties.
+    std::vector<std::pair<bool, CookiePriority>> cookie_data;
 
     // Parse |coded_priority_str| and add cookies.
     for (const std::string& token :
          base::SplitString(coded_priority_str, " ", base::TRIM_WHITESPACE,
                            base::SPLIT_WANT_ALL)) {
       DCHECK(!token.empty());
-      // Take last character as priority.
-      CookiePriority priority = CharToPriority(token.back());
-      std::string priority_str = CookiePriorityToString(priority);
+
+      // Take last character as security status, then discard it.
+      bool is_secure = token[token.size() - 1] == 'S';
+
+      // The second-to-last character is the priority. Grab and discard it.
+      CookiePriority priority = CharToPriority(token[token.size() - 2]);
+
       // The rest of the string (possibly empty) specifies repetition.
       int rep = 1;
       if (!token.empty()) {
         bool result = base::StringToInt(
-            base::StringPiece(token.begin(), token.end() - 1), &rep);
+            base::StringPiece(token.begin(), token.end() - 2), &rep);
         DCHECK(result);
       }
       for (; rep > 0; --rep, ++next_cookie_id) {
-        std::string cookie = base::StringPrintf(
-            "a%d=b;priority=%s", next_cookie_id, priority_str.c_str());
-        EXPECT_TRUE(SetCookie(cm, http_www_google_.url(), cookie));
-        priority_list.push_back(priority);
-        id_list[priority].push_back(next_cookie_id);
+        std::string cookie =
+            base::StringPrintf("a%d=b;priority=%s;%s", next_cookie_id,
+                               CookiePriorityToString(priority).c_str(),
+                               is_secure ? "secure" : "");
+        EXPECT_TRUE(SetCookie(cm, https_www_google_.url(), cookie));
+        cookie_data.push_back(std::make_pair(is_secure, priority));
+        id_list[is_secure][priority].push_back(next_cookie_id);
       }
     }
 
-    int num_cookies = static_cast<int>(priority_list.size());
-    std::vector<int> surviving_id_list[3];  // Indexed by CookiePriority.
+    int num_cookies = static_cast<int>(cookie_data.size());
+    // A list of cookie IDs, indexed by secure status, then by priority.
+    std::vector<std::vector<std::vector<int>>> surviving_id_list{{{}, {}, {}},
+                                                                 {{}, {}, {}}};
 
     // Parse the list of cookies
-    std::string cookie_str = this->GetCookies(cm, http_www_google_.url());
+    std::string cookie_str = this->GetCookies(cm, https_www_google_.url());
+    size_t num_nonsecure = 0;
+    size_t num_secure = 0;
     for (const std::string& token : base::SplitString(
              cookie_str, ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
       // Assuming *it is "a#=b", so extract and parse "#" portion.
@@ -410,22 +427,40 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
       DCHECK(result);
       DCHECK_GE(id, 0);
       DCHECK_LT(id, num_cookies);
-      surviving_id_list[priority_list[id]].push_back(id);
+      surviving_id_list[cookie_data[id].first][cookie_data[id].second]
+          .push_back(id);
+      if (cookie_data[id].first)
+        num_secure += 1;
+      else
+        num_nonsecure += 1;
     }
 
+    EXPECT_EQ(expected_nonsecure, num_nonsecure);
+    EXPECT_EQ(expected_secure, num_secure);
+
     // Validate each priority.
     size_t expected_count[3] = {
         expected_low_count, expected_medium_count, expected_high_count};
     for (int i = 0; i < 3; ++i) {
-      DCHECK_LE(surviving_id_list[i].size(), id_list[i].size());
-      EXPECT_EQ(expected_count[i], surviving_id_list[i].size());
+      size_t num_for_priority =
+          surviving_id_list[0][i].size() + surviving_id_list[1][i].size();
+      EXPECT_EQ(expected_count[i], num_for_priority);
       // Verify that the remaining cookies are the most recent among those
       // with the same priorities.
-      if (expected_count[i] == surviving_id_list[i].size()) {
-        std::sort(surviving_id_list[i].begin(), surviving_id_list[i].end());
-        EXPECT_TRUE(std::equal(surviving_id_list[i].begin(),
-                               surviving_id_list[i].end(),
-                               id_list[i].end() - expected_count[i]));
+      if (expected_count[i] == num_for_priority) {
+        // Non-secure:
+        std::sort(surviving_id_list[0][i].begin(),
+                  surviving_id_list[0][i].end());
+        EXPECT_TRUE(std::equal(
+            surviving_id_list[0][i].begin(), surviving_id_list[0][i].end(),
+            id_list[0][i].end() - surviving_id_list[0][i].size()));
+
+        // Secure:
+        std::sort(surviving_id_list[1][i].begin(),
+                  surviving_id_list[1][i].end());
+        EXPECT_TRUE(std::equal(
+            surviving_id_list[1][i].begin(), surviving_id_list[1][i].end(),
+            id_list[1][i].end() - surviving_id_list[1][i].size()));
       }
     }
   }
@@ -492,54 +527,152 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
     EXPECT_EQ(expected_non_secure_cookies, total_non_secure_cookies);
   }
 
-  void TestPriorityAwareGarbageCollectHelper() {
+  void TestPriorityAwareGarbageCollectHelperNonSecure() {
+    // Hard-coding limits in the test, but use DCHECK_EQ to enforce constraint.
+    DCHECK_EQ(180U, CookieMonster::kDomainMaxCookies);
+    DCHECK_EQ(150U, CookieMonster::kDomainMaxCookies -
+                        CookieMonster::kDomainPurgeCookies);
+
+    scoped_refptr<CookieMonster> cm(new CookieMonster(NULL, NULL));
+
+    // Each test case adds 181 cookies, so 31 cookies are evicted.
+    // Cookie same priority, repeated for each priority.
+    TestPriorityCookieCase(cm.get(), "181LN", 150U, 0U, 0U, 150U, 0U);
+    TestPriorityCookieCase(cm.get(), "181MN", 0U, 150U, 0U, 150U, 0U);
+    TestPriorityCookieCase(cm.get(), "181HN", 0U, 0U, 150U, 150U, 0U);
+
+    // Pairwise scenarios.
+    // Round 1 => none; round2 => 31M; round 3 => none.
+    TestPriorityCookieCase(cm.get(), "10HN 171MN", 0U, 140U, 10U, 150U, 0U);
+    // Round 1 => 10L; round2 => 21M; round 3 => none.
+    TestPriorityCookieCase(cm.get(), "141MN 40LN", 30U, 120U, 0U, 150U, 0U);
+    // Round 1 => none; round2 => none; round 3 => 31H.
+    TestPriorityCookieCase(cm.get(), "101HN 80MN", 0U, 80U, 70U, 150U, 0U);
+
+    // For {low, medium} priorities right on quota, different orders.
+    // Round 1 => 1L; round 2 => none, round3 => 30L.
+    TestPriorityCookieCase(cm.get(), "31LN 50MN 100HN", 0U, 50U, 100U, 150U,
+                           0U);
+    // Round 1 => none; round 2 => 1M, round3 => 30M.
+    TestPriorityCookieCase(cm.get(), "51MN 100HN 30LN", 30U, 20U, 100U, 150U,
+                           0U);
+    // Round 1 => none; round 2 => none; round3 => 31H.
+    TestPriorityCookieCase(cm.get(), "101HN 50MN 30LN", 30U, 50U, 70U, 150U,
+                           0U);
+
+    // Round 1 => 10L; round 2 => 10M; round3 => 11H.
+    TestPriorityCookieCase(cm.get(), "81HN 60MN 40LN", 30U, 50U, 70U, 150U, 0U);
+
+    // More complex scenarios.
+    // Round 1 => 10L; round 2 => 10M; round 3 => 11H.
+    TestPriorityCookieCase(cm.get(), "21HN 60MN 40LN 60HN", 30U, 50U, 70U, 150U,
+                           0U);
+    // Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
+    TestPriorityCookieCase(cm.get(), "11HN 10MN 20LN 110MN 20LN 10HN", 20U,
+                           109U, 21U, 150U, 0U);
+    // Round 1 => none; round 2 => none; round 3 => 11L, 10M, 10H.
+    TestPriorityCookieCase(cm.get(), "11LN 10MN 140HN 10MN 10LN", 10U, 10U,
+                           130U, 150U, 0U);
+    // Round 1 => none; round 2 => 1M; round 3 => 10L, 10M, 10H.
+    TestPriorityCookieCase(cm.get(), "11MN 10HN 10LN 60MN 90HN", 0U, 60U, 90U,
+                           150U, 0U);
+    // Round 1 => none; round 2 => 10L, 21M; round 3 => none.
+    TestPriorityCookieCase(cm.get(), "11MN 10HN 10LN 90MN 60HN", 0U, 80U, 70U,
+                           150U, 0U);
+  }
+
+  void TestPriorityAwareGarbageCollectHelperSecure() {
     // Hard-coding limits in the test, but use DCHECK_EQ to enforce constraint.
     DCHECK_EQ(180U, CookieMonster::kDomainMaxCookies);
     DCHECK_EQ(150U, CookieMonster::kDomainMaxCookies -
                         CookieMonster::kDomainPurgeCookies);
-    DCHECK_EQ(30U, CookieMonster::kDomainCookiesQuotaLow);
-    DCHECK_EQ(50U, CookieMonster::kDomainCookiesQuotaMedium);
-    DCHECK_EQ(70U, CookieMonster::kDomainCookiesQuotaHigh);
 
     scoped_refptr<CookieMonster> cm(new CookieMonster(NULL, NULL));
 
     // Each test case adds 181 cookies, so 31 cookies are evicted.
     // Cookie same priority, repeated for each priority.
-    TestPriorityCookieCase(cm.get(), "181L", 150U, 0U, 0U);
-    TestPriorityCookieCase(cm.get(), "181M", 0U, 150U, 0U);
-    TestPriorityCookieCase(cm.get(), "181H", 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "181LS", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "181MS", 0U, 150U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "181HS", 0U, 0U, 150U, 0U, 150U);
 
     // Pairwise scenarios.
     // Round 1 => none; round2 => 31M; round 3 => none.
-    TestPriorityCookieCase(cm.get(), "10H 171M", 0U, 140U, 10U);
+    TestPriorityCookieCase(cm.get(), "10HS 171MS", 0U, 140U, 10U, 0U, 150U);
     // Round 1 => 10L; round2 => 21M; round 3 => none.
-    TestPriorityCookieCase(cm.get(), "141M 40L", 30U, 120U, 0U);
+    TestPriorityCookieCase(cm.get(), "141MS 40LS", 30U, 120U, 0U, 0U, 150U);
     // Round 1 => none; round2 => none; round 3 => 31H.
-    TestPriorityCookieCase(cm.get(), "101H 80M", 0U, 80U, 70U);
+    TestPriorityCookieCase(cm.get(), "101HS 80MS", 0U, 80U, 70U, 0U, 150U);
 
     // For {low, medium} priorities right on quota, different orders.
     // Round 1 => 1L; round 2 => none, round3 => 30L.
-    TestPriorityCookieCase(cm.get(), "31L 50M 100H", 0U, 50U, 100U);
+    TestPriorityCookieCase(cm.get(), "31LS 50MS 100HS", 0U, 50U, 100U, 0U,
+                           150U);
     // Round 1 => none; round 2 => 1M, round3 => 30M.
-    TestPriorityCookieCase(cm.get(), "51M 100H 30L", 30U, 20U, 100U);
+    TestPriorityCookieCase(cm.get(), "51MS 100HS 30LS", 30U, 20U, 100U, 0U,
+                           150U);
     // Round 1 => none; round 2 => none; round3 => 31H.
-    TestPriorityCookieCase(cm.get(), "101H 50M 30L", 30U, 50U, 70U);
+    TestPriorityCookieCase(cm.get(), "101HS 50MS 30LS", 30U, 50U, 70U, 0U,
+                           150U);
 
     // Round 1 => 10L; round 2 => 10M; round3 => 11H.
-    TestPriorityCookieCase(cm.get(), "81H 60M 40L", 30U, 50U, 70U);
+    TestPriorityCookieCase(cm.get(), "81HS 60MS 40LS", 30U, 50U, 70U, 0U, 150U);
 
     // More complex scenarios.
     // Round 1 => 10L; round 2 => 10M; round 3 => 11H.
-    TestPriorityCookieCase(cm.get(), "21H 60M 40L 60H", 30U, 50U, 70U);
+    TestPriorityCookieCase(cm.get(), "21HS 60MS 40LS 60HS", 30U, 50U, 70U, 0U,
+                           150U);
     // Round 1 => 10L; round 2 => 11M, 10L; round 3 => none.
-    TestPriorityCookieCase(cm.get(), "11H 10M 20L 110M 20L 10H", 20U, 109U,
-                           21U);
+    TestPriorityCookieCase(cm.get(), "11HS 10MS 20LS 110MS 20LS 10HS", 20U,
+                           109U, 21U, 0U, 150U);
     // Round 1 => none; round 2 => none; round 3 => 11L, 10M, 10H.
-    TestPriorityCookieCase(cm.get(), "11L 10M 140H 10M 10L", 10U, 10U, 130U);
+    TestPriorityCookieCase(cm.get(), "11LS 10MS 140HS 10MS 10LS", 10U, 10U,
+                           130U, 0U, 150U);
     // Round 1 => none; round 2 => 1M; round 3 => 10L, 10M, 10H.
-    TestPriorityCookieCase(cm.get(), "11M 10H 10L 60M 90H", 0U, 60U, 90U);
+    TestPriorityCookieCase(cm.get(), "11MS 10HS 10LS 60MS 90HS", 0U, 60U, 90U,
+                           0U, 150U);
     // Round 1 => none; round 2 => 10L, 21M; round 3 => none.
-    TestPriorityCookieCase(cm.get(), "11M 10H 10L 90M 60H", 0U, 80U, 70U);
+    TestPriorityCookieCase(cm.get(), "11MS 10HS 10LS 90MS 60HS", 0U, 80U, 70U,
+                           0U, 150U);
+  }
+
+  void TestPriorityAwareGarbageCollectHelperMixed() {
+    // Hard-coding limits in the test, but use DCHECK_EQ to enforce constraint.
+    DCHECK_EQ(180U, CookieMonster::kDomainMaxCookies);
+    DCHECK_EQ(150U, CookieMonster::kDomainMaxCookies -
+                        CookieMonster::kDomainPurgeCookies);
+
+    scoped_refptr<CookieMonster> cm(new CookieMonster(NULL, NULL));
+
+    // Each test case adds 180 secure cookies, and some non-secure cookie. The

[jww, 2016/02/18 01:05:19]

Maybe add a few sanity check cases with > 180 cookies? Also, is it worth having
some mashed up test cases with the eviction test cases below in
EvictSecureCookies, to make sure that priorities are playing nice with some of
the other subtle edge cases, such as weird ordering of secure cookies and
non-secure cookies?

+    // secure cookies take priority, so the non-secure cookie is removed, along
+    // with 30 secure cookies. Repeated for each priority, and with the
+    // non-secure cookie as older and newer.
+    TestPriorityCookieCase(cm.get(), "1LN 180LS", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1MN 180MS", 0U, 150U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1HN 180HS", 0U, 0U, 150U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "180LS 1LN", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "180MS 1MN", 0U, 150U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "180HS 1HN", 0U, 0U, 150U, 0U, 150U);
+
+    // Higher-priority non-secure cookies are removed before any secure cookie.
+    TestPriorityCookieCase(cm.get(), "180LS 1MN", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "180LS 1HN", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "180MS 1HN", 0U, 150U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1MN 180LS", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1HN 180LS", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1HN 180MS", 0U, 150U, 0U, 0U, 150U);
+
+    // Pairwise:
+    TestPriorityCookieCase(cm.get(), "1LS 180LN", 150U, 0U, 0U, 149U, 1U);
+    TestPriorityCookieCase(cm.get(), "100LS 81LN", 150U, 0U, 0U, 50U, 100U);
+    TestPriorityCookieCase(cm.get(), "150LS 31LN", 150U, 0U, 0U, 0U, 150U);
+    TestPriorityCookieCase(cm.get(), "1LS 180HN", 1U, 0U, 149U, 149U, 1U);
+    TestPriorityCookieCase(cm.get(), "100LS 81HN", 100U, 0U, 50U, 50U, 100U);
+    TestPriorityCookieCase(cm.get(), "150LS 31HN", 150U, 0U, 0U, 0U, 150U);
+
+    // Quota calculations inside non-secure/secure blocks remain in place:
+    TestPriorityCookieCase(cm.get(), "50HN 50LS 81HS", 50U, 0U, 100U, 19U,
+                           131U);
   }
 
   // Function for creating a CM with a number of cookies in it,
@@ -1248,8 +1381,16 @@ TEST_F(CookieMonsterTest, TestHostGarbageCollection) {
   TestHostGarbageCollectHelper();
 }
 
-TEST_F(CookieMonsterTest, TestPriorityAwareGarbageCollection) {
-  TestPriorityAwareGarbageCollectHelper();
+TEST_F(CookieMonsterTest, TestPriorityAwareGarbageCollectionNonSecure) {
+  TestPriorityAwareGarbageCollectHelperNonSecure();
+}
+
+TEST_F(CookieMonsterTest, TestPriorityAwareGarbageCollectionSecure) {
+  TestPriorityAwareGarbageCollectHelperSecure();
+}
+
+TEST_F(CookieMonsterTest, TestPriorityAwareGarbageCollectionMixed) {
+  TestPriorityAwareGarbageCollectHelperMixed();
 }
 
 TEST_F(CookieMonsterTest, SetCookieableSchemes) {