Bug 1319403 - Modified nsCookieService::FindStaleCookie() and test cases, r=jdm
authorAmy Chung <amchung@mozilla.com>
Mon, 27 Feb 2017 18:31:33 +0800
changeset 374076 b780bfdac971fcba73575f454a45d042cf2ed14e
parent 374075 dd7207e80b803095d868985743134498d512f776
child 374077 e6332dc0a1837295e549629bf5f2ab1a50cba00b
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
bugs1319403
milestone54.0a1
Bug 1319403 - Modified nsCookieService::FindStaleCookie() and test cases, r=jdm
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/test/unit/test_eviction.js
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -4619,27 +4619,19 @@ nsCookieService::FindStaleCookie(nsCooki
   if (aSource) {
     GetBaseDomain(aSource, baseDomain, requireHostMatch);
     aSource->GetAsciiHost(sourceHost);
     sourcePath = GetPathFromURI(aSource);
   }
 
   const nsCookieEntry::ArrayType &cookies = aEntry->GetCookies();
 
-  int64_t oldestNonMatchingSessionCookieTime = 0;
-  nsListIter oldestNonMatchingSessionCookie;
-  oldestNonMatchingSessionCookie.entry = nullptr;
-
-  int64_t oldestSessionCookieTime = 0;
-  nsListIter oldestSessionCookie;
-  oldestSessionCookie.entry = nullptr;
-
-  int64_t oldestNonMatchingNonSessionCookieTime = 0;
-  nsListIter oldestNonMatchingNonSessionCookie;
-  oldestNonMatchingNonSessionCookie.entry = nullptr;
+  int64_t oldestNonMatchingCookieTime = 0;
+  nsListIter oldestNonMatchingCookie;
+  oldestNonMatchingCookie.entry = nullptr;
 
   int64_t oldestCookieTime = 0;
   nsListIter oldestCookie;
   oldestCookie.entry = nullptr;
 
   int64_t actualOldestCookieTime = cookies.Length() ? cookies[0]->LastAccessed() : 0;
   for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
     nsCookie *cookie = cookies[i];
@@ -4662,69 +4654,44 @@ nsCookieService::FindStaleCookie(nsCooki
     if (aIsSecure.isSome() && !aIsSecure.value()) {
       // We want to look for the oldest non-secure cookie first time through,
       // then find the oldest secure cookie the second time we are called.
       if (cookie->IsSecure()) {
         continue;
       }
     }
 
-    // Update our various records of oldest cookies fitting several restrictions:
-    // * session cookies
-    // * non-session cookies
-    // * cookies with paths and domains that don't match the cookie triggering this purge
-
     // This cookie is a candidate for eviction if we have no information about
     // the source request, or if it is not a path or domain match against the
     // source request.
     bool isPrimaryEvictionCandidate = true;
     if (aSource) {
       isPrimaryEvictionCandidate = !PathMatches(cookie, sourcePath) || !DomainMatches(cookie, sourceHost);
     }
 
-    if (cookie->IsSession()) {
-      if (!oldestSessionCookie.entry || oldestSessionCookieTime > lastAccessed) {
-        oldestSessionCookieTime = lastAccessed;
-        oldestSessionCookie.entry = aEntry;
-        oldestSessionCookie.index = i;
-      }
-
-      if (isPrimaryEvictionCandidate &&
-          (!oldestNonMatchingSessionCookie.entry ||
-           oldestNonMatchingSessionCookieTime > lastAccessed)) {
-        oldestNonMatchingSessionCookieTime = lastAccessed;
-        oldestNonMatchingSessionCookie.entry = aEntry;
-        oldestNonMatchingSessionCookie.index = i;
-      }
-    } else if (isPrimaryEvictionCandidate &&
-               (!oldestNonMatchingNonSessionCookie.entry ||
-                oldestNonMatchingNonSessionCookieTime > lastAccessed)) {
-      oldestNonMatchingNonSessionCookieTime = lastAccessed;
-      oldestNonMatchingNonSessionCookie.entry = aEntry;
-      oldestNonMatchingNonSessionCookie.index = i;
+    if (isPrimaryEvictionCandidate &&
+        (!oldestNonMatchingCookie.entry ||
+         oldestNonMatchingCookieTime > lastAccessed)) {
+      oldestNonMatchingCookieTime = lastAccessed;
+      oldestNonMatchingCookie.entry = aEntry;
+      oldestNonMatchingCookie.index = i;
     }
 
     // Check if we've found the oldest cookie so far.
     if (!oldestCookie.entry || oldestCookieTime > lastAccessed) {
       oldestCookieTime = lastAccessed;
       oldestCookie.entry = aEntry;
       oldestCookie.index = i;
     }
   }
 
-  // Prefer to evict the oldest session cookies with a non-matching path/domain,
-  // followed by the oldest session cookie with a matching path/domain,
-  // followed by the oldest non-session cookie with a non-matching path/domain,
-  // resorting to the oldest non-session cookie with a matching path/domain.
-  if (oldestNonMatchingSessionCookie.entry) {
-    aIter = oldestNonMatchingSessionCookie;
-  } else if (oldestSessionCookie.entry) {
-    aIter = oldestSessionCookie;
-  } else if (oldestNonMatchingNonSessionCookie.entry) {
-    aIter = oldestNonMatchingNonSessionCookie;
+  // Prefer to evict the oldest cookie with a non-matching path/domain,
+  // followed by the oldest matching cookie.
+  if (oldestNonMatchingCookie.entry) {
+    aIter = oldestNonMatchingCookie;
   } else {
     aIter = oldestCookie;
   }
 
   return actualOldestCookieTime;
 }
 
 void
--- a/netwerk/cookie/test/unit/test_eviction.js
+++ b/netwerk/cookie/test/unit/test_eviction.js
@@ -147,85 +147,82 @@ function* test_basic_eviction(base_host,
     // Ensure that cookies set for the / path appear more recent.
     cs.getCookieString(OTHER_SUBDOMAIN_URI, null)
     verifyCookies(['non_session_non_path_non_domain',
                    'session_foo_path',
                    'session_bar_path',
                    'non_session_non_path_subdomain',
                    'session_non_path_pub_domain'], BASE_URI);
 
-    // Evict oldest session cookie that does not match example.org/foo (session_bar_path)
+    // Evict oldest cookie that does not match example.org/foo (session_bar_path)
     yield setCookie("session_foo_path_2", null, "/foo", null, FOO_PATH);
     verifyCookies(['non_session_non_path_non_domain',
                    'session_foo_path',
                    'non_session_non_path_subdomain',
                    'session_non_path_pub_domain',
                    'session_foo_path_2'], BASE_URI);
 
-    // Evict oldest session cookie that does not match example.org/bar (session_foo_path)
+    // Evict oldest cookie that does not match example.org/bar (session_foo_path)
     yield setCookie("session_bar_path_2", null, "/bar", null, BAR_PATH);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'session_non_path_pub_domain',
                    'session_foo_path_2',
                    'session_bar_path_2'], BASE_URI);
 
-    // Evict oldest session cookie that does not match example.org/ (session_non_path_pub_domain)
+    // Evict oldest cookie that does not match example.org/ (session_non_path_pub_domain)
     yield setCookie("non_session_non_path_non_domain_2", null, null, 100000, BASE_URI);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'session_foo_path_2',
                    'session_bar_path_2',
                    'non_session_non_path_non_domain_2'], BASE_URI);
 
