Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(135)

Side by Side Diff: net/base/cookie_monster.cc

Issue 3070001: Fixes targeting the unique creation times invariant in the CookieMonster: (Closed)
Patch Set: Made creation times in perftest unique. Created 10 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « net/base/cookie_monster.h ('k') | net/base/cookie_monster_perftest.cc » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright (c) 2010 The Chromium Authors. All rights reserved. 1 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 // Portions of this code based on Mozilla: 5 // Portions of this code based on Mozilla:
6 // (netwerk/cookie/src/nsCookieService.cpp) 6 // (netwerk/cookie/src/nsCookieService.cpp)
7 /* ***** BEGIN LICENSE BLOCK ***** 7 /* ***** BEGIN LICENSE BLOCK *****
8 * Version: MPL 1.1/GPL 2.0/LGPL 2.1 8 * Version: MPL 1.1/GPL 2.0/LGPL 2.1
9 * 9 *
10 * The contents of this file are subject to the Mozilla Public License Version 10 * The contents of this file are subject to the Mozilla Public License Version
(...skipping 167 matching lines...) Expand 10 before | Expand all | Expand 10 after
178 DCHECK(store_) << "Store must exist to initialize"; 178 DCHECK(store_) << "Store must exist to initialize";
179 179
180 // Initialize the store and sync in any saved persistent cookies. We don't 180 // Initialize the store and sync in any saved persistent cookies. We don't
181 // care if it's expired, insert it so it can be garbage collected, removed, 181 // care if it's expired, insert it so it can be garbage collected, removed,
182 // and sync'd. 182 // and sync'd.
183 std::vector<KeyedCanonicalCookie> cookies; 183 std::vector<KeyedCanonicalCookie> cookies;
184 // Reserve space for the maximum amount of cookies a database should have. 184 // Reserve space for the maximum amount of cookies a database should have.
185 // This prevents multiple vector growth / copies as we append cookies. 185 // This prevents multiple vector growth / copies as we append cookies.
186 cookies.reserve(kNumCookiesTotal); 186 cookies.reserve(kNumCookiesTotal);
187 store_->Load(&cookies); 187 store_->Load(&cookies);
188
189 // Avoid ever letting cookies with duplicate creation times into the store;
190 // that way we don't have to worry about what sections of code are safe
191 // to call while it's in that state.
192 std::set<int64> creation_times;
188 for (std::vector<KeyedCanonicalCookie>::const_iterator it = cookies.begin(); 193 for (std::vector<KeyedCanonicalCookie>::const_iterator it = cookies.begin();
189 it != cookies.end(); ++it) { 194 it != cookies.end(); ++it) {
190 InternalInsertCookie(it->first, it->second, false); 195 int64 cookie_creation_time = it->second->CreationDate().ToInternalValue();
196
197 if (creation_times.insert(cookie_creation_time).second) {
198 InternalInsertCookie(it->first, it->second, false);
199 } else {
200 LOG(ERROR) << StringPrintf("Found cookies with duplicate creation "
201 "times in backing store: "
202 "{name='%s', domain='%s', path='%s'}",
203 it->second->Name().c_str(),
204 it->first.c_str(),
205 it->second->Path().c_str());
206 }
191 } 207 }
192 208
193 // After importing cookies from the PersistentCookieStore, verify that 209 // After importing cookies from the PersistentCookieStore, verify that
194 // none of our constraints are violated. 210 // none of our other constraints are violated.
195 // 211 //
196 // In particular, the backing store might have given us duplicate cookies. 212 // In particular, the backing store might have given us duplicate cookies.
197 EnsureCookiesMapIsValid(); 213 EnsureCookiesMapIsValid();
198 } 214 }
199 215
200 void CookieMonster::EnsureCookiesMapIsValid() { 216 void CookieMonster::EnsureCookiesMapIsValid() {
201 lock_.AssertAcquired(); 217 lock_.AssertAcquired();
202 218
203 int num_duplicates_trimmed = 0; 219 int num_duplicates_trimmed = 0;
204 220
205 // Iterate through all the of the cookies, grouped by host. 221 // Iterate through all the of the cookies, grouped by host.
206 CookieMap::iterator prev_range_end = cookies_.begin(); 222 CookieMap::iterator prev_range_end = cookies_.begin();
207 while (prev_range_end != cookies_.end()) { 223 while (prev_range_end != cookies_.end()) {
208 CookieMap::iterator cur_range_begin = prev_range_end; 224 CookieMap::iterator cur_range_begin = prev_range_end;
209 const std::string key = cur_range_begin->first; // Keep a copy. 225 const std::string key = cur_range_begin->first; // Keep a copy.
210 CookieMap::iterator cur_range_end = cookies_.upper_bound(key); 226 CookieMap::iterator cur_range_end = cookies_.upper_bound(key);
211 prev_range_end = cur_range_end; 227 prev_range_end = cur_range_end;
212 228
213 // Ensure no equivalent cookies for this host. 229 // Ensure no equivalent cookies for this host.
214 num_duplicates_trimmed += 230 num_duplicates_trimmed +=
215 TrimDuplicateCookiesForHost(key, cur_range_begin, cur_range_end); 231 TrimDuplicateCookiesForHost(key, cur_range_begin, cur_range_end);
216 } 232 }
217 233
218 // Record how many duplicates were found in the database. 234 // Record how many duplicates were found in the database.
219 // See InitializeHistograms() for details. 235 // See InitializeHistograms() for details.
220 histogram_cookie_deletion_cause_->Add(num_duplicates_trimmed); 236 histogram_cookie_deletion_cause_->Add(num_duplicates_trimmed);
221
222 // TODO(eroman): Should also verify that there are no cookies with the same
223 // creation time, since that is assumed to be unique by the rest of the code.
224 } 237 }
225 238
226 // Our strategy to find duplicates is: 239 // Our strategy to find duplicates is:
227 // (1) Build a map from (cookiename, cookiepath) to 240 // (1) Build a map from (cookiename, cookiepath) to
228 // {list of cookies with this signature, sorted by creation time}. 241 // {list of cookies with this signature, sorted by creation time}.
229 // (2) For each list with more than 1 entry, keep the cookie having the 242 // (2) For each list with more than 1 entry, keep the cookie having the
230 // most recent creation time, and delete the others. 243 // most recent creation time, and delete the others.
231 namespace { 244 namespace {
232 // Two cookies are considered equivalent if they have the same domain, 245 // Two cookies are considered equivalent if they have the same domain,
233 // name, and path. 246 // name, and path.
(...skipping 53 matching lines...) Expand 10 before | Expand all | Expand 10 after
287 CookieSignature signature(cookie->Name(), cookie->Domain(), 300 CookieSignature signature(cookie->Name(), cookie->Domain(),
288 cookie->Path()); 301 cookie->Path());
289 CookieSet& list = equivalent_cookies[signature]; 302 CookieSet& list = equivalent_cookies[signature];
290 303
291 // We found a duplicate! 304 // We found a duplicate!
292 if (!list.empty()) 305 if (!list.empty())
293 num_duplicates++; 306 num_duplicates++;
294 307
295 // We save the iterator into |cookies_| rather than the actual cookie 308 // We save the iterator into |cookies_| rather than the actual cookie
296 // pointer, since we may need to delete it later. 309 // pointer, since we may need to delete it later.
297 list.insert(it); 310 bool insert_success = list.insert(it).second;
311 DCHECK(insert_success) <<
312 "Duplicate creation times found in duplicate cookie name scan.";
298 } 313 }
299 314
300 // If there were no duplicates, we are done! 315 // If there were no duplicates, we are done!
301 if (num_duplicates == 0) 316 if (num_duplicates == 0)
302 return 0; 317 return 0;
303 318
319 // Make sure we find everything below that we did above.
320 int num_duplicates_found = 0;
321
304 // Otherwise, delete all the duplicate cookies, both from our in-memory store 322 // Otherwise, delete all the duplicate cookies, both from our in-memory store
305 // and from the backing store. 323 // and from the backing store.
306 for (EquivalenceMap::iterator it = equivalent_cookies.begin(); 324 for (EquivalenceMap::iterator it = equivalent_cookies.begin();
307 it != equivalent_cookies.end(); 325 it != equivalent_cookies.end();
308 ++it) { 326 ++it) {
309 const CookieSignature& signature = it->first; 327 const CookieSignature& signature = it->first;
310 CookieSet& dupes = it->second; 328 CookieSet& dupes = it->second;
311 329
312 if (dupes.size() <= 1) 330 if (dupes.size() <= 1)
313 continue; // This cookiename/path has no duplicates. 331 continue; // This cookiename/path has no duplicates.
332 num_duplicates_found += dupes.size() - 1;
314 333
315 // Since |dups| is sorted by creation time (descending), the first cookie 334 // Since |dups| is sorted by creation time (descending), the first cookie
316 // is the most recent one, so we will keep it. The rest are duplicates. 335 // is the most recent one, so we will keep it. The rest are duplicates.
317 dupes.erase(dupes.begin()); 336 dupes.erase(dupes.begin());
318 337
319 LOG(ERROR) << StringPrintf("Found %d duplicate cookies for host='%s', " 338 LOG(ERROR) << StringPrintf("Found %d duplicate cookies for host='%s', "
320 "with {name='%s', domain='%s', path='%s'}", 339 "with {name='%s', domain='%s', path='%s'}",
321 static_cast<int>(dupes.size()), 340 static_cast<int>(dupes.size()),
322 key.c_str(), 341 key.c_str(),
323 signature.name.c_str(), 342 signature.name.c_str(),
324 signature.domain.c_str(), 343 signature.domain.c_str(),
325 signature.path.c_str()); 344 signature.path.c_str());
326 345
327 // Remove all the cookies identified by |dupes|. It is valid to delete our 346 // Remove all the cookies identified by |dupes|. It is valid to delete our
328 // list of iterators one at a time, since |cookies_| is a multimap (they 347 // list of iterators one at a time, since |cookies_| is a multimap (they
329 // don't invalidate existing iterators following deletion). 348 // don't invalidate existing iterators following deletion).
330 for (CookieSet::iterator dupes_it = dupes.begin(); 349 for (CookieSet::iterator dupes_it = dupes.begin();
331 dupes_it != dupes.end(); 350 dupes_it != dupes.end();
332 ++dupes_it) { 351 ++dupes_it) {
333 InternalDeleteCookie(*dupes_it, true /*sync_to_store*/, 352 InternalDeleteCookie(*dupes_it, true /*sync_to_store*/,
334 DELETE_COOKIE_DUPLICATE_IN_BACKING_STORE); 353 DELETE_COOKIE_DUPLICATE_IN_BACKING_STORE);
335 } 354 }
336 } 355 }
356 DCHECK_EQ(num_duplicates, num_duplicates_found);
337 357
338 return num_duplicates; 358 return num_duplicates;
339 } 359 }
340 360
341 void CookieMonster::SetDefaultCookieableSchemes() { 361 void CookieMonster::SetDefaultCookieableSchemes() {
342 // Note: file must be the last scheme. 362 // Note: file must be the last scheme.
343 static const char* kDefaultCookieableSchemes[] = { "http", "https", "file" }; 363 static const char* kDefaultCookieableSchemes[] = { "http", "https", "file" };
344 int num_schemes = enable_file_scheme_ ? 3 : 2; 364 int num_schemes = enable_file_scheme_ ? 3 : 2;
345 SetCookieableSchemes(kDefaultCookieableSchemes, num_schemes); 365 SetCookieableSchemes(kDefaultCookieableSchemes, num_schemes);
346 } 366 }
(...skipping 300 matching lines...) Expand 10 before | Expand all | Expand 10 after
647 cookieable_schemes_.clear(); 667 cookieable_schemes_.clear();
648 cookieable_schemes_.insert(cookieable_schemes_.end(), 668 cookieable_schemes_.insert(cookieable_schemes_.end(),
649 schemes, schemes + num_schemes); 669 schemes, schemes + num_schemes);
650 } 670 }
651 671
652 bool CookieMonster::SetCookieWithCreationTimeAndOptions( 672 bool CookieMonster::SetCookieWithCreationTimeAndOptions(
653 const GURL& url, 673 const GURL& url,
654 const std::string& cookie_line, 674 const std::string& cookie_line,
655 const Time& creation_time_or_null, 675 const Time& creation_time_or_null,
656 const CookieOptions& options) { 676 const CookieOptions& options) {
657 AutoLock autolock(lock_); 677 lock_.AssertAcquired();
658
659 if (!HasCookieableScheme(url)) {
660 return false;
661 }
662
663 InitIfNecessary();
664 678
665 COOKIE_DLOG(INFO) << "SetCookie() line: " << cookie_line; 679 COOKIE_DLOG(INFO) << "SetCookie() line: " << cookie_line;
666 680
667 Time creation_time = creation_time_or_null; 681 Time creation_time = creation_time_or_null;
668 if (creation_time.is_null()) { 682 if (creation_time.is_null()) {
669 creation_time = CurrentTime(); 683 creation_time = CurrentTime();
670 last_time_seen_ = creation_time; 684 last_time_seen_ = creation_time;
671 } 685 }
672 686
673 // Parse the cookie. 687 // Parse the cookie.
(...skipping 25 matching lines...) Expand all
699 creation_time, creation_time, 713 creation_time, creation_time,
700 !cookie_expires.is_null(), cookie_expires)); 714 !cookie_expires.is_null(), cookie_expires));
701 715
702 if (!cc.get()) { 716 if (!cc.get()) {
703 COOKIE_DLOG(WARNING) << "Failed to allocate CanonicalCookie"; 717 COOKIE_DLOG(WARNING) << "Failed to allocate CanonicalCookie";
704 return false; 718 return false;
705 } 719 }
706 return SetCanonicalCookie(&cc, creation_time, options); 720 return SetCanonicalCookie(&cc, creation_time, options);
707 } 721 }
708 722
723 bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
724 const std::string& cookie_line,
725 const base::Time& creation_time) {
726 AutoLock autolock(lock_);
727
728 if (!HasCookieableScheme(url)) {
729 return false;
730 }
731
732 InitIfNecessary();
733 return SetCookieWithCreationTimeAndOptions(url, cookie_line, creation_time,
734 CookieOptions());
735 }
736
709 bool CookieMonster::SetCookieWithDetails( 737 bool CookieMonster::SetCookieWithDetails(
710 const GURL& url, const std::string& name, const std::string& value, 738 const GURL& url, const std::string& name, const std::string& value,
711 const std::string& domain, const std::string& path, 739 const std::string& domain, const std::string& path,
712 const base::Time& expiration_time, bool secure, bool http_only) { 740 const base::Time& expiration_time, bool secure, bool http_only) {
713 741
714 AutoLock autolock(lock_); 742 AutoLock autolock(lock_);
715 743
716 if (!HasCookieableScheme(url)) 744 if (!HasCookieableScheme(url))
717 return false; 745 return false;
718 746
(...skipping 304 matching lines...) Expand 10 before | Expand all | Expand 10 after
1023 static bool CookieSorter(CookieMonster::CanonicalCookie* cc1, 1051 static bool CookieSorter(CookieMonster::CanonicalCookie* cc1,
1024 CookieMonster::CanonicalCookie* cc2) { 1052 CookieMonster::CanonicalCookie* cc2) {
1025 if (cc1->Path().length() == cc2->Path().length()) 1053 if (cc1->Path().length() == cc2->Path().length())
1026 return cc1->CreationDate() < cc2->CreationDate(); 1054 return cc1->CreationDate() < cc2->CreationDate();
1027 return cc1->Path().length() > cc2->Path().length(); 1055 return cc1->Path().length() > cc2->Path().length();
1028 } 1056 }
1029 1057
1030 bool CookieMonster::SetCookieWithOptions(const GURL& url, 1058 bool CookieMonster::SetCookieWithOptions(const GURL& url,
1031 const std::string& cookie_line, 1059 const std::string& cookie_line,
1032 const CookieOptions& options) { 1060 const CookieOptions& options) {
1061 AutoLock autolock(lock_);
ahendrickson 2010/07/27 19:15:37 I'm curious about why you moved this section (and
Randy Smith (Not in Mondays) 2010/07/27 20:11:47 Historically, all of the functions that took the C
1062
1063 if (!HasCookieableScheme(url)) {
1064 return false;
1065 }
1066
1067 InitIfNecessary();
1068
1033 return SetCookieWithCreationTimeAndOptions(url, cookie_line, Time(), options); 1069 return SetCookieWithCreationTimeAndOptions(url, cookie_line, Time(), options);
1034 } 1070 }
1035 1071
1036 // Currently our cookie datastructure is based on Mozilla's approach. We have a 1072 // Currently our cookie datastructure is based on Mozilla's approach. We have a
1037 // hash keyed on the cookie's domain, and for any query we walk down the domain 1073 // hash keyed on the cookie's domain, and for any query we walk down the domain
1038 // components and probe for cookies until we reach the TLD, where we stop. 1074 // components and probe for cookies until we reach the TLD, where we stop.
1039 // For example, a.b.blah.com, we would probe 1075 // For example, a.b.blah.com, we would probe
1040 // - a.b.blah.com 1076 // - a.b.blah.com
1041 // - .a.b.blah.com (TODO should we check this first or second?) 1077 // - .a.b.blah.com (TODO should we check this first or second?)
1042 // - .b.blah.com 1078 // - .b.blah.com
(...skipping 596 matching lines...) Expand 10 before | Expand all | Expand 10 after
1639 std::string CookieMonster::CanonicalCookie::DebugString() const { 1675 std::string CookieMonster::CanonicalCookie::DebugString() const {
1640 return StringPrintf("name: %s value: %s domain: %s path: %s creation: %" 1676 return StringPrintf("name: %s value: %s domain: %s path: %s creation: %"
1641 PRId64, 1677 PRId64,
1642 name_.c_str(), value_.c_str(), 1678 name_.c_str(), value_.c_str(),
1643 domain_.c_str(), path_.c_str(), 1679 domain_.c_str(), path_.c_str(),
1644 static_cast<int64>(creation_date_.ToTimeT())); 1680 static_cast<int64>(creation_date_.ToTimeT()));
1645 } 1681 }
1646 1682
1647 } // namespace 1683 } // namespace
1648 1684
OLDNEW
« no previous file with comments | « net/base/cookie_monster.h ('k') | net/base/cookie_monster_perftest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698