Don't force non-session only cookies to be session only cookies, instead delete on shutdown

chrome/browser/net/sqlite_persistent_cookie_store.cc

Index: chrome/browser/net/sqlite_persistent_cookie_store.cc
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index 9da57675666f0d06c5ed5a0c3a4f3fa1bee82c38..dc6c94c9511de0c4d5d541ca41c3ea6e6ffb7b01 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -25,11 +25,13 @@
 #include "base/time.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/common/url_constants.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/registry_controlled_domain.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
+#include "webkit/quota/special_storage_policy.h"
 
 using base::Time;
 using content::BrowserThread;
@@ -59,13 +61,16 @@ using content::BrowserThread;
 class SQLitePersistentCookieStore::Backend
     : public base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend> {
  public:
-  Backend(const FilePath& path, bool restore_old_session_cookies)
+  Backend(const FilePath& path,
+          bool restore_old_session_cookies,
+          quota::SpecialStoragePolicy* special_storage_policy)
       : path_(path),
         db_(NULL),
         num_pending_(0),
         clear_local_state_on_exit_(false),
         initialized_(false),
         restore_old_session_cookies_(restore_old_session_cookies),
+        special_storage_policy_(special_storage_policy),
         num_cookies_read_(0),
         num_priority_waiting_(0),
         total_priority_requests_(0) {
@@ -181,7 +186,9 @@ class SQLitePersistentCookieStore::Backend
   // Close() executed on the background thread.
   void InternalBackgroundClose();
 
-  void DeleteSessionCookies();
+  void DeleteSessionCookiesOnStartup();
+
+  void DeleteSessionCookiesOnShutdown();
 
   FilePath path_;
   scoped_ptr<sql::Connection> db_;
@@ -192,7 +199,8 @@ class SQLitePersistentCookieStore::Backend
   PendingOperationsList::size_type num_pending_;
   // True if the persistent store should be deleted upon destruction.
   bool clear_local_state_on_exit_;
-  // Guard |cookies_|, |pending_|, |num_pending_|, |clear_local_state_on_exit_|
+  // Guard |cookies_|, |pending_|, |num_pending_|, |clear_local_state_on_exit_|,
+  // and |cookies_per_domain_|.
   base::Lock lock_;
 
   // Temporary buffer for cookies loaded from DB. Accumulates cookies to reduce
@@ -203,12 +211,18 @@ class SQLitePersistentCookieStore::Backend
   // 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_;
 
+  // Map of domain keys(eTLD+1) to number of cookies in the database.
+  std::map<std::string, int> cookies_per_domain_;
+
   // Indicates if DB has been initialized.
   bool initialized_;
 
   // If false, we should filter out session cookies when reading the DB.
   bool restore_old_session_cookies_;
 
+  // Storage Policy defining what data is deleted on shutdown.
+  scoped_refptr<quota::SpecialStoragePolicy> special_storage_policy_;
+
   // The cumulative time spent loading the cookies on the DB thread. Incremented
   // and reported from the DB thread.
   base::TimeDelta cookie_load_duration_;
@@ -565,7 +579,7 @@ void SQLitePersistentCookieStore::Backend::ChainLoadCookies(
       base::Bind(&SQLitePersistentCookieStore::Backend::CompleteLoadOnIOThread,
                  this, loaded_callback, load_success));
     if (load_success && !restore_old_session_cookies_)
-      DeleteSessionCookies();
+      DeleteSessionCookiesOnStartup();
   }
 }
 
@@ -595,9 +609,11 @@ bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
 
   std::vector<net::CookieMonster::CanonicalCookie*> cookies;
   std::set<std::string>::const_iterator it = domains.begin();
