Fixes targeting the unique creation times invariant in the CookieMonster:

net/base/cookie_monster.cc

Index: net/base/cookie_monster.cc
diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index 4c460148195cd8ab993ba27e0b8f9bb95195785d..795d3ea8f65dc3fb3672a5d2a7a9ffa177fa2b23 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -185,13 +185,29 @@ void CookieMonster::InitStore() {
   // This prevents multiple vector growth / copies as we append cookies.
   cookies.reserve(kNumCookiesTotal);
   store_->Load(&cookies);
+
+  // Avoid ever letting cookies with duplicate creation times into the store;
+  // that way we don't have to worry about what sections of code are safe
+  // to call while it's in that state.
+  std::set<int64> creation_times;
   for (std::vector<KeyedCanonicalCookie>::const_iterator it = cookies.begin();
        it != cookies.end(); ++it) {
-    InternalInsertCookie(it->first, it->second, false);
+    int64 cookie_creation_time = it->second->CreationDate().ToInternalValue();
+
+    if (creation_times.insert(cookie_creation_time).second) {
+      InternalInsertCookie(it->first, it->second, false);
+    } else {
+      LOG(ERROR) << StringPrintf("Found cookies with duplicate creation "
+                                 "times in backing store: "
+                                 "{name='%s', domain='%s', path='%s'}",
+                                 it->second->Name().c_str(),
+                                 it->first.c_str(),
+                                 it->second->Path().c_str());
+    }
   }
 
   // After importing cookies from the PersistentCookieStore, verify that
-  // none of our constraints are violated.
+  // none of our other constraints are violated.
   //
   // In particular, the backing store might have given us duplicate cookies.
   EnsureCookiesMapIsValid();
@@ -218,9 +234,6 @@ void CookieMonster::EnsureCookiesMapIsValid() {
   // Record how many duplicates were found in the database.
   // See InitializeHistograms() for details.
   histogram_cookie_deletion_cause_->Add(num_duplicates_trimmed);
-
-  // TODO(eroman): Should also verify that there are no cookies with the same
-  // creation time, since that is assumed to be unique by the rest of the code.
 }
 
 // Our strategy to find duplicates is:
@@ -294,13 +307,18 @@ int CookieMonster::TrimDuplicateCookiesForHost(
 
     // We save the iterator into |cookies_| rather than the actual cookie
     // pointer, since we may need to delete it later.
-    list.insert(it);
+    bool insert_success = list.insert(it).second;
+    DCHECK(insert_success) <<
+        "Duplicate creation times found in duplicate cookie name scan.";
   }
 
   // If there were no duplicates, we are done!
   if (num_duplicates == 0)
     return 0;
 
+  // Make sure we find everything below that we did above.
+  int num_duplicates_found = 0;
+
   // Otherwise, delete all the duplicate cookies, both from our in-memory store
   // and from the backing store.
   for (EquivalenceMap::iterator it = equivalent_cookies.begin();
@@ -311,6 +329,7 @@ int CookieMonster::TrimDuplicateCookiesForHost(
 
     if (dupes.size() <= 1)
       continue;  // This cookiename/path has no duplicates.
+    num_duplicates_found += dupes.size() - 1;
 
     // Since |dups| is sorted by creation time (descending), the first cookie
     // is the most recent one, so we will keep it. The rest are duplicates.
@@ -334,6 +353,7 @@ int CookieMonster::TrimDuplicateCookiesForHost(
                            DELETE_COOKIE_DUPLICATE_IN_BACKING_STORE);
     }
   }
+  DCHECK_EQ(num_duplicates, num_duplicates_found);
 
   return num_duplicates;
 }
@@ -654,13 +674,7 @@ bool CookieMonster::SetCookieWithCreationTimeAndOptions(
     const std::string& cookie_line,
     const Time& creation_time_or_null,
     const CookieOptions& options) {
-  AutoLock autolock(lock_);
-
-  if (!HasCookieableScheme(url)) {
-    return false;
-  }
-
-  InitIfNecessary();
+  lock_.AssertAcquired();
 
   COOKIE_DLOG(INFO) << "SetCookie() line: " << cookie_line;
 
@@ -706,6 +720,20 @@ bool CookieMonster::SetCookieWithCreationTimeAndOptions(
   return SetCanonicalCookie(&cc, creation_time, options);
 }
 
+bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
+                                              const std::string& cookie_line,
+                                              const base::Time& creation_time) {
+  AutoLock autolock(lock_);
+
+  if (!HasCookieableScheme(url)) {
+    return false;
+  }
+
+  InitIfNecessary();
+  return SetCookieWithCreationTimeAndOptions(url, cookie_line, creation_time,
+                                             CookieOptions());
+}
+
 bool CookieMonster::SetCookieWithDetails(
     const GURL& url, const std::string& name, const std::string& value,
     const std::string& domain, const std::string& path,
@@ -1030,6 +1058,14 @@ static bool CookieSorter(CookieMonster::CanonicalCookie* cc1,
 bool CookieMonster::SetCookieWithOptions(const GURL& url,
                                          const std::string& cookie_line,
                                          const CookieOptions& options) {
+  AutoLock autolock(lock_);

[ahendrickson, 2010/07/27 19:15:37]

I'm curious about why you moved this section (and in
SetCookieWithCreationTime()) out from SetCookieWithCreationTimeAndOptions().

[Randy Smith (Not in Mondays), 2010/07/27 20:11:47]

Historically, all of the functions that took the CookieMonster lock have been
public functions, and I was trying to preserve that.  But the it's not like
there's anything sacred about it; it just makes the locking structure slightly
easier to understand.  And in fact, I did break the invariant, as
SetCookieWithCreationTime() is now a private function, but it's still takes the
lock, as it's called from the test functions.  I figured it was more important
to take away the possibility for CookieMonster consumers to cause a nasty bug.

+
+  if (!HasCookieableScheme(url)) {
+    return false;
+  }
+
+  InitIfNecessary();
+
   return SetCookieWithCreationTimeAndOptions(url, cookie_line, Time(), options);
 }