Refactored cookies persistent store clean-up on shutdown.

Changed the static call of SQLitePersistantCookieStore::ClearLocalState to
a call of a virtual method PersistantCookieStore::ClearLocalState from
inside the destruction sequence of the owning UrlRequestContext.

Thus the code will now work with any other persistancy implementation and
allow for better parallelism.

To test. Enable deleting of cookies and user data on shutdown and check if
the Cookies file in the profile directory has been deleted.

BUG=64920
TEST=Manual.

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

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

7 files changed, 164 insertions, 16 deletions

diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 652f7b90..e260184 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -35,7 +35,6 @@
 #include "chrome/browser/net/chrome_net_log.h"
 #include "chrome/browser/net/predictor_api.h"
 #include "chrome/browser/net/sdch_dictionary_fetcher.h"
-#include "chrome/browser/net/sqlite_persistent_cookie_store.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/plugin_service.h"
 #include "chrome/browser/plugin_updater.h"
@@ -504,8 +503,6 @@ bool BrowserProcessImpl::have_inspector_files() const {
 }
 
 void BrowserProcessImpl::ClearLocalState(const FilePath& profile_path) {
-  SQLitePersistentCookieStore::ClearLocalState(profile_path.Append(
-      chrome::kCookieFilename));
   webkit_database::DatabaseTracker::ClearLocalState(profile_path);
   ChromeAppCacheService::ClearLocalState(profile_path);
 }
diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 8aa95da..a551857 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -315,6 +315,7 @@ ChromeURLRequestContext* FactoryForOriginal::Create() {
 
     scoped_refptr<SQLitePersistentCookieStore> cookie_db =
         new SQLitePersistentCookieStore(cookie_store_path_);
+    cookie_db->SetClearLocalStateOnExit(clear_local_state_on_exit_);
     context->set_cookie_store(new net::CookieMonster(cookie_db.get(),
         cookie_monster_delegate_));
   }
@@ -705,6 +706,15 @@ void ChromeURLRequestContextGetter::Observe(
               this,
               &ChromeURLRequestContextGetter::OnDefaultCharsetChange,
               default_charset));
