Split initial load of cookies by domains

chrome/browser/net/sqlite_persistent_cookie_store.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.
 
 #include "chrome/browser/net/sqlite_persistent_cookie_store.h"
 
 #include <list>
+#include <map>
+#include <set>
+#include <utility>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "content/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/registry_controlled_domain.h"
+

[erikwright (departed), 2011/09/20 19:00:44]

nit: Remove this blank line.

[guohui, 2011/10/06 15:40:00]

Done.

 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 
 using base::Time;
 
 // This class is designed to be shared between any calling threads and the
-// database thread.  It batches operations and commits them on a timer.
-// This class expects to be Load()'ed once on any thread. Loading occurs
-// asynchronously on the DB thread and the caller will be notified on the IO
-// thread. Subsequent to loading, mutations may be queued by any thread using
+// database thread. It batches operations and commits them on a timer.
+
+// This class loads cookies by domain keys (always eTLD+1). Loading is triggered

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

I thought it was the CookieMonster that was based around eTLD+1, and this class
was based around the cookie domain; am I confused?

If you're referring to the arguments to Load & LoadCookiesForKey, I agree with
you, but given that the database is organized (I believe) around cookie domains,
I found this lead in describing what the class as a whole did confusing (it does
things besides load cookies).  If that's what you meant, could you rephrase?

[guohui, 2011/10/06 15:40:00]

Done.

+// either by Load() which chain-loads cookies for all domain keys, or by
+// LoadCookiesForKey() which loads cookies for a specific domain key and thus
+// allows individual URL requests to be processed before the whole cookie store
+// is loaded. In both cases, loading occurs asynchronously on the DB thread and
+// the caller will be notified on the IO thread upon the loading completes, with
+// all cookies that have been accumulated in cookies_ since last notification.
+
+// Subsequent to loading, mutations may be queued by any thread using
 // AddCookie, UpdateCookieAccessTime, and DeleteCookie. These are flushed to
 // disk on the DB thread every 30 seconds, 512 operations, or call to Flush(),
 // whichever occurs first.
 class SQLitePersistentCookieStore::Backend
     : public base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend> {
  public:
   explicit Backend(const FilePath& path)
       : path_(path),
         db_(NULL),
         num_pending_(0),
         clear_local_state_on_exit_(false) {
   }
 
-  // Creates or load the SQLite database.
-  bool Load(const LoadedCallback& loaded_callback);
+  // Creates or loads the SQLite database.
+  void Load(const LoadedCallback& loaded_callback);
+
+  // Loads cookies for the domain key (eTLD+1).
+  void LoadCookiesForKey(const std::string& domain,
+      const LoadedCallback& loaded_callback);
 
   // Batch a cookie addition.
   void AddCookie(const net::CookieMonster::CanonicalCookie& cc);
 
   // Batch a cookie access time update.
   void UpdateCookieAccessTime(const net::CookieMonster::CanonicalCookie& cc);
 
   // Batch a cookie deletion.
   void DeleteCookie(const net::CookieMonster::CanonicalCookie& cc);
 
@@ skipping 32 matching lines @@
 
     OperationType op() const { return op_; }
     const net::CookieMonster::CanonicalCookie& cc() const { return cc_; }
 
    private:
     OperationType op_;
     net::CookieMonster::CanonicalCookie cc_;
   };
 
  private:
-  // Creates or load the SQLite database on DB thread.
+  // Creates or loads the SQLite database on DB thread.
   void LoadAndNotifyOnDBThread(const LoadedCallback& loaded_callback);
-  // Notify the CookieMonster when loading complete.
+
+  // Loads cookies for the domain key (eTLD+1) on DB thread.
+  void LoadKeyAndNotifyOnDBThread(const std::string& domains,
+      const LoadedCallback& loaded_callback);
+
+  // Notifies the CookieMonster when loading completes for a specific domain key
+  // or for all domain keys. Triggers the callback and passes it all cookies
+  // that have been loaded from DB since last IO notification.
   void NotifyOnIOThread(
       const LoadedCallback& loaded_callback,
-      bool load_success,
-      const std::vector<net::CookieMonster::CanonicalCookie*>& cookies);
+      bool load_success);
   // Initialize the data base.
