Split initial load of cookies by domains

net/base/cookie_monster.cc

Index: net/base/cookie_monster.cc
===================================================================
--- net/base/cookie_monster.cc	(revision 100935)
+++ net/base/cookie_monster.cc	(working copy)
@@ -993,7 +993,7 @@
                                    expiration_time, secure, http_only,
                                    callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetAllCookiesAsync(const GetCookieListCallback& callback) {
@@ -1011,7 +1011,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetAllCookiesForURLAsync(
@@ -1021,7 +1021,7 @@
   scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
       new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteAllAsync(const DeleteCallback& callback) {
@@ -1046,7 +1046,7 @@
   scoped_refptr<DeleteAllForHostTask> task =
       new DeleteAllForHostTask(this, url, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteCanonicalCookieAsync(
@@ -1066,7 +1066,7 @@
   scoped_refptr<SetCookieWithOptionsTask> task =
       new SetCookieWithOptionsTask(this, url, cookie_line, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetCookiesWithOptionsAsync(
@@ -1076,7 +1076,7 @@
   scoped_refptr<GetCookiesWithOptionsTask> task =
       new GetCookiesWithOptionsTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::GetCookiesWithInfoAsync(
@@ -1086,7 +1086,7 @@
   scoped_refptr<GetCookiesWithInfoTask> task =
       new GetCookiesWithInfoTask(this, url, options, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DeleteCookieAsync(const GURL& url,
@@ -1095,7 +1095,7 @@
   scoped_refptr<DeleteCookieTask> task =
       new DeleteCookieTask(this, url, cookie_name, callback);
 
-  DoCookieTask(task);
+  DoCookieTask(task, url);
 }
 
 void CookieMonster::DoCookieTask(
@@ -1113,6 +1113,30 @@
   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,
@@ -1470,6 +1494,17 @@
   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
@@ -1477,24 +1512,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 +1534,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.
   EnsureCookiesMapIsValid();
 }
 
@@ -1523,6 +1550,8 @@
       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();