+  std::map<std::string, int> cookies_per_domain;
   for (; it != domains.end(); ++it) {
     smt.BindString(0, *it);
     while (smt.Step()) {
+      cookies_per_domain[*it]++;
       scoped_ptr<net::CookieMonster::CanonicalCookie> cc(
           new net::CookieMonster::CanonicalCookie(
               // The "source" URL is not used with persisted cookies.
@@ -625,6 +641,11 @@ bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
   {
     base::AutoLock locked(lock_);
     cookies_.insert(cookies_.end(), cookies.begin(), cookies.end());
+    std::map<std::string, int>::iterator domain;
+    for (domain = cookies_per_domain.begin();
+         domain != cookies_per_domain.end(); ++domain) {
+      cookies_per_domain_[domain->first] += domain->second;
+    }
   }
   return true;
 }
@@ -820,12 +841,14 @@ void SQLitePersistentCookieStore::Backend::Commit() {
   if (!transaction.Begin())
     return;
 
+  std::map<std::string, int> cookies_per_domain;
   for (PendingOperationsList::iterator it = ops.begin();
        it != ops.end(); ++it) {
     // Free the cookies as we commit them to the database.
     scoped_ptr<PendingOperation> po(*it);
     switch (po->op()) {
       case PendingOperation::COOKIE_ADD:
+        cookies_per_domain[po->cc().Domain()]++;
         add_smt.Reset(true);
         add_smt.BindInt64(0, po->cc().CreationDate().ToInternalValue());
         add_smt.BindString(1, po->cc().Domain());
@@ -853,6 +876,7 @@ void SQLitePersistentCookieStore::Backend::Commit() {
         break;
 
       case PendingOperation::COOKIE_DELETE:
+        cookies_per_domain[po->cc().Domain()]--;
         del_smt.Reset(true);
         del_smt.BindInt64(0, po->cc().CreationDate().ToInternalValue());
         if (!del_smt.Run())
@@ -867,6 +891,14 @@ void SQLitePersistentCookieStore::Backend::Commit() {
   bool succeeded = transaction.Commit();
   UMA_HISTOGRAM_ENUMERATION("Cookie.BackingStoreUpdateResults",
                             succeeded ? 0 : 1, 2);
+  if (succeeded) {
+    base::AutoLock locked(lock_);
+    std::map<std::string, int>::iterator domain;
+    for (domain = cookies_per_domain.begin();
+         domain != cookies_per_domain.end(); ++domain) {
+      cookies_per_domain_[domain->first] += domain->second;
+    }
+  }
 }
 
 void SQLitePersistentCookieStore::Backend::Flush(
@@ -901,19 +933,70 @@ void SQLitePersistentCookieStore::Backend::InternalBackgroundClose() {
   // Commit any pending operations
   Commit();
 
+  if (!clear_local_state_on_exit_ && special_storage_policy_.get() &&
+      special_storage_policy_->HasSessionOnlyOrigins()) {
+    DeleteSessionCookiesOnShutdown();
+  }
+
   db_.reset();
 
   if (clear_local_state_on_exit_)
     file_util::Delete(path_, false);
 }
 
+void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnShutdown() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+
+
+  sql::Statement del_smt(db_->GetCachedStatement(SQL_FROM_HERE,
+                         "DELETE FROM cookies WHERE host_key=?"));
+  if (!del_smt.is_valid()) {
+    LOG(WARNING) << "Unable to delete cookies on shutdown.";
+    return;
+  }
+
+  sql::Transaction transaction(db_.get());
+  if (!transaction.Begin()) {
+    LOG(WARNING) << "Unable to delete cookies on shutdown.";
+    return;
+  }
+
+  base::AutoLock locked(lock_);
+  std::map<std::string, int>::iterator domain;
+  for (domain = cookies_per_domain_.begin();
+       domain != cookies_per_domain_.end(); ++domain) {
+    if (domain->second <= 0) {
+      DCHECK_EQ(0, domain->second);
+      continue;
+    }
+
+    GURL url(

[jochen (gone - plz use gerrit), 2012/05/24 11:04:03]

compare to CookiesTreeModel::PopulateCookieInfoWithFilter

[erikwright (departed), 2012/05/24 17:36:59]

Presumably this should be matching whoever defines the SpecialStoragePolicy. Can
you provide a reference to that?

I don't like the way this is coupled to whatever code is defining the policy.

I see that the SpecialStoragePolicy is URL-oriented. I think it makes sense to
define an interface (living somewhere in chrome/browser/ I think) that is
domain-oriented. The implementation of that interface can do the conversion to a
GURL and delegate to a SpecialStoragePolicy if that is what makes sense.

If you want to discuss this further, feel free to schedule a VC in my Friday AM.

+        std::string(chrome::kHttpScheme) +
+        content::kStandardSchemeSeparator +
+        (domain->first[0] == '.' ? domain->first.substr(1) : domain->first) +
+        "/");
+    if (!special_storage_policy_->IsStorageSessionOnly(url))
+      continue;
+    if (special_storage_policy_->IsStorageProtected(url))
+      continue;
+
+    del_smt.Reset(true);
+    del_smt.BindString(0, domain->first);
+    if (!del_smt.Run())
+      NOTREACHED() << "Could not delete a cookie from the DB.";
+  }
+
+  if (!transaction.Commit())
+    LOG(WARNING) << "Unable to delete cookies on shutdown.";
+}
+
 void SQLitePersistentCookieStore::Backend::SetClearLocalStateOnExit(
     bool clear_local_state) {
   base::AutoLock locked(lock_);
   clear_local_state_on_exit_ = clear_local_state;
 }
 
-void SQLitePersistentCookieStore::Backend::DeleteSessionCookies() {
+void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnStartup() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   if (!db_->Execute("DELETE FROM cookies WHERE persistent == 0"))
     LOG(WARNING) << "Unable to delete session cookies.";
@@ -921,8 +1004,9 @@ void SQLitePersistentCookieStore::Backend::DeleteSessionCookies() {
 
 SQLitePersistentCookieStore::SQLitePersistentCookieStore(
     const FilePath& path,
-    bool restore_old_session_cookies)
-    : backend_(new Backend(path, restore_old_session_cookies)) {
+    bool restore_old_session_cookies,
+    quota::SpecialStoragePolicy* storage_policy)
+    : backend_(new Backend(path, restore_old_session_cookies, storage_policy)) {
 }
 
 void SQLitePersistentCookieStore::Load(const LoadedCallback& loaded_callback) {