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,25 @@
 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. In this case
+// a priority load is triggered 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.

[Randy Smith (Not in Mondays), 2011/10/11 15:58:58]

Again, thank you--I very much like this summary.  One thing I'd request adding
to it: from the cookie monster perspective, what triggers a priority load and
what doesn't?  The cookie monster doesn't know from initial web pages.  You
might also want to mention "DoCookieTask" vs. "DoCookieTaskForUrl" since that's
how the priority/non-priority signal is done within the CM.

[guohui, 2011/10/11 20:15:37]

Done.

+
 static const int kMinutesInTenYears = 10 * 365 * 24 * 60;
 
 namespace net {
@@ -993,7 +1013,7 @@
                                    expiration_time, secure, http_only,
                                    callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetAllCookiesAsync(const GetCookieListCallback& callback) {
@@ -1011,7 +1031,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetAllCookiesForURLAsync(
@@ -1021,7 +1041,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteAllAsync(const DeleteCallback& callback) {
@@ -1046,7 +1066,7 @@
   scoped_refptr<DeleteAllForHostTask> task =
       new DeleteAllForHostTask(this, url, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteCanonicalCookieAsync(
@@ -1066,7 +1086,7 @@
   scoped_refptr<SetCookieWithOptionsTask> task =
       new SetCookieWithOptionsTask(this, url, cookie_line, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetCookiesWithOptionsAsync(
@@ -1076,7 +1096,7 @@
   scoped_refptr<GetCookiesWithOptionsTask> task =
       new GetCookiesWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::GetCookiesWithInfoAsync(
@@ -1086,7 +1106,7 @@
   scoped_refptr<GetCookiesWithInfoTask> task =
       new GetCookiesWithInfoTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTaskForURL(task, url);
 }
 
 void CookieMonster::DeleteCookieAsync(const GURL& url,
@@ -1095,15 +1115,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 +1132,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 +1517,26 @@
   InvokeQueue();
 }
 
+void CookieMonster::OnKeyLoaded(const std::string& key,
+                                const std::vector<CanonicalCookie*>& cookies) {
+  // This function does its own separate locking.
+  StoreLoadedCookies(cookies);
+  {
+    base::AutoLock autolock(lock_);
+    keys_loaded_.insert(key);
+  }

[Randy Smith (Not in Mondays), 2011/10/11 15:58:58]

I'm a bit worried by this series of statements.  If I think of the CookieMonster
as a purely thread-safe class (which I think it still is) there is a period of
time during which other threads can see a state in which we claim we haven't
loaded the key, but we've actually loaded it.  At a minimum this'll produce
extra messages flying back and forth to the DB thread, and I'm not sure (haven't
traced the pathway) if there are any worse possibilities (e.g. multiple copies
of a cookie showing up).  Is there something I'm missing that makes this thread
safe?

[erikwright (departed), 2011/10/11 17:24:46]

Previously, we guaranteed the ordering of all received tasks. If a task were
received between load completion and invocation of the last queued task, it
would be appended to the queue. This included the perfectly likely case of
invoking a queued task which, in turn, made another cookie request.

The changes to support priority loading remove some of the ordering guarantees
(i.e., requests from different domains do not have guaranteed ordering, and
requests that are not keyed will come after those that are). I think this is OK
(I can't think of a realistic case where it would be an issue).

At a minimum, we must ensure that we can safely accept a new request when
processing the queued ones. In order to do so, we must hold the lock while
accessing tasks_queued_, but not while invoking the actual tasks.

[Randy Smith (Not in Mondays), 2011/10/11 20:03:15]

I think I follow; there's no real difference in cookie monster behavior with the
cookies loaded and with the cookies unloaded; the real changes in state are
around setting keys_loaded_ (new requests are satisfied immediately) and
clearing tasks_queued_ (if keys_loaded_ had *not* been set, that would result in
new tasks being posted to the DB thread).

[guohui, 2011/10/11 20:15:37]

If a request is received between load completion on DB thread and the statement
at 1526, then yes we will send an extra message to the persistent cookie store.
And the persistent cookie store will guarantee that no duplicate cookies are
returned (by tracking a list of domains to be loaded, domains already loaded
will be removed  from the list).

On 2011/10/11 17:24:46, erikwright wrote:

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

I thought I understood, but now I'm less sure :-}.  Doesn't the code in
DoCookieTasksForURL check to see if we have any tasks queued for the key, and
avoid sending a load message if we do?  Since we haven't yet removed anything
from tasks_queued_ at this point, that race shouldn't produce another message to
the DB thread, should it?  (That was my understanding after I puzzled over
Erik's response for a bit, but if it's wrong, I'd like to get it right :-}).

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

DoCookieTaskForURL first checks keys_loaded_. So as soon as line 1526 executes,
no one else is inserting or mutating tasks_queued_[key].

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

+  std::map<std::string, std::queue<scoped_refptr<CookieMonsterTask> > >
+    ::iterator it = tasks_queued_.find(key);

[Randy Smith (Not in Mondays), 2011/10/11 15:58:58]

I think you need to hold the lock around all access to CM member variables--I
think the lock is needed for everything.  If you're worried about performance,
you could swap out the task_queue_ list for that key, and then run the tasks
outside the lock (probably a good idea, actually; we don't really know what
those tasks are going to do).  But I believe you need to hold the lock over any
CM value accesses.

(In general, I'd recommend looking quite closely at any functions that don't
start with AutoLock autolock(lock_); or lock_.AssertAcquired() as their first
statement.  Sometimes it's safe, but figuring that out usually requires some
careful consideration of race conditions, and even when it is safe, it's a
problem for maintainability).

[erikwright (departed), 2011/10/11 17:24:46]

It is correct that tasks_queued_ must be protected by a lock.

On the other hand, task->Run must not be protected by a lock (since it might
result in new requests to the cookie monster).

Since 'it' can possibly be invalidated when you release the lock the easiest
thing to do is probably:

queue local_copy;

{
  AutoLock autolock;
  iterator it = tasks_queued_.find(key);
  if (it != end) {
    local_copy = it->second;
    tasks_queued_.erase(it);
  }
}

while (!local_copy.empty()) {
 local_copy.front().Run();
 local_copy.pop();
}

[guohui, 2011/10/11 20:15:37]

Done.

+  if (it != tasks_queued_.end()) {
+    while (!it->second.empty()) {
+      scoped_refptr<CookieMonsterTask> task = it->second.front();
+      task->Run();
+      it->second.pop();
+    }
+    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 +1544,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 +1566,12 @@
       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.

[Randy Smith (Not in Mondays), 2011/10/11 15:58:58]

The combination of these two statements right next to each other makes it seem
like duplicate cookies could be an expected result of priority loading.  Could
you make it clear that these two statements aren't related to each
other/duplicate cookies are indication of something wrong/corrupted with the
backing store?

[guohui, 2011/10/11 20:15:37]

Done.

   EnsureCookiesMapIsValid();
 }
 
@@ -1523,6 +1582,8 @@
       base::AutoLock autolock(lock_);
       if (queue_.empty()) {
         loaded_ = true;
+        creation_times_.clear();
+        keys_loaded_.clear();
         break;
       }
       request_task = queue_.front();