+    } else if (*pref_name_in == prefs::kClearSiteDataOnExit) {
+      bool clear_site_data =
+          prefs->GetBoolean(prefs::kClearSiteDataOnExit);
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          NewRunnableMethod(
+              this,
+              &ChromeURLRequestContextGetter::OnClearSiteDataOnExitChange,
+              clear_site_data));
     }
   } else {
     NOTREACHED();
@@ -717,6 +727,7 @@ void ChromeURLRequestContextGetter::RegisterPrefsObserver(Profile* profile) {
   registrar_.Init(profile->GetPrefs());
   registrar_.Add(prefs::kAcceptLanguages, this);
   registrar_.Add(prefs::kDefaultCharset, this);
+  registrar_.Add(prefs::kClearSiteDataOnExit, this);
 }
 
 // static
@@ -742,6 +753,12 @@ void ChromeURLRequestContextGetter::OnDefaultCharsetChange(
   GetIOContext()->OnDefaultCharsetChange(default_charset);
 }
 
+void ChromeURLRequestContextGetter::OnClearSiteDataOnExitChange(
+    bool clear_site_data) {
+  GetCookieStore()->GetCookieMonster()->
+      SetClearPersistentStoreOnExit(clear_site_data);
+}
+
 void ChromeURLRequestContextGetter::GetCookieStoreAsyncHelper(
     base::WaitableEvent* completion,
     net::CookieStore** result) {
@@ -882,6 +899,7 @@ ChromeURLRequestContextFactory::ChromeURLRequestContextFactory(Profile* profile)
   std::string default_charset = prefs->GetString(prefs::kDefaultCharset);
   accept_charset_ =
       net::HttpUtil::GenerateAcceptCharsetHeader(default_charset);
+  clear_local_state_on_exit_ = prefs->GetBoolean(prefs::kClearSiteDataOnExit);
 
   // At this point, we don't know the charset of the referring page
   // where a url request originates from. This is used to get a suggested
diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h
index e9faf59..ddc3550 100644
--- a/chrome/browser/net/chrome_url_request_context.h
+++ b/chrome/browser/net/chrome_url_request_context.h
@@ -310,6 +310,7 @@ class ChromeURLRequestContextGetter : public URLRequestContextGetter,
   // ChromeURLRequestContext.
   void OnAcceptLanguageChange(const std::string& accept_language);
   void OnDefaultCharsetChange(const std::string& default_charset);
+  void OnClearSiteDataOnExitChange(bool clear_site_data);
 
   // Saves the cookie store to |result| and signals |completion|.
   void GetCookieStoreAsyncHelper(base::WaitableEvent* completion,
@@ -362,6 +363,7 @@ class ChromeURLRequestContextFactory {
   // ApplyProfileParametersToContext().
   bool is_media_;
   bool is_off_the_record_;
+  bool clear_local_state_on_exit_;
   std::string accept_language_;
   std::string accept_charset_;
   std::string referrer_charset_;
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index 88b7bb9..6a77e7f 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -31,7 +31,8 @@ class SQLitePersistentCookieStore::Backend
   explicit Backend(const FilePath& path)
       : path_(path),
         db_(NULL),
-        num_pending_(0) {
+        num_pending_(0),
+        clear_local_state_on_exit_(false) {
   }
 
   // Creates or load the SQLite database.
@@ -50,6 +51,8 @@ class SQLitePersistentCookieStore::Backend
   // before the object is destructed.
   void Close();
 
+  void SetClearLocalStateOnExit(bool clear_local_state);
+
  private:
   friend class base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend>;
 
@@ -98,7 +101,10 @@ class SQLitePersistentCookieStore::Backend
   typedef std::list<PendingOperation*> PendingOperationsList;
   PendingOperationsList pending_;
   PendingOperationsList::size_type num_pending_;
-  Lock pending_lock_;  // Guard pending_ and 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_|.
+  Lock lock_;
 
   DISALLOW_COPY_AND_ASSIGN(Backend);
 };
@@ -297,7 +303,7 @@ void SQLitePersistentCookieStore::Backend::BatchOperation(
 
   PendingOperationsList::size_type num_pending;
   {
-    AutoLock locked(pending_lock_);
+    AutoLock locked(lock_);
     pending_.push_back(po.release());
     num_pending = ++num_pending_;
   }
@@ -319,7 +325,7 @@ void SQLitePersistentCookieStore::Backend::Commit() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   PendingOperationsList ops;
   {
-    AutoLock locked(pending_lock_);
+    AutoLock locked(lock_);
     pending_.swap(ops);
     num_pending_ = 0;
   }
@@ -420,8 +426,16 @@ void SQLitePersistentCookieStore::Backend::InternalBackgroundClose() {
   Commit();
 
   db_.reset();
+
+  if (clear_local_state_on_exit_)
+    file_util::Delete(path_, false);
 }
 
+void SQLitePersistentCookieStore::Backend::SetClearLocalStateOnExit(
+    bool clear_local_state) {
+  AutoLock locked(lock_);
+  clear_local_state_on_exit_ = clear_local_state;
+}
 SQLitePersistentCookieStore::SQLitePersistentCookieStore(const FilePath& path)
     : backend_(new Backend(path)) {
 }
@@ -458,8 +472,8 @@ void SQLitePersistentCookieStore::DeleteCookie(
     backend_->DeleteCookie(cc);
 }
 
-// static
-void SQLitePersistentCookieStore::ClearLocalState(
-    const FilePath& path) {
-  file_util::Delete(path, false);
+void SQLitePersistentCookieStore::SetClearLocalStateOnExit(
+    bool clear_local_state) {
+  if (backend_.get())
+    backend_->SetClearLocalStateOnExit(clear_local_state);
 }
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.h b/chrome/browser/net/sqlite_persistent_cookie_store.h
index 4fcc031..05cb550 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.h
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.h
@@ -16,20 +16,23 @@
 
 class FilePath;
 
+// Implements the PersistentCookieStore interface in terms of a SQLite database.
+// For documentation about the actual member functions consult the documentation
+// of the parent class |net::CookieMonster::PersistentCookieStore|.
 class SQLitePersistentCookieStore
     : public net::CookieMonster::PersistentCookieStore {
  public:
   explicit SQLitePersistentCookieStore(const FilePath& path);
   virtual ~SQLitePersistentCookieStore();
 
-  virtual bool Load(std::vector<net::CookieMonster::CanonicalCookie*>*);
+  virtual bool Load(std::vector<net::CookieMonster::CanonicalCookie*>* cookies);
 
-  virtual void AddCookie(const net::CookieMonster::CanonicalCookie&);
+  virtual void AddCookie(const net::CookieMonster::CanonicalCookie& cc);
   virtual void UpdateCookieAccessTime(
-      const net::CookieMonster::CanonicalCookie&);
-  virtual void DeleteCookie(const net::CookieMonster::CanonicalCookie&);
+      const net::CookieMonster::CanonicalCookie& cc);
+  virtual void DeleteCookie(const net::CookieMonster::CanonicalCookie& cc);
 
-  static void ClearLocalState(const FilePath& path);
+  virtual void SetClearLocalStateOnExit(bool clear_local_state);
 
  private:
   class Backend;
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc b/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
new file mode 100644
index 0000000..d1b056e
--- /dev/null
+++ b/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
@@ -0,0 +1,113 @@
+// Copyright (c) 2010 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 "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/ref_counted.h"
+#include "base/scoped_temp_dir.h"
+#include "base/stl_util-inl.h"
+#include "base/time.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/net/sqlite_persistent_cookie_store.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/test/thread_test_helper.h"
+#include "googleurl/src/gurl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class SQLitePersistentCookieStoreTest : public testing::Test {
+ public:
+  SQLitePersistentCookieStoreTest()
+      : ui_thread_(BrowserThread::UI),
+        db_thread_(BrowserThread::DB) {
+  }
+
+ protected:
+  virtual void SetUp() {
+    ui_thread_.Start();
+    db_thread_.Start();
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    store_ = new SQLitePersistentCookieStore(
+        temp_dir_.path().Append(chrome::kCookieFilename));
+    std::vector<net::CookieMonster::CanonicalCookie*> cookies;
+    ASSERT_TRUE(store_->Load(&cookies));
+    ASSERT_TRUE(0 == cookies.size());
+    // Make sure the store gets written at least once.
+    store_->AddCookie(
+        net::CookieMonster::CanonicalCookie("A", "B", "http://foo.bar", "/",
+                                            false, false,
+                                            base::Time::Now(),
+                                            base::Time::Now(),
+                                            true, base::Time::Now()));
+  }
+
+  BrowserThread ui_thread_;
+  BrowserThread db_thread_;
+  scoped_refptr<SQLitePersistentCookieStore> store_;
+  ScopedTempDir temp_dir_;
+};
+
+TEST_F(SQLitePersistentCookieStoreTest, KeepOnDestruction) {
+  store_->SetClearLocalStateOnExit(false);
+  store_ = NULL;
+  // Make sure we wait until the destructor has run.
+  scoped_refptr<ThreadTestHelper> helper(
+      new ThreadTestHelper(BrowserThread::DB));
+  ASSERT_TRUE(helper->Run());
+
+  ASSERT_TRUE(file_util::PathExists(
+      temp_dir_.path().Append(chrome::kCookieFilename)));
+  ASSERT_TRUE(file_util::Delete(
+      temp_dir_.path().Append(chrome::kCookieFilename), false));
+}
+
+TEST_F(SQLitePersistentCookieStoreTest, RemoveOnDestruction) {
+  store_->SetClearLocalStateOnExit(true);
+  // Replace the store effectively destroying the current one and forcing it
+  // to write it's data to disk. Then we can see if after loading it again it
+  // is still there.
+  store_ = NULL;
+  // Make sure we wait until the destructor has run.
+  scoped_refptr<ThreadTestHelper> helper(
+      new ThreadTestHelper(BrowserThread::DB));
+  ASSERT_TRUE(helper->Run());
+
+  ASSERT_FALSE(file_util::PathExists(
+      temp_dir_.path().Append(chrome::kCookieFilename)));
+}
+
+// Test if data is stored as expected in the SQLite database.
+TEST_F(SQLitePersistentCookieStoreTest, TestPersistance) {
+  std::vector<net::CookieMonster::CanonicalCookie*> cookies;
+  // Replace the store effectively destroying the current one and forcing it
+  // to write it's data to disk. Then we can see if after loading it again it
+  // is still there.
+  store_ = NULL;
+  scoped_refptr<ThreadTestHelper> helper(
+      new ThreadTestHelper(BrowserThread::DB));
+  // Make sure we wait until the destructor has run.
+  ASSERT_TRUE(helper->Run());
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+
+  // Reload and test for persistence
+  ASSERT_TRUE(store_->Load(&cookies));
+  ASSERT_EQ(1U, cookies.size());
+  ASSERT_STREQ("http://foo.bar", cookies[0]->Domain().c_str());
+  ASSERT_STREQ("A", cookies[0]->Name().c_str());
+  ASSERT_STREQ("B", cookies[0]->Value().c_str());
+
+  // Now delete the cookie and check persistence again.
+  store_->DeleteCookie(*cookies[0]);
+  store_ = NULL;
+  // Make sure we wait until the destructor has run.
+  ASSERT_TRUE(helper->Run());
+  STLDeleteContainerPointers(cookies.begin(), cookies.end());
+  cookies.clear();
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+
+  // Reload and check if the cookie has been removed.
+  ASSERT_TRUE(store_->Load(&cookies));
+  ASSERT_EQ(0U, cookies.size());
+}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 815006f..c343c79 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1234,6 +1234,7 @@
         'browser/net/pref_proxy_config_service_unittest.cc',
         'browser/net/prerender_interceptor_unittest.cc',
         'browser/net/resolve_proxy_msg_helper_unittest.cc',
+        'browser/net/sqlite_persistent_cookie_store_unittest.cc',
         'browser/net/url_fixer_upper_unittest.cc',
         'browser/net/url_info_unittest.cc',
         'browser/notifications/desktop_notification_service_unittest.cc',