SQL cookie store: Safely post to DB thread from IO thread.

BUG=25245
TEST=?

Review URL: http://codereview.chromium.org/295032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@29588 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 65 insertions, 32 deletions

diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 368e95e..bfa396d 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -164,9 +164,7 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginal(
     DCHECK(!cookie_store_path.empty());
 
     scoped_refptr<SQLitePersistentCookieStore> cookie_db =
-        new SQLitePersistentCookieStore(
-            cookie_store_path,
-            g_browser_process->db_thread()->message_loop());
+        new SQLitePersistentCookieStore(cookie_store_path);
     context->cookie_store_ = new net::CookieMonster(cookie_db.get());
   }
 
@@ -193,9 +191,7 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOriginalForExtensions(
   DCHECK(!cookie_store_path.empty());
 
   scoped_refptr<SQLitePersistentCookieStore> cookie_db =
-      new SQLitePersistentCookieStore(
-          cookie_store_path,
-          g_browser_process->db_thread()->message_loop());
+      new SQLitePersistentCookieStore(cookie_store_path);
   net::CookieMonster* cookie_monster = new net::CookieMonster(cookie_db.get());
 
   // Enable cookies for extension URLs only.
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index 7f19610..2d00455 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -14,10 +14,56 @@
 #include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/thread.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 
 using base::Time;
 
+namespace {
+
+// The UI thread can shut down the DB thread. Thus when we want to post a task
+// to the DB thread from the IO thread we must first pipe through the UI thread
+// to make sure the DB thread still exists. This class handles that for us. If
+// the DB thread does not still exist, then |callback_task_| will be dropped.
+class UIProxyForDBTask : public Task {
+ public:
+  explicit UIProxyForDBTask(Task* callback_task)
+      : callback_task_(callback_task) {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+  }
+
+  virtual ~UIProxyForDBTask() {
+  }
+
+  void Post() {
+    ChromeThread::GetMessageLoop(ChromeThread::UI)->PostTask(FROM_HERE, this);
+  }
+
+  void PostDelayed(int delay_ms) {
+    ChromeThread::GetMessageLoop(ChromeThread::UI)->
+        PostDelayedTask(FROM_HERE, this, delay_ms);
+  }
+
+  virtual void Run() {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+
+    base::Thread* db_thread = g_browser_process->db_thread();
+    if (db_thread)  // db_thread has not been torn down yet.
+      db_thread->message_loop()->PostTask(FROM_HERE, callback_task_);
+    else
+      delete callback_task_;
+
+    callback_task_ = NULL;
+  }
+
+ private:
+  // The task to be posted on the DB thread.
+  Task* callback_task_;
+};
+
+}  // namespace
+
 // This class is designed to be shared between any calling threads and the
 // database thread.  It batches operations and commits them on a timer.
 class SQLitePersistentCookieStore::Backend
@@ -25,9 +71,8 @@ class SQLitePersistentCookieStore::Backend
  public:
   // The passed database pointer must be already-initialized. This object will
   // take ownership.
-  explicit Backend(sql::Connection* db, MessageLoop* loop)
+  explicit Backend(sql::Connection* db)
       : db_(db),
-        background_loop_(loop),
         num_pending_(0) {
     DCHECK(db_) << "Database must exist.";
   }
@@ -87,7 +132,6 @@ class SQLitePersistentCookieStore::Backend
   void InternalBackgroundClose();
 
   sql::Connection* db_;
-  MessageLoop* background_loop_;
 
   typedef std::list<PendingOperation*> PendingOperationsList;
   PendingOperationsList pending_;
@@ -121,7 +165,7 @@ void SQLitePersistentCookieStore::Backend::BatchOperation(
   static const int kCommitIntervalMs = 30 * 1000;
   // Commit right away if we have more than 512 outstanding operations.
   static const size_t kCommitAfterBatchSize = 512;
-  DCHECK(MessageLoop::current() != background_loop_);
+  DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::DB));
 
   // We do a full copy of the cookie here, and hopefully just here.
   scoped_ptr<PendingOperation> po(new PendingOperation(op, key, cc));