+
   bool InitializeDatabase();
-  // Load cookies to the data base, and read cookies.
-  bool LoadInternal(std::vector<net::CookieMonster::CanonicalCookie*>* cookies);
+
+  // Chain load cookies from DB by domain key.
+  void ChainLoadCookies(const LoadedCallback& loaded_callback);
+
+  // Load all cookies for a set of domains/hosts
+  bool LoadCookiesForDomains(const std::set<std::string>& key);
 
   // Batch a cookie operation (add or delete)
   void BatchOperation(PendingOperation::OperationType op,
                       const net::CookieMonster::CanonicalCookie& cc);
   // Commit our pending operations to the database.
   void Commit();
   // Close() executed on the background thread.
   void InternalBackgroundClose();
 
   FilePath path_;
   scoped_ptr<sql::Connection> db_;
   sql::MetaTable meta_table_;
 
   typedef std::list<PendingOperation*> PendingOperationsList;
   PendingOperationsList pending_;
   PendingOperationsList::size_type num_pending_;
   // True if the persistent store should be deleted upon destruction.
   bool clear_local_state_on_exit_;
-  // Guard |pending_|, |num_pending_| and |clear_local_state_on_exit_|.
+  // Guard |cookies_|, |pending_|, |num_pending_| and
+  // |clear_local_state_on_exit_|.
   base::Lock lock_;
 
+  // Temporary buffer for cookies loaded from DB. Accumulates cookies to reduce
+  // the number of messages sent to the IO thread. Sent back in response to
+  // individual load requests for domain keys or when all loading completes.
+  std::vector<net::CookieMonster::CanonicalCookie*> cookies_;
+
+  // Map of domain keys(eTLD+1) to domains/hosts that are to be loaded from DB.
+  std::map<std::string, std::set<std::string>> keys_to_load_;
+
+  // Indicates if DB has been initialized.
+  bool initialized_;
+
   DISALLOW_COPY_AND_ASSIGN(Backend);
 };
 
 // Version number of the database. In version 4, we migrated the time epoch.
 // If you open the DB with an older version on Mac or Linux, the times will
 // look wonky, but the file will likely be usable. On Windows version 3 and 4
 // are the same.
 //
 // Version 3 updated the database to include the last access time, so we can
 // expire them in decreasing order of use when we've reached the maximum
@@ skipping 17 matching lines @@
                      "secure INTEGER NOT NULL,"
                      "httponly INTEGER NOT NULL,"
                      "last_access_utc INTEGER NOT NULL)"))
       return false;
   }
 
   // Try to create the index every time. Older versions did not have this index,
   // so we want those people to get it. Ignore errors, since it may exist.
   db->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
               " (creation_utc)");
+
+  db->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies(host_key)");
+
   return true;
 }
 
 }  // namespace
 
