Split initial load of cookies by domains

net/base/cookie_monster.cc

 // Copyright (c) 2011 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 31 matching lines @@
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "net/base/cookie_monster.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/format_macros.h"

[erikwright (departed), 2011/09/17 02:22:10]

#include "base/callback.h" for Closure

[guohui, 2011/09/20 18:34:46]

Done.

 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "googleurl/src/gurl.h"
 #include "googleurl/src/url_canon.h"
@@ skipping 923 matching lines @@
 void CookieMonster::SetCookieWithDetailsAsync(
     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,
     const SetCookiesCallback& callback) {
   scoped_refptr<SetCookieWithDetailsTask> task =
       new SetCookieWithDetailsTask(this, url, name, value, domain, path,
                                    expiration_time, secure, http_only,
                                    callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetAllCookiesAsync(const GetCookieListCallback& callback) {
   scoped_refptr<GetAllCookiesTask> task =
       new GetAllCookiesTask(this, callback);
 
   DoCookieTask(task);
 }
 
 
 void CookieMonster::GetAllCookiesForURLWithOptionsAsync(
     const GURL& url,
     const CookieOptions& options,
     const GetCookieListCallback& callback) {
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetAllCookiesForURLAsync(
     const GURL& url, const GetCookieListCallback& callback) {
   CookieOptions options;
   options.set_include_httponly();
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteAllAsync(const DeleteCallback& callback) {
   scoped_refptr<DeleteAllTask> task =
       new DeleteAllTask(this, callback);
 
   DoCookieTask(task);
 }
 
 void CookieMonster::DeleteAllCreatedBetweenAsync(
     const Time& delete_begin, const Time& delete_end,
     const DeleteCallback& callback) {
   scoped_refptr<DeleteAllCreatedBetweenTask> task =
       new DeleteAllCreatedBetweenTask(this, delete_begin, delete_end,
                                       callback);
 
   DoCookieTask(task);
 }
 
 void CookieMonster::DeleteAllForHostAsync(
     const GURL& url, const DeleteCallback& callback) {
   scoped_refptr<DeleteAllForHostTask> task =
       new DeleteAllForHostTask(this, url, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteCanonicalCookieAsync(
     const CanonicalCookie& cookie,
     const DeleteCookieCallback& callback) {
   scoped_refptr<DeleteCanonicalCookieTask> task =
       new DeleteCanonicalCookieTask(this, cookie, callback);
 
   DoCookieTask(task);
 }
 
 void CookieMonster::SetCookieWithOptionsAsync(
     const GURL& url,
     const std::string& cookie_line,
     const CookieOptions& options,
     const SetCookiesCallback& callback) {
   scoped_refptr<SetCookieWithOptionsTask> task =
       new SetCookieWithOptionsTask(this, url, cookie_line, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetCookiesWithOptionsAsync(
     const GURL& url,
     const CookieOptions& options,
     const GetCookiesCallback& callback) {
   scoped_refptr<GetCookiesWithOptionsTask> task =
       new GetCookiesWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetCookiesWithInfoAsync(
     const GURL& url,
     const CookieOptions& options,
     const GetCookieInfoCallback& callback) {
   scoped_refptr<GetCookiesWithInfoTask> task =
       new GetCookiesWithInfoTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteCookieAsync(const GURL& url,
                                       const std::string& cookie_name,
                                       const base::Closure& callback) {
   scoped_refptr<DeleteCookieTask> task =
       new DeleteCookieTask(this, url, cookie_name, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DoCookieTask(
     const scoped_refptr<CookieMonsterTask>& task_item) {
   InitIfNecessary();

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

(Not fron this CL, but worrisome to me.)  Historically, InitIfNecessary has been
inside of the function AutoLock.  I'm not sure how I missed this on an earlier
review, but it worries me that it's been moved outside.  Erik, this is probably
a question for you: Is this really safe, and if so, why?  It probably deserves a
comment if it is.  If it's not safe, I think it's important to fix it.

 
   {
     base::AutoLock autolock(lock_);
     if (!loaded_) {
       queue_.push(task_item);
       return;
     }
   }
 
   task_item->Run();
 }
 
+void CookieMonster::DoCookieTask(
+    const scoped_refptr<CookieMonsterTask>& task_item,
+    const GURL& url) {
+  InitIfNecessary();

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

See above.

+
+  {
+    base::AutoLock autolock(lock_);
+    // If cookies for the requested domain key (eTLD+1) have been loaded from DB
+    // then run the task, otherwise load from DB.
+    if (!loaded_) {
+      // Check if the domain key has been loaded.
+      std::string key(GetEffectiveDomain(url.scheme(), url.host()));
+      if (keys_loaded_.find(key) == keys_loaded_.end()) {
+        store_->LoadCookiesForKey(key,
+          base::Bind(&CookieMonster::OnKeyLoaded, this,
+                     base::Bind(&CookieMonsterTask::Run, task_item.get()),
+                     key));
+        return;
+      }
+    }
+  }
+  task_item->Run();
+}
+
 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) {
   base::AutoLock autolock(lock_);
 
   if (!HasCookieableScheme(url))
     return false;
 
   Time creation_time = CurrentTime();
@@ skipping 337 matching lines @@
 
 void CookieMonster::OnLoaded(TimeTicks beginning_time,
                              const std::vector<CanonicalCookie*>& cookies) {
   StoreLoadedCookies(cookies);
   histogram_time_load_->AddTime(TimeTicks::Now() - beginning_time);
 
   // Invoke the task queue of cookie request.
   InvokeQueue();
 }
 
+void CookieMonster::OnKeyLoaded(
+    const base::Closure& request_task, const std::string& key,
+    const std::vector<CanonicalCookie*>& cookies) {
+  StoreLoadedCookies(cookies);

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

Would you put in a comment that this function does its own separate locking? 
That'll keep people from twitching about a lack of locking as they read the
code.  

(This is a minor performance loss, as we do double locking, but I don't think it
matters for this code path.)

[guohui, 2011/09/20 18:34:46]

Done.

+  {
+    base::AutoLock autolock(lock_);
+    keys_loaded_.insert(key);
+  }
+  request_task.Run();
+}
+
 void CookieMonster::StoreLoadedCookies(
     const std::vector<CanonicalCookie*>& cookies) {
   // 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.
   base::AutoLock autolock(lock_);
 
-  // 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;
-
-  // Presumably later than any access time in the store.
-  Time earliest_access_time;
-
   for (std::vector<CanonicalCookie*>::const_iterator it = cookies.begin();
        it != cookies.end(); ++it) {
     int64 cookie_creation_time = (*it)->CreationDate().ToInternalValue();
 
-    if (creation_times.insert(cookie_creation_time).second) {
+    if (creation_times_.insert(cookie_creation_time).second) {
       InternalInsertCookie(GetKey((*it)->Domain()), *it, false);
       const Time cookie_access_time((*it)->LastAccessDate());
-      if (earliest_access_time.is_null() ||
-          cookie_access_time < earliest_access_time)
-        earliest_access_time = cookie_access_time;
+      if (earliest_access_time_.is_null() ||
+          cookie_access_time < earliest_access_time_)
+        earliest_access_time_ = cookie_access_time;
     } else {
       LOG(ERROR) << base::StringPrintf("Found cookies with duplicate creation "
                                        "times in backing store: "
                                        "{name='%s', domain='%s', path='%s'}",
                                        (*it)->Name().c_str(),
                                        (*it)->Domain().c_str(),
                                        (*it)->Path().c_str());
       // We've been given ownership of the cookie and are throwing it
       // away; reclaim the space.
       delete (*it);
     }
   }
-  earliest_access_time_= earliest_access_time;
 
   // After importing cookies from the PersistentCookieStore, verify that
   // none of our other constraints are violated.
-  //
   // In particular, the backing store might have given us duplicate cookies.
+  // "Priority loaded" cookies will be validated more than once, but this is OK
+  // since they are expected to be much fewer than total DB.
   EnsureCookiesMapIsValid();
 }
 
 void CookieMonster::InvokeQueue() {
   while (true) {
     scoped_refptr<CookieMonsterTask> request_task;
     {
       base::AutoLock autolock(lock_);
       if (queue_.empty()) {

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

Are we using this queue anymore?  Or are we bouncing all CookieMonsterTasks off
the persistent store through the callbacks now?

[guohui, 2011/09/20 18:34:46]

Yes, this queue is still used for cookie tasks that are blocked till the whole
cookie store is loaded. The callback approach is used for other cookie tasks
that are blocked till cookies for a specific domain key is loaded. More comments
about this are put at top of cookie_monster.h.

On 2011/09/19 00:58:57, rdsmith wrote:

         loaded_ = true;
+        creation_times_.clear();
+        keys_loaded_.clear();
         break;
       }
       request_task = queue_.front();
       queue_.pop();
     }
     request_task->Run();
   }
 }
 
 void CookieMonster::EnsureCookiesMapIsValid() {
@@ skipping 1228 matching lines @@
 std::string CookieMonster::CanonicalCookie::DebugString() const {
   return base::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