-    // Evict oldest session cookie that does not match example.org/ (session_foo_path_2)
+    // Evict oldest cookie that does not match example.org/ (session_foo_path_2)
     yield setCookie("session_non_path_non_domain_3", null, null, null, BASE_URI);
     verifyCookies(['non_session_non_path_non_domain',
                    'non_session_non_path_subdomain',
                    'session_bar_path_2',
                    'non_session_non_path_non_domain_2',
                    'session_non_path_non_domain_3'], BASE_URI);
 
-    // Evict oldest session cookie; all such cookies match example.org/bar (session_bar_path_2)
-    // note: this new cookie doesn't have an explicit path, but empty paths inherit the
-    // request's path
+    // Evict oldest cookie; all such cookies match example.org/bar (non_session_non_path_non_domain)
     yield setCookie("non_session_bar_path_non_domain", null, null, 100000, BAR_PATH);
-    verifyCookies(['non_session_non_path_non_domain',
-                   'non_session_non_path_subdomain',
+    verifyCookies(['non_session_non_path_subdomain',
+                   'session_bar_path_2',
                    'non_session_non_path_non_domain_2',
                    'session_non_path_non_domain_3',
                    'non_session_bar_path_non_domain'], BASE_URI);
 
-    // Evict oldest session cookie, even though it matches pub.example.org (session_non_path_non_domain_3)
+    // Evict oldest cookie that deose not match example.org/ (session_bar_path_2)
     yield setCookie("non_session_non_path_pub_domain", null, null, 100000, OTHER_SUBDOMAIN_URI);
-    verifyCookies(['non_session_non_path_non_domain',
-                   'non_session_non_path_subdomain',
+    verifyCookies(['non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
+                   'session_non_path_non_domain_3',
                    'non_session_bar_path_non_domain',
                    'non_session_non_path_pub_domain'], BASE_URI);
 
-    // All session cookies have been evicted.
-    // Evict oldest non-session non-domain-matching cookie (non_session_non_path_pub_domain)
+    // Evict oldest cookie that does not match example.org/bar (non_session_non_path_pub_domain)
     yield setCookie("non_session_bar_path_non_domain_2", null, '/bar', 100000, BAR_PATH);
-    verifyCookies(['non_session_non_path_non_domain',
-                   'non_session_non_path_subdomain',
+    verifyCookies(['non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
+                   'session_non_path_non_domain_3',
                    'non_session_bar_path_non_domain',
                    'non_session_bar_path_non_domain_2'], BASE_URI);
 
-    // Evict oldest non-session non-path-matching cookie (non_session_bar_path_non_domain)
+    // Evict oldest cookie that does not match example.org/ (non_session_bar_path_non_domain)
     yield setCookie("non_session_non_path_non_domain_4", null, null, 100000, BASE_URI);
-    verifyCookies(['non_session_non_path_non_domain',
-                   'non_session_non_path_subdomain',
+    verifyCookies(['non_session_non_path_subdomain',
                    'non_session_non_path_non_domain_2',
+                   'session_non_path_non_domain_3',
                    'non_session_bar_path_non_domain_2',
                    'non_session_non_path_non_domain_4'], BASE_URI);
 
-    // At this point all remaining cookies are non-session cookies, have a path of /,
-    // and either don't have a domain or have one that matches subdomains.
+    // At this point all remaining cookies have a path of / and either don't have a domain
+    // or have one that matches subdomains.
     // They will therefore be evicted from oldest to newest if all new cookies added share
     // similar characteristics.
 }
 
 // Verify that the given cookie names exist, and are ordered from least to most recently accessed
 function verifyCookies(names, uri) {
     do_check_eq(cm.countCookiesFromHost(uri.host), names.length);
     let cookies = cm.getCookiesFromHost(uri.host, {});