Split initial load of cookies by domains

net/base/cookie_monster.cc

Index: net/base/cookie_monster.cc
===================================================================
--- net/base/cookie_monster.cc	(revision 103757)
+++ net/base/cookie_monster.cc	(working copy)
@@ -49,6 +49,7 @@
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ -67,6 +68,26 @@
 using base::TimeDelta;
 using base::TimeTicks;
 
+// In steady state, most cookie requests can be satisfied by the in memory
+// cookie monster store.  However, if a request comes in during the initial
+// cookie load, it must be delayed until that load completes. That is done by
+// queueing it on CookieMonster::queue_ and running it when notification of
+// cookie load completion is received via CookieMonster::OnLoaded. This callback
+// is passed to the persistent store from CookieMonster::InitStore(), which is
+// called on the first operation invoked on the CookieMonster.
+//
+// On the browser critical paths (e.g. for loading initial web pages in a
+// session restore) it may take too long to wait for the full load. If a cookie
+// request is for a specific URL, DoCookieTaskForURL is called, which triggers a
+// priority load if the key is not loaded yet by calling PersistentCookieStore
+// :: LoadCookiesForKey. The request is queued in CookieMonster::tasks_queued
+// and executed upon receiving notification of key load completion via
+// CookieMonster::OnKeyLoaded(). If multiple requests for the same eTLD+1 are
+// received before key load completion, only the first request calls
+// PersistentCookieStore::LoadCookiesForKey, all subsequent requests are queued
+// in CookieMonster::tasks_queued and executed upon receiving notification of
+// key load completion triggered by the first request for the same eTLD+1.
+
 static const int kMinutesInTenYears = 10 * 365 * 24 * 60;
 
 namespace net {
@@ -993,7 +1014,7 @@
                                    expiration_time, secure, http_only,
                                    callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetAllCookiesAsync(const GetCookieListCallback& callback) {
@@ -1011,7 +1032,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetAllCookiesForURLAsync(
@@ -1021,7 +1042,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteAllAsync(const DeleteCallback& callback) {
@@ -1046,7 +1067,7 @@
   scoped_refptr<DeleteAllForHostTask> task =
       new DeleteAllForHostTask(this, url, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteCanonicalCookieAsync(
@@ -1066,7 +1087,7 @@
   scoped_refptr<SetCookieWithOptionsTask> task =
       new SetCookieWithOptionsTask(this, url, cookie_line, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetCookiesWithOptionsAsync(
@@ -1076,7 +1097,7 @@
   scoped_refptr<GetCookiesWithOptionsTask> task =
       new GetCookiesWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetCookiesWithInfoAsync(
@@ -1086,7 +1107,7 @@
   scoped_refptr<GetCookiesWithInfoTask> task =
       new GetCookiesWithInfoTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteCookieAsync(const GURL& url,
@@ -1095,15 +1116,14 @@
   scoped_refptr<DeleteCookieTask> task =
       new DeleteCookieTask(this, url, cookie_name, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DoCookieTask(
     const scoped_refptr<CookieMonsterTask>& task_item) {
-  InitIfNecessary();
-
   {
     base::AutoLock autolock(lock_);
+    InitIfNecessary();
     if (!loaded_) {
       queue_.push(task_item);
       return;
@@ -1113,6 +1133,34 @@
   task_item->Run();
 }
 
+void CookieMonster::DoCookieTaskForURL(
+    const scoped_refptr<CookieMonsterTask>& task_item,
+    const GURL& url) {
+  {
+    base::AutoLock autolock(lock_);
+    InitIfNecessary();
+    // If cookies for the requested domain key (eTLD+1) have been loaded from DB
+    // then run the task, otherwise load from DB.
+    if (!loaded_) {
+      // Checks if the domain key has been loaded.
+      std::string key(GetEffectiveDomain(url.scheme(), url.host()));
+      if (keys_loaded_.find(key) == keys_loaded_.end()) {
+        std::map<std::string, std::queue<scoped_refptr<CookieMonsterTask> > >
+          ::iterator it = tasks_queued_.find(key);
+        if (it == tasks_queued_.end()) {
+          store_->LoadCookiesForKey(key,
+            base::Bind(&CookieMonster::OnKeyLoaded, this, key));
+          it = tasks_queued_.insert(std::make_pair(key,
+            std::queue<scoped_refptr<CookieMonsterTask> >())).first;
+        }
+        it->second.push(task_item);
+        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,
@@ -1470,6 +1518,39 @@
   InvokeQueue();
 }
 
+void CookieMonster::OnKeyLoaded(const std::string& key,
+                                const std::vector<CanonicalCookie*>& cookies) {
+  std::map<std::string, std::queue<scoped_refptr<CookieMonsterTask> > >
+    ::iterator it;
+  // This function does its own separate locking.
+  StoreLoadedCookies(cookies);
+
+  {
+    base::AutoLock autolock(lock_);
+    keys_loaded_.insert(key);
+    it = tasks_queued_.find(key);
+    if (it == tasks_queued_.end())
+      return;
+  }
+
+  // No lock is required here since 1) after the key is inserted into
+  // keys_loaded_, subsequent tasks for the same key will not be queued, so the
+  // queue pointed to by |it| is constant at this time point. 2) inserting a new
+  // element into a map or erasing an element from a map does not invalidate any
+  // iterator (except for iterators that point to the element that is being
+  // erased).

[Randy Smith (Not in Mondays), 2011/10/13 21:36:28]

I agree with this comment as stated, but my belief is that those guarantees by
the STL spec are all made in the context of single threaded access.  And STL
implementations are often really wonky, and rely on things right at the corners
of the C++ spec.  So I'd feel more comfortable if we were only working on a
local variable at this point.  

I think if you made the secondary structure inside of tasks_queued_ a deque
instead of a queue, you could use the "swap" method to pull the entire contents
of the specific keys queue into a stack based deque, and do all these operations
in about the same number of lines of code and performance without working inside
of a multi-threaded data structure.

Alternatively, I suspect it's ok if the variable that survived the autolock
block was a reference/pointer directly to the queue (i.e. to it->second rather
than it); I really can't see how that could be touched in a multi-threaded
context.  It still makes me twitch a bit, but I think it's safe, so with a
comment I'd be ok with it.

[erikwright (departed), 2011/10/14 01:15:50]

We hemmed and hawed over this one. I agree that it's a little ambiguous as to
whether this is OK.

I think the solution of switching to deque and swapping with a local variable is
a very reasonable and tidy one. Not only that, we will be able to do the erase
within the first autlock block :)

On 2011/10/13 21:36:28, rdsmith wrote:

[guohui, 2011/10/14 15:39:00]

Done.

+  while (!it->second.empty()) {
+    scoped_refptr<CookieMonsterTask> task = it->second.front();
+    task->Run();
+    it->second.pop();
+  }
+
+  {
+    base::AutoLock autolock(lock_);
+    tasks_queued_.erase(it);
+  }
+}
+
 void CookieMonster::StoreLoadedCookies(
     const std::vector<CanonicalCookie*>& cookies) {
   // Initialize the store and sync in any saved persistent cookies.  We don't
@@ -1477,24 +1558,16 @@
   // 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: "
@@ -1507,12 +1580,14 @@
       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.
+
+  // This method could be called multiple times due to priority loading, thus
+  // cookies loaded in previous runs will be validated again, but this is OK
+  // since they are expected to be much fewer than total DB.
   EnsureCookiesMapIsValid();
 }
 
@@ -1523,6 +1598,8 @@
       base::AutoLock autolock(lock_);
       if (queue_.empty()) {
         loaded_ = true;
+        creation_times_.clear();
+        keys_loaded_.clear();
         break;
       }
       request_task = queue_.front();