Fixes targeting the unique creation times invariant in the CookieMonster:

net/base/cookie_monster.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 167 matching lines @@
   DCHECK(store_) << "Store must exist to initialize";
 
   // Initialize the store and sync in any saved persistent cookies.  We don't
   // care if it's expired, insert it so it can be garbage collected, removed,
   // and sync'd.
   std::vector<KeyedCanonicalCookie> cookies;
   // Reserve space for the maximum amount of cookies a database should have.
   // 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();
 }
 
 void CookieMonster::EnsureCookiesMapIsValid() {
   lock_.AssertAcquired();
 
   int num_duplicates_trimmed = 0;
 
   // Iterate through all the of the cookies, grouped by host.
   CookieMap::iterator prev_range_end = cookies_.begin();
   while (prev_range_end != cookies_.end()) {
     CookieMap::iterator cur_range_begin = prev_range_end;
     const std::string key = cur_range_begin->first;  // Keep a copy.
     CookieMap::iterator cur_range_end = cookies_.upper_bound(key);
     prev_range_end = cur_range_end;
 
     // Ensure no equivalent cookies for this host.
     num_duplicates_trimmed +=
         TrimDuplicateCookiesForHost(key, cur_range_begin, cur_range_end);
   }
 
   // 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:
 // (1) Build a map from (cookiename, cookiepath) to
 //     {list of cookies with this signature, sorted by creation time}.
 // (2) For each list with more than 1 entry, keep the cookie having the
 //     most recent creation time, and delete the others.
 namespace {
 // Two cookies are considered equivalent if they have the same domain,
 // name, and path.
@@ skipping 53 matching lines @@
     CookieSignature signature(cookie->Name(), cookie->Domain(),
                               cookie->Path());
     CookieSet& list = equivalent_cookies[signature];
 
     // We found a duplicate!
     if (!list.empty())
       num_duplicates++;
 
     // 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();
        it != equivalent_cookies.end();
        ++it) {
     const CookieSignature& signature = it->first;
     CookieSet& dupes = it->second;
 
     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.
     dupes.erase(dupes.begin());
 
     LOG(ERROR) << StringPrintf("Found %d duplicate cookies for host='%s', "
                                "with {name='%s', domain='%s', path='%s'}",
                                static_cast<int>(dupes.size()),
                                key.c_str(),
                                signature.name.c_str(),
                                signature.domain.c_str(),
                                signature.path.c_str());
 
     // Remove all the cookies identified by |dupes|. It is valid to delete our
     // list of iterators one at a time, since |cookies_| is a multimap (they
     // don't invalidate existing iterators following deletion).
     for (CookieSet::iterator dupes_it = dupes.begin();
          dupes_it != dupes.end();
          ++dupes_it) {
       InternalDeleteCookie(*dupes_it, true /*sync_to_store*/,
                            DELETE_COOKIE_DUPLICATE_IN_BACKING_STORE);
     }
   }
+  DCHECK_EQ(num_duplicates, num_duplicates_found);
 
   return num_duplicates;
 }
 
 void CookieMonster::SetDefaultCookieableSchemes() {
   // Note: file must be the last scheme.
   static const char* kDefaultCookieableSchemes[] = { "http", "https", "file" };
   int num_schemes = enable_file_scheme_ ? 3 : 2;
   SetCookieableSchemes(kDefaultCookieableSchemes, num_schemes);
 }
@@ skipping 300 matching lines @@
   cookieable_schemes_.clear();
   cookieable_schemes_.insert(cookieable_schemes_.end(),
                              schemes, schemes + num_schemes);
 }
 
 bool CookieMonster::SetCookieWithCreationTimeAndOptions(
     const GURL& url,
     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;
 
   Time creation_time = creation_time_or_null;
   if (creation_time.is_null()) {
     creation_time = CurrentTime();
     last_time_seen_ = creation_time;
   }
 
   // Parse the cookie.
@@ skipping 25 matching lines @@
                                creation_time, creation_time,
                                !cookie_expires.is_null(), cookie_expires));
 
   if (!cc.get()) {
     COOKIE_DLOG(WARNING) << "Failed to allocate CanonicalCookie";
     return false;
   }
   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,
     const base::Time& expiration_time, bool secure, bool http_only) {
 
   AutoLock autolock(lock_);
 
   if (!HasCookieableScheme(url))
     return false;
 
@@ skipping 304 matching lines @@
 static bool CookieSorter(CookieMonster::CanonicalCookie* cc1,
                          CookieMonster::CanonicalCookie* cc2) {
   if (cc1->Path().length() == cc2->Path().length())
     return cc1->CreationDate() < cc2->CreationDate();
   return cc1->Path().length() > cc2->Path().length();
 }
 
 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);
 }
 
 // Currently our cookie datastructure is based on Mozilla's approach.  We have a
 // hash keyed on the cookie's domain, and for any query we walk down the domain
 // components and probe for cookies until we reach the TLD, where we stop.
 // For example, a.b.blah.com, we would probe
 //   - a.b.blah.com
 //   - .a.b.blah.com (TODO should we check this first or second?)
 //   - .b.blah.com
@@ skipping 596 matching lines @@
 std::string CookieMonster::CanonicalCookie::DebugString() const {
   return StringPrintf("name: %s value: %s domain: %s path: %s creation: %"
                       PRId64,
                       name_.c_str(), value_.c_str(),
                       domain_.c_str(), path_.c_str(),
                       static_cast<int64>(creation_date_.ToTimeT()));
 }
 
 }  // namespace