-bool SQLitePersistentCookieStore::Backend::Load(
+void SQLitePersistentCookieStore::Backend::Load(
     const LoadedCallback& loaded_callback) {
   // This function should be called only once per instance.
   DCHECK(!db_.get());
   BrowserThread::PostTask(
       BrowserThread::DB, FROM_HERE,
-      base::Bind(&Backend::LoadAndNotifyOnDBThread, base::Unretained(this),
-                 loaded_callback));
-  return true;
+      base::Bind(&Backend::LoadAndNotifyOnDBThread, this, loaded_callback));
+}
+
+void SQLitePersistentCookieStore::Backend::LoadCookiesForKey(
+    const std::string& key,
+    const LoadedCallback& loaded_callback) {
+  BrowserThread::PostTask(
+    BrowserThread::DB, FROM_HERE,
+    base::Bind(&Backend::LoadKeyAndNotifyOnDBThread, this,
+    key,
+    loaded_callback));
 }
 
 void SQLitePersistentCookieStore::Backend::LoadAndNotifyOnDBThread(
     const LoadedCallback& loaded_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
-  std::vector<net::CookieMonster::CanonicalCookie*> cookies;
 
-  bool load_success = LoadInternal(&cookies);
+  if (!InitializeDatabase()) {
+    BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&SQLitePersistentCookieStore::Backend::NotifyOnIOThread,
+                 this, loaded_callback, false));
+  } else {
+    ChainLoadCookies(loaded_callback);
+  }
+}
 
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
-      &SQLitePersistentCookieStore::Backend::NotifyOnIOThread,
-      base::Unretained(this), loaded_callback, load_success, cookies));
+void SQLitePersistentCookieStore::Backend::LoadKeyAndNotifyOnDBThread(
+    const std::string& key,
+    const LoadedCallback& loaded_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+
+  bool success = false;
+  if (InitializeDatabase()) {
+    std::map<std::string, std::set<std::string> >::iterator
+      it = keys_to_load_.find(key);
+    if (it != keys_to_load_.end()) {
+      success = LoadCookiesForDomains(it->second);
+      keys_to_load_.erase(it);
+    } else {
+      success = true;
+    }
+  }
+
+  BrowserThread::PostTask(
+    BrowserThread::IO, FROM_HERE,
+    base::Bind(&SQLitePersistentCookieStore::Backend::NotifyOnIOThread,
+    this, loaded_callback, success));
 }
 
 void SQLitePersistentCookieStore::Backend::NotifyOnIOThread(
     const LoadedCallback& loaded_callback,
-    bool load_success,
-    const std::vector<net::CookieMonster::CanonicalCookie*>& cookies) {
+    bool load_success) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  std::vector<net::CookieMonster::CanonicalCookie*> cookies;
+  {
+    base::AutoLock locked(lock_);
+    cookies = cookies_;
+    cookies_.clear();
+  }
+
   loaded_callback.Run(cookies);
 }
 
 bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
+  if (initialized_) {
+    return true;
+  }
+
   const FilePath dir = path_.DirName();
   if (!file_util::PathExists(dir) && !file_util::CreateDirectory(dir)) {
     return false;
   }
 
   db_.reset(new sql::Connection);
   if (!db_->Open(path_)) {
     NOTREACHED() << "Unable to open cookie DB.";
     db_.reset();
     return false;
   }
 
   db_->set_error_delegate(GetErrorHandlerForCookieDb());
 
   if (!EnsureDatabaseVersion() || !InitTable(db_.get())) {
     NOTREACHED() << "Unable to open cookie DB.";
     db_.reset();
     return false;
   }
 
   db_->Preload();
-  return true;
-}
 
-bool SQLitePersistentCookieStore::Backend::LoadInternal(
-    std::vector<net::CookieMonster::CanonicalCookie*>* cookies) {
-  if (!InitializeDatabase()) {
-    return false;
-  }
+  // Retrieve all the domains
+  sql::Statement smt(db_->GetUniqueStatement(
+    "SELECT DISTINCT host_key FROM cookies"));
 
-  // Slurp all the cookies into the out-vector.
-  sql::Statement smt(db_->GetUniqueStatement(
-      "SELECT creation_utc, host_key, name, value, path, expires_utc, secure, "
-      "httponly, last_access_utc FROM cookies"));
   if (!smt) {
     NOTREACHED() << "select statement prep failed";
     db_.reset();
     return false;
   }
 
+  // Build a map of domain keys (always eTLD+1) to domains.
   while (smt.Step()) {
-    scoped_ptr<net::CookieMonster::CanonicalCookie> cc(
-        new net::CookieMonster::CanonicalCookie(
-            // The "source" URL is not used with persisted cookies.
-            GURL(),                                         // Source
-            smt.ColumnString(2),                            // name
-            smt.ColumnString(3),                            // value
-            smt.ColumnString(1),                            // domain
-            smt.ColumnString(4),                            // path
-            std::string(),  // TODO(abarth): Persist mac_key
-            std::string(),  // TODO(abarth): Persist mac_algorithm
-            Time::FromInternalValue(smt.ColumnInt64(0)),    // creation_utc
-            Time::FromInternalValue(smt.ColumnInt64(5)),    // expires_utc
-            Time::FromInternalValue(smt.ColumnInt64(8)),    // last_access_utc
-            smt.ColumnInt(6) != 0,                          // secure
-            smt.ColumnInt(7) != 0,                          // httponly
-            true));                                         // has_expires
-    DLOG_IF(WARNING,
-            cc->CreationDate() > Time::Now()) << L"CreationDate too recent";
-    cookies->push_back(cc.release());
+    std::string domain = smt.ColumnString(0);
+    std::string key =
+      net::RegistryControlledDomainService::GetDomainAndRegistry(domain);
+
+    std::map<std::string, std::set<std::string> >::iterator it =
+      keys_to_load_.find(key);
+    if (it == keys_to_load_.end())
+      it = keys_to_load_.insert(std::make_pair
+                                (key, std::set<std::string>())).first;
+    it->second.insert(domain);
+  }
+
+  initialized_ = true;
+  return true;
+}
+
+void SQLitePersistentCookieStore::Backend::ChainLoadCookies(
+    const LoadedCallback& loaded_callback) {

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

It's fairly important (for both reader and computer :-}) to track what threads
these routines are allowed to be run on.  Could you add
"DCHECK(BrowserThread::CurrentlyOn" statements to the new functions you've added
(and could you put one in InitializeDatabase while you're at it?)

[guohui, 2011/10/06 15:40:00]

Done.

+  bool load_success = true;
+
+  if (keys_to_load_.size() > 0) {
+    // Load cookies for the first domain key.
+    std::map<std::string, std::set<std::string> >::iterator
+      it = keys_to_load_.begin();
+    load_success = LoadCookiesForDomains(it->second);
+    keys_to_load_.erase(it);
+  }
+
+  // If load is successful and there are more domain keys to be loaded,
+  // then post a DB task to continue chain-load;
+  // Otherwise notify on IO thread.
+  if (load_success && keys_to_load_.size() > 0) {
+    BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(&Backend::ChainLoadCookies, this, loaded_callback));
+  } else {
+    BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&SQLitePersistentCookieStore::Backend::NotifyOnIOThread,
+                 this, loaded_callback, load_success));
+  }
+}
+
+bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
+  const std::set<std::string>& domains) {
+  sql::Statement smt(db_->GetCachedStatement(SQL_FROM_HERE,
+    "SELECT creation_utc, host_key, name, value, path, expires_utc, secure, "
+    "httponly, last_access_utc FROM cookies WHERE host_key = ?"));
+  if (!smt) {
+    NOTREACHED() << "select statement prep failed";
+    db_.reset();
+    return false;
+  }
+
+  std::set<std::string>::const_iterator it = domains.begin();
+  for (; it != domains.end(); ++it) {
+    base::AutoLock locked(lock_);

[erikwright (departed), 2011/09/20 19:00:44]

I think what Randy had in mind was to only lock around line 405. The file IO he
mentioned must either be occurring in line 385 or 386, so moving the lock here
has only reduced the average length of time the lock is held, not the class of
operations performed while holding it.

That being said, I hesitate to acquire the lock once per cookie in the store. I
would either revert this change or accumulate all cookies in a local collection
and then acquire the lock just before copying all the pointers from the local
collection to the member collection.

Randy, what are your thoughts?

That being said, I hesitate to 

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

Yes, and I think of file system operations as generally being an order of
magnitude longer than almost anything you can do in core. 
I don't like the idea of taking the lock once per cookie, but I *really* don't
like the idea of holding the lock over a file system operation (the former
"just" burns CPU/bus cycles, the latter put a potentially large delay in
acquisition for other threads grabbing the lock).  So I'm in favor of
accumulating the cookies in a local collection and pushing them onto cookies_
inside the lock once you've created all of them.

A possible alternative is skipping locking around cookies_ at all, and instead
returning them back through the callback.  The easy way to do this would be to
accumulate them as currently, but pull them off the backend into a variable
that's tossed across to the other thread in
ChainLoadCookies/LoadkKeyAndNotifyOnDBThread (i.e. they would only be accessed
on the DB thread).  That means you can stop worrying about this issue
completely.  (Of the options raised so far I think I like this the best, but I'm
happy to keep the lock if you don't hold it across file system operations or
take it per cookie, and would accept taking it per cookie.)

[guohui, 2011/10/06 15:40:00]

Fixed by accumulating all cookies in a local collection and then moving its
content to the member collection. I think we have to use a member variable to
accumulate cookies, since ChainLoadCookies does not return cookies immediately
and LoadKey could potentially return cookies that are loaded by
ChainLoadCookies. And since cookies_ are accessed on both DB thread and IO
thread(by NotifyOnIOThread), we need acquire a lock before accessing it.

On 2011/09/20 19:00:44, erikwright wrote:

+    smt.BindString(0, *it);
+    while (smt.Step()) {
+      scoped_ptr<net::CookieMonster::CanonicalCookie> cc(
+          new net::CookieMonster::CanonicalCookie(
+              // The "source" URL is not used with persisted cookies.
+              GURL(),                                         // Source
+              smt.ColumnString(2),                            // name
+              smt.ColumnString(3),                            // value
+              smt.ColumnString(1),                            // domain
+              smt.ColumnString(4),                            // path
+              std::string(),  // TODO(abarth): Persist mac_key
+              std::string(),  // TODO(abarth): Persist mac_algorithm
+              Time::FromInternalValue(smt.ColumnInt64(0)),    // creation_utc
+              Time::FromInternalValue(smt.ColumnInt64(5)),    // expires_utc
+              Time::FromInternalValue(smt.ColumnInt64(8)),    // last_access_utc
+              smt.ColumnInt(6) != 0,                          // secure
+              smt.ColumnInt(7) != 0,                          // httponly
+              true));                                         // has_expires
+      DLOG_IF(WARNING,
+              cc->CreationDate() > Time::Now()) << L"CreationDate too recent";
+      cookies_.push_back(cc.release());
+    }
+    smt.Reset();
   }
 
   return true;
 }
 
 bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
   // Version check.
   if (!meta_table_.Init(
       db_.get(), kCurrentVersionNumber, kCompatibleVersionNumber)) {
     return false;
@@ skipping 250 matching lines @@
 
 SQLitePersistentCookieStore::~SQLitePersistentCookieStore() {
   if (backend_.get()) {
     backend_->Close();
     // Release our reference, it will probably still have a reference if the
     // background thread has not run Close() yet.
     backend_ = NULL;
   }
 }
 
-bool SQLitePersistentCookieStore::Load(const LoadedCallback& loaded_callback) {
-  return backend_->Load(loaded_callback);
+void SQLitePersistentCookieStore::Load(const LoadedCallback& loaded_callback) {
+  backend_->Load(loaded_callback);
+}
+
+void SQLitePersistentCookieStore::LoadCookiesForKey(
+    const std::string& key,
+    const LoadedCallback& loaded_callback) {
+  backend_->LoadCookiesForKey(key, loaded_callback);
 }
 
 void SQLitePersistentCookieStore::AddCookie(
     const net::CookieMonster::CanonicalCookie& cc) {
   if (backend_.get())
     backend_->AddCookie(cc);
 }
 
 void SQLitePersistentCookieStore::UpdateCookieAccessTime(
     const net::CookieMonster::CanonicalCookie& cc) {
@@ skipping 12 matching lines @@
   if (backend_.get())
     backend_->SetClearLocalStateOnExit(clear_local_state);
 }
 
 void SQLitePersistentCookieStore::Flush(Task* completion_task) {
   if (backend_.get())
     backend_->Flush(completion_task);
   else if (completion_task)
     MessageLoop::current()->PostTask(FROM_HERE, completion_task);
 }