Bug 1155169 - Avoid restoring the same cookie twice if the previous version of the cookie is not stale; r=jdm

netwerk/cookie/nsCookie.cpp

@@ -8,6 +8,8 @@
 #include <stdlib.h>
 #include "nsAutoPtr.h"
 
+static const int64_t kCookieStaleThreshold = 60 * PR_USEC_PER_SEC; // 1 minute in microseconds
+
 /******************************************************************************
  * nsCookie:
  * string helper impl
@@ -120,6 +122,14 @@ nsCookie::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
     return aMallocSizeOf(this);
 }
 
+bool
+nsCookie::IsStale() const
+{
+  int64_t currentTimeInUsec = PR_Now();
+
+  return currentTimeInUsec - LastAccessed() > kCookieStaleThreshold;
+}
+
 /******************************************************************************
  * nsCookie:
  * xpcom impl

netwerk/cookie/nsCookie.h

@@ -100,6 +100,8 @@ class nsCookie : public nsICookie2
     // Create(). Use with caution!
     inline void SetCreationTime(int64_t aTime)    { mCreationTime = aTime; }
 
+    bool IsStale() const;
+
   protected:
     virtual ~nsCookie() {}
 

netwerk/cookie/nsCookieService.cpp

@@ -91,7 +91,6 @@ static nsCookieService *gCookieService;
 #define IDX_APP_ID 10
 #define IDX_BROWSER_ELEM 11
 
-static const int64_t kCookieStaleThreshold = 60 * PR_USEC_PER_SEC; // 1 minute in microseconds
 static const int64_t kCookiePurgeAge =
   int64_t(30 * 24 * 60 * 60) * PR_USEC_PER_SEC; // 30 days in microseconds
 
@@ -2758,8 +2757,9 @@ nsCookieService::GetCookieStringInternal(nsIURI *aHostURI,
 
     // all checks passed - add to list and check if lastAccessed stamp needs updating
     foundCookieList.AppendElement(cookie);
-    if (currentTimeInUsec - cookie->LastAccessed() > kCookieStaleThreshold)
+    if (cookie->IsStale()) {
       stale = true;
+    }
   }
 
   int32_t count = foundCookieList.Length();
@@ -2780,8 +2780,9 @@ nsCookieService::GetCookieStringInternal(nsIURI *aHostURI,
     for (int32_t i = 0; i < count; ++i) {
       cookie = foundCookieList.ElementAt(i);
 
-      if (currentTimeInUsec - cookie->LastAccessed() > kCookieStaleThreshold)
+      if (cookie->IsStale()) {
         UpdateCookieInList(cookie, currentTimeInUsec, paramsArray);
+      }
     }
     // Update the database now if necessary.
     if (paramsArray) {
@@ -2995,6 +2996,24 @@ nsCookieService::AddInternal(const nsCookieKey             &aKey,
         return;
       }
 
+      // If the new cookie has the same value, expiry date, and isSecure,
+      // isSession, and isHttpOnly flags then we can just keep the old one.
+      // Only if any of these differ we would want to override the cookie.
+      if (oldCookie->Value().Equals(aCookie->Value()) &&
+          oldCookie->Expiry() == aCookie->Expiry() &&
+          oldCookie->IsSecure() == aCookie->IsSecure() &&
+          oldCookie->IsSession() == aCookie->IsSession() &&
+          oldCookie->IsHttpOnly() == aCookie->IsHttpOnly() &&
+          // We don't want to perform this optimization if the cookie is
+          // considered stale, since in this case we would need to update the
+          // database.
+          !oldCookie->IsStale()) {
+        // Update the last access time on the old cookie.
+        oldCookie->SetLastAccessed(aCookie->LastAccessed());
+        UpdateCookieOldestTime(mDBState, oldCookie);
+        return;
+      }
+
       // Remove the old cookie.
       RemoveCookieFromList(matchIter);
 
@@ -4234,6 +4253,15 @@ bindCookieParameters(mozIStorageBindingParamsArray *aParamsArray,
   NS_ASSERT_SUCCESS(rv);
 }
 
+void
+nsCookieService::UpdateCookieOldestTime(DBState* aDBState,
+                                        nsCookie* aCookie)
+{
+  if (aCookie->LastAccessed() < aDBState->cookieOldestTime) {
+    aDBState->cookieOldestTime = aCookie->LastAccessed();
+  }
+}
+
 void
 nsCookieService::AddCookieToList(const nsCookieKey             &aKey,
                                  nsCookie                      *aCookie,
@@ -4253,8 +4281,7 @@ nsCookieService::AddCookieToList(const nsCookieKey             &aKey,
   ++aDBState->cookieCount;
 
   // keep track of the oldest cookie, for when it comes time to purge
-  if (aCookie->LastAccessed() < aDBState->cookieOldestTime)
-    aDBState->cookieOldestTime = aCookie->LastAccessed();
+  UpdateCookieOldestTime(aDBState, aCookie);
 
   // if it's a non-session cookie and hasn't just been read from the db, write it out.
   if (aWriteToDB && !aCookie->IsSession() && aDBState->dbConn) {

netwerk/cookie/nsCookieService.h

@@ -314,6 +314,7 @@ class nsCookieService final : public nsICookieService
     void                          NotifyChanged(nsISupports *aSubject, const char16_t *aData);
     void                          NotifyPurged(nsICookie2* aCookie);
     already_AddRefed<nsIArray>    CreatePurgeList(nsICookie2* aCookie);
+    void                          UpdateCookieOldestTime(DBState* aDBState, nsCookie* aCookie);
 
     /**
      * This method is used to iterate the cookie hash table and select the ones

netwerk/cookie/test/unit/test_bug1155169.js

@@ -0,0 +1,73 @@
+const {utils: Cu, interfaces: Ci, classes: Cc} = Components;
+
+Cu.import("resource://gre/modules/Services.jsm");
+
+const URI = Services.io.newURI("http://example.org/", null, null);
+
+const cs = Cc["@mozilla.org/cookieService;1"]
+             .getService(Ci.nsICookieService);
+
+function run_test() {
+  // Allow all cookies.
+  Services.prefs.setIntPref("network.cookie.cookieBehavior", 0);
+
+  // Clear cookies.
+  Services.cookies.removeAll();
+
+  // Add a new cookie.
+  setCookie("foo=bar", {
+    type: "added", isSession: true, isSecure: false, isHttpOnly: false
+  });
+
+  // Update cookie with isHttpOnly=true.
+  setCookie("foo=bar; HttpOnly", {
+    type: "changed", isSession: true, isSecure: false, isHttpOnly: true
+  });
+
+  // Update cookie with isSecure=true.
+  setCookie("foo=bar; Secure", {
+    type: "changed", isSession: true, isSecure: true, isHttpOnly: false
+  });
+
+  // Update cookie with isSession=false.
+  let expiry = new Date();
+  expiry.setUTCFullYear(expiry.getUTCFullYear() + 2);
+  setCookie(`foo=bar; Expires=${expiry.toGMTString()}`, {
+    type: "changed", isSession: false, isSecure: false, isHttpOnly: false
+  });
+
+  // Reset cookie.
+  setCookie("foo=bar", {
+    type: "changed", isSession: true, isSecure: false, isHttpOnly: false
+  });
+}
+
+function setCookie(value, expected) {
+  function setCookieInternal(value, expected = null) {
+    function observer(subject, topic, data) {
+      if (!expected) {
+        do_throw("no notification expected");
+        return;
+      }
+
+      // Check we saw the right notification.
+      do_check_eq(data, expected.type);
+
+      // Check cookie details.
+      let cookie = subject.QueryInterface(Ci.nsICookie2);
+      do_check_eq(cookie.isSession, expected.isSession);
+      do_check_eq(cookie.isSecure, expected.isSecure);
+      do_check_eq(cookie.isHttpOnly, expected.isHttpOnly);
+    }
+
+    Services.obs.addObserver(observer, "cookie-changed", false);
+    cs.setCookieStringFromHttp(URI, null, null, value, null, null);
+    Services.obs.removeObserver(observer, "cookie-changed");
+  }
+
+  // Check that updating/inserting the cookie works.
+  setCookieInternal(value, expected);
+
+  // Check that we ignore identical cookies.
+  setCookieInternal(value);
+}

netwerk/cookie/test/unit/xpcshell.ini

@@ -4,5 +4,6 @@ tail =
 skip-if = toolkit == 'gonk'
 
 [test_bug643051.js]
+[test_bug1155169.js]
 [test_parser_0001.js]
 [test_parser_0019.js]