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

10 files changed, 188 insertions, 22 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',
diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index fdfd779..1a987fe 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -406,6 +406,11 @@ void CookieMonster::SetExpiryAndKeyScheme(ExpiryAndKeyScheme key_scheme) {
   expiry_and_key_scheme_ = key_scheme;
 }
 
+void CookieMonster::SetClearPersistentStoreOnExit(bool clear_local_store) {
+  if(store_)
+    store_->SetClearLocalStateOnExit(clear_local_store);
+}
+
 // The system resolution is not high enough, so we can have multiple
 // set cookies that result in the same system time.  When this happens, we
 // increment by one Time unit.  Let's hope computers don't get too fast.
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index 44bd510..0b11130 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -207,6 +207,10 @@ class CookieMonster : public CookieStore {
   // function must be called before initialization.
   void SetExpiryAndKeyScheme(ExpiryAndKeyScheme key_scheme);
 
+  // Delegates the call to set the |clear_local_store_on_exit_| flag of the
+  // PersistentStore if it exists.
+  void SetClearPersistentStoreOnExit(bool clear_local_store);
+
   // There are some unknowns about how to correctly handle file:// cookies,
   // and our implementation for this is not robust enough. This allows you
   // to enable support, but it should only be used for testing. Bug 1157243.
@@ -677,18 +681,22 @@ typedef base::RefCountedThreadSafe<CookieMonster::PersistentCookieStore>
 class CookieMonster::PersistentCookieStore
     : public RefcountedPersistentCookieStore {
  public:
-  virtual ~PersistentCookieStore() { }
+  virtual ~PersistentCookieStore() {}
 
   // Initializes the store and retrieves the existing cookies. This will be
   // called only once at startup.
-  virtual bool Load(std::vector<CookieMonster::CanonicalCookie*>*) = 0;
+  virtual bool Load(std::vector<CookieMonster::CanonicalCookie*>* cookies) = 0;
+
+  virtual void AddCookie(const CanonicalCookie& cc) = 0;
+  virtual void UpdateCookieAccessTime(const CanonicalCookie& cc) = 0;
+  virtual void DeleteCookie(const CanonicalCookie& cc) = 0;
 
-  virtual void AddCookie(const CanonicalCookie&) = 0;
-  virtual void UpdateCookieAccessTime(const CanonicalCookie&) = 0;
-  virtual void DeleteCookie(const CanonicalCookie&) = 0;
+  // Sets the value of the user preference whether the persistent storage
+  // must be deleted upon destruction.
+  virtual void SetClearLocalStateOnExit(bool clear_local_state) = 0;
 
  protected:
-  PersistentCookieStore() { }
+  PersistentCookieStore() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PersistentCookieStore);
diff --git a/net/base/cookie_monster_store_test.h b/net/base/cookie_monster_store_test.h
index 8a21930..65c91b7 100644
--- a/net/base/cookie_monster_store_test.h
+++ b/net/base/cookie_monster_store_test.h
@@ -66,6 +66,9 @@ class MockPersistentCookieStore
         CookieStoreCommand(CookieStoreCommand::REMOVE, cookie));
   }
 
+  // No files are created so nothing to clear either
+  virtual void SetClearLocalStateOnExit(bool clear_local_state) {}
+
   void SetLoadExpectation(
       bool return_value,
       const std::vector<net::CookieMonster::CanonicalCookie*>& result) {
@@ -185,6 +188,8 @@ class MockSimplePersistentCookieStore
     cookies_.erase(it);
   }
 
+  virtual void SetClearLocalStateOnExit(bool clear_local_state) {}
+
  private:
   CanonicalCookieMap cookies_;
 };