Expire cookies by last access date, rather than creation date.

BUG=2906
Review URL: http://codereview.chromium.org/8753

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4354 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 106 insertions, 21 deletions

diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index 7a56831..987bfb8 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -68,6 +68,10 @@ using base::TimeDelta;
 
 namespace net {
 
+// Default minimum delay after updating a cookie's LastAccessDate before we
+// will update it again.
+static const int kDefaultAccessUpdateThresholdSeconds = 60;
+
 // static
 bool CookieMonster::enable_file_scheme_ = false;
 
@@ -78,12 +82,16 @@ void CookieMonster::EnableFileScheme() {
 
 CookieMonster::CookieMonster()
     : initialized_(false),
-      store_(NULL) {
+      store_(NULL),
+      last_access_threshold_(
+          TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds)) {
 }
 
 CookieMonster::CookieMonster(PersistentCookieStore* store)
     : initialized_(false),
-      store_(store) {
+      store_(store),
+      last_access_threshold_(
+          TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds)) {
 }
 
 CookieMonster::~CookieMonster() {
@@ -398,8 +406,8 @@ bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
 
   cc.reset(new CanonicalCookie(pc.Name(), pc.Value(), cookie_path,
                                pc.IsSecure(), pc.IsHttpOnly(),
-                               creation_time, !cookie_expires.is_null(),
-                               cookie_expires));
+                               creation_time, creation_time,
+                               !cookie_expires.is_null(), cookie_expires));
 
   if (!cc.get()) {
     COOKIE_DLOG(WARNING) << "Failed to allocate CanonicalCookie";
@@ -440,6 +448,20 @@ void CookieMonster::InternalInsertCookie(const std::string& key,
   cookies_.insert(CookieMap::value_type(key, cc));
 }
 
+void CookieMonster::InternalUpdateCookieAccessTime(CanonicalCookie* cc) {
+  // Based off the Mozilla code.  When a cookie has been accessed recently,
+  // don't bother updating its access time again.  This reduces the number of
+  // updates we do during pageload, which in turn reduces the chance our storage
+  // backend will hit its batch thresholds and be forced to update.
+  const Time current = Time::Now();
+  if ((current - cc->LastAccessDate()) < last_access_threshold_)
+    return;
+
+  cc->SetLastAccessDate(current);
+  if (cc->IsPersistent() && store_)
+    store_->UpdateCookieAccessTime(*cc);
+}
+
 void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
                                          bool sync_to_store) {
   CanonicalCookie* cc = it->second;
@@ -507,10 +529,15 @@ int CookieMonster::GarbageCollect(const Time& current,
   return num_deleted;
 }
 
-// TODO we should be sorting by last access time, however, right now
-// we're not saving an access time, so we're sorting by creation time.
-static bool OldestCookieSorter(const CookieMonster::CookieMap::iterator& it1,
-                               const CookieMonster::CookieMap::iterator& it2) {
+static bool LRUCookieSorter(const CookieMonster::CookieMap::iterator& it1,
+                            const CookieMonster::CookieMap::iterator& it2) {
+  // Cookies accessed less recently should be deleted first.
+  if (it1->second->LastAccessDate() != it2->second->LastAccessDate())
+    return it1->second->LastAccessDate() < it2->second->LastAccessDate();
+
+  // In rare cases we might have two cookies with identical last access times.
+  // To preserve the stability of the sort, in these cases prefer to delete
+  // older cookies over newer ones.  CreationDate() is guaranteed to be unique.
   return it1->second->CreationDate() < it2->second->CreationDate();
 }
 
@@ -522,7 +549,7 @@ int CookieMonster::GarbageCollectRange(const Time& current,
   std::vector<CookieMap::iterator> cookie_its;
   int num_deleted = GarbageCollectExpired(current, itpair, &cookie_its);
 
-  // If the range still has too many cookies, delete the oldest.
+  // If the range still has too many cookies, delete the least recently used.
   if (cookie_its.size() > num_max) {
     COOKIE_DLOG(INFO) << "GarbageCollectRange() Deep Garbage Collect.";
     // Purge down to (|num_max| - |num_purge|) total cookies.
@@ -530,7 +557,7 @@ int CookieMonster::GarbageCollectRange(const Time& current,
     num_purge += cookie_its.size() - num_max;
 
     std::partial_sort(cookie_its.begin(), cookie_its.begin() + num_purge,
-                      cookie_its.end(), OldestCookieSorter);
+                      cookie_its.end(), LRUCookieSorter);
     for (size_t i = 0; i < num_purge; ++i)
       InternalDeleteCookie(cookie_its[i], true);
 
@@ -763,7 +790,9 @@ void CookieMonster::FindCookiesForKey(
     if (!cc->IsOnPath(url.path()))
       continue;
 
-    // Congratulations Charlie, you passed the test!
+    // Add this cookie to the set of matching cookies.  Since we're reading the
+    // cookie, update its last access time.
+    InternalUpdateCookieAccessTime(cc);
     cookies->push_back(cc);
   }
 }
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index 6db774a..68d8c3b 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -30,9 +30,6 @@ namespace net {
 //
 // TODO(deanm) Implement CookieMonster, the cookie database.
 //  - Verify that our domain enforcement and non-dotted handling is correct
-//  - Currently garbage collection is done on oldest CreationUTC, Mozilla
-//    purges cookies on last access time, which would require adding and
-//    keeping track of access times on a CanonicalCookie
 class CookieMonster {
  public:
   class ParsedCookie;
@@ -68,6 +65,15 @@ class CookieMonster {
   // existence.
   CookieMonster(PersistentCookieStore* store);
 
+#ifdef UNIT_TEST
+  CookieMonster::CookieMonster(int last_access_threshold_seconds)
+      : initialized_(false),
+        store_(NULL),
+        last_access_threshold_(
+            base::TimeDelta::FromSeconds(last_access_threshold_seconds)) {
+  }
+#endif
+
   ~CookieMonster();
 
   // Parse the string with the cookie time (very forgivingly).
@@ -90,7 +96,8 @@ class CookieMonster {
   // It will _not_ return httponly cookies, see GetCookiesWithOptions
   std::string GetCookies(const GURL& url);
   std::string GetCookiesWithOptions(const GURL& url, CookieOptions options);
-  // Returns all the cookies, for use in management UI, etc.
+  // Returns all the cookies, for use in management UI, etc.  This does not mark
+  // the cookies as having been accessed.
   CookieList GetAllCookies();
 
   // Delete all of the cookies.
@@ -149,6 +156,8 @@ class CookieMonster {
                             CanonicalCookie* cc,
                             bool sync_to_store);
 
+  void InternalUpdateCookieAccessTime(CanonicalCookie* cc);
+
   void InternalDeleteCookie(CookieMap::iterator it, bool sync_to_store);
 
   // If the number of cookies for host |key|, or globally, are over preset
@@ -161,7 +170,8 @@ class CookieMonster {
 
   // Deletes all expired cookies in |itpair|;
   // then, if the number of remaining cookies is greater than |num_max|,
-  // collects the oldest cookies until (|num_max| - |num_purge|) cookies remain.
+  // collects the least recently accessed cookies until
+  // (|num_max| - |num_purge|) cookies remain.
   //
   // Returns the number of cookies deleted.
   int GarbageCollectRange(const base::Time& current,
@@ -191,6 +201,10 @@ class CookieMonster {
   base::Time CurrentTime();
   base::Time last_time_seen_;
 
+  // Minimum delay after updating a cookie's LastAccessDate before we will
+  // update it again.
+  const base::TimeDelta last_access_threshold_;
+
   // Lock for thread-safety
   Lock lock_;
 
@@ -261,12 +275,14 @@ class CookieMonster::CanonicalCookie {
                   bool secure,
                   bool httponly,
                   const base::Time& creation,
+                  const base::Time& last_access,
                   bool has_expires,
                   const base::Time& expires)
       : name_(name),
         value_(value),
         path_(path),
         creation_date_(creation),
+        last_access_date_(last_access),
         expiry_date_(expires),
         has_expires_(has_expires),
         secure_(secure),
@@ -288,6 +304,7 @@ class CookieMonster::CanonicalCookie {
   const std::string& Value() const { return value_; }
   const std::string& Path() const { return path_; }
   const base::Time& CreationDate() const { return creation_date_; }
+  const base::Time& LastAccessDate() const { return last_access_date_; }
   bool DoesExpire() const { return has_expires_; }
   bool IsPersistent() const { return DoesExpire(); }
   const base::Time& ExpiryDate() const { return expiry_date_; }
@@ -306,6 +323,10 @@ class CookieMonster::CanonicalCookie {
     return name_ == ecc.Name() && path_ == ecc.Path();
   }
 
+  void SetLastAccessDate(const base::Time& date) {
+    last_access_date_ = date;
+  }
+
   bool IsOnPath(const std::string& url_path) const;
 
   std::string DebugString() const;
@@ -314,6 +335,7 @@ class CookieMonster::CanonicalCookie {
   std::string value_;
   std::string path_;
   base::Time creation_date_;
+  base::Time last_access_date_;
   base::Time expiry_date_;
   bool has_expires_;
   bool secure_;
@@ -329,6 +351,7 @@ class CookieMonster::PersistentCookieStore {
   virtual bool Load(std::vector<CookieMonster::KeyedCanonicalCookie>*) = 0;
 
   virtual void AddCookie(const std::string&, const CanonicalCookie&) = 0;
+  virtual void UpdateCookieAccessTime(const CanonicalCookie&) = 0;
   virtual void DeleteCookie(const CanonicalCookie&) = 0;
 
  protected:
diff --git a/net/base/cookie_monster_unittest.cc b/net/base/cookie_monster_unittest.cc
index e9a5b00..7b6f0ad 100644
--- a/net/base/cookie_monster_unittest.cc
+++ b/net/base/cookie_monster_unittest.cc
@@ -718,6 +718,31 @@ TEST(CookieMonsterTest, TestSecure) {
   EXPECT_EQ("D=E; A=B", cm.GetCookies(url_google_secure));
 }
 
+static Time GetFirstCookieAccessDate(net::CookieMonster* cm) {
+  const net::CookieMonster::CookieList all_cookies(cm->GetAllCookies());
+  return all_cookies.front().second.LastAccessDate();
+}
+
+static const int kLastAccessThresholdSeconds = 1;
+
+TEST(CookieMonsterTest, TestLastAccess) {
+  GURL url_google(kUrlGoogle);
+  net::CookieMonster cm(kLastAccessThresholdSeconds);
+
+  EXPECT_TRUE(cm.SetCookie(url_google, "A=B"));
+  const Time last_access_date(GetFirstCookieAccessDate(&cm));
+
+  // Reading the cookie again immediately shouldn't update the access date,
+  // since we're inside the threshold.
+  EXPECT_EQ("A=B", cm.GetCookies(url_google));
+  EXPECT_TRUE(last_access_date == GetFirstCookieAccessDate(&cm));
+
+  // Reading after a short wait should update the access date.
+  Sleep(1500);
+  EXPECT_EQ("A=B", cm.GetCookies(url_google));
+  EXPECT_FALSE(last_access_date == GetFirstCookieAccessDate(&cm));
+}
+
 static int CountInString(const std::string& str, char c) {
   int count = 0;
   for (std::string::const_iterator it = str.begin();
@@ -744,23 +769,31 @@ TEST(CookieMonsterTest, TestHostGarbageCollection) {
 }
 
 TEST(CookieMonsterTest, TestTotalGarbageCollection) {
-  net::CookieMonster cm;
+  net::CookieMonster cm(kLastAccessThresholdSeconds);
   // Add a bunch of cookies on a bunch of host, some should get purged.
+  const GURL sticky_cookie("http://a0000.izzle");
   for (int i = 0; i < 2000; ++i) {
     GURL url(StringPrintf("http://a%04d.izzle", i));
     EXPECT_TRUE(cm.SetCookie(url, "a=b"));
     EXPECT_EQ("a=b", cm.GetCookies(url));
+
+    // Keep touching the first cookie to ensure it's not purged (since it will
+    // always have the most recent access time).
+    if (!(i % 500)) {
+      Sleep(1500);  // Ensure the timestamps will be different enough to update.
+      EXPECT_EQ("a=b", cm.GetCookies(sticky_cookie));
+    }
   }
 
   // Check that cookies that still exist.
   for (int i = 0; i < 2000; ++i) {
     GURL url(StringPrintf("http://a%04d.izzle", i));
-    if (i < 900) {
-      // Cookies should have gotten purged.
-      EXPECT_TRUE(cm.GetCookies(url).empty());
-    } else if (i > 1100) {
+    if ((i == 0) || (i > 1101)) {
       // Cookies should still be around.
       EXPECT_FALSE(cm.GetCookies(url).empty());
+    } else if (i < 901) {
+      // Cookies should have gotten purged.
+      EXPECT_TRUE(cm.GetCookies(url).empty());
     }
   }
 }