@@ -134,20 +178,21 @@ void SQLitePersistentCookieStore::Backend::BatchOperation(
     num_pending = ++num_pending_;
   }
 
-  // TODO(abarth): What if the DB thread is being destroyed on the UI thread?
   if (num_pending == 1) {
     // We've gotten our first entry for this batch, fire off the timer.
-    background_loop_->PostDelayedTask(FROM_HERE,
-        NewRunnableMethod(this, &Backend::Commit), kCommitIntervalMs);
+    UIProxyForDBTask* proxy =
+        new UIProxyForDBTask(NewRunnableMethod(this, &Backend::Commit));
+    proxy->PostDelayed(kCommitIntervalMs);
   } else if (num_pending == kCommitAfterBatchSize) {
     // We've reached a big enough batch, fire off a commit now.
-    background_loop_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &Backend::Commit));
+    UIProxyForDBTask* proxy =
+        new UIProxyForDBTask(NewRunnableMethod(this, &Backend::Commit));
+    proxy->Post();
   }
 }
 
 void SQLitePersistentCookieStore::Backend::Commit() {
-  DCHECK(MessageLoop::current() == background_loop_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   PendingOperationsList ops;
   {
     AutoLock locked(pending_lock_);
@@ -236,15 +281,15 @@ void SQLitePersistentCookieStore::Backend::Commit() {
 // pending commit timer that will be holding a reference on us, but if/when
 // this fires we will already have been cleaned up and it will be ignored.
 void SQLitePersistentCookieStore::Backend::Close() {
-  DCHECK(MessageLoop::current() != background_loop_);
+  DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::DB));
   // Must close the backend on the background thread.
-  // TODO(abarth): What if the DB thread is being destroyed on the UI thread?
-  background_loop_->PostTask(FROM_HERE,
+  UIProxyForDBTask* task = new UIProxyForDBTask(
       NewRunnableMethod(this, &Backend::InternalBackgroundClose));
+  task->Post();
 }
 
 void SQLitePersistentCookieStore::Backend::InternalBackgroundClose() {
-  DCHECK(MessageLoop::current() == background_loop_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   // Commit any pending operations
   Commit();
 
@@ -252,12 +297,8 @@ void SQLitePersistentCookieStore::Backend::InternalBackgroundClose() {
   db_ = NULL;
 }
 
-SQLitePersistentCookieStore::SQLitePersistentCookieStore(
-    const FilePath& path,
-    MessageLoop* background_loop)
-    : path_(path),
-      background_loop_(background_loop) {
-  DCHECK(background_loop) << "SQLitePersistentCookieStore needs a MessageLoop";
+SQLitePersistentCookieStore::SQLitePersistentCookieStore(const FilePath& path)
+    : path_(path) {
 }
 
 SQLitePersistentCookieStore::~SQLitePersistentCookieStore() {
@@ -353,7 +394,7 @@ bool SQLitePersistentCookieStore::Load(
   }
 
   // Create the backend, this will take ownership of the db pointer.
-  backend_ = new Backend(db.release(), background_loop_);
+  backend_ = new Backend(db.release());
   return true;
 }
 
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.h b/chrome/browser/net/sqlite_persistent_cookie_store.h
index fe7e3f0..8b82a16 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.h
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.h
@@ -22,8 +22,7 @@ class MessageLoop;
 class SQLitePersistentCookieStore
     : public net::CookieMonster::PersistentCookieStore {
  public:
-  SQLitePersistentCookieStore(const FilePath& path,
-                              MessageLoop* background_loop);
+  SQLitePersistentCookieStore(const FilePath& path);
   ~SQLitePersistentCookieStore();
 
   virtual bool Load(std::vector<net::CookieMonster::KeyedCanonicalCookie>*);
@@ -43,9 +42,6 @@ class SQLitePersistentCookieStore
   FilePath path_;
   scoped_refptr<Backend> backend_;
 
-  // Background MessageLoop on which to access the backend_;
-  MessageLoop* background_loop_;
-
   sql::MetaTable meta_table_;
 
   DISALLOW_COPY_AND_ASSIGN(SQLitePersistentCookieStore);