Remove QuotaPolicyCookieStore.

It's always created[1] with null SpecialStoragePolicy, so it doesn't actually do any cleanup, but its base class still does some bookkeeping, wasting memory. The actual cleanup for CONTENT_SETTING_SESSION_ONLY is done by SessionCleanupCookieStore in the network service process with help from its CookieSettings.

[1] For the chrome-extension:// cookie jar and for Android Webview when it creates the cookie jar itself.

Bug: 488710, 934009
Change-Id: I5bc34551e92e50e9cb799f99c804c24895c290a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906642Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738948}

android_webview/browser/cookie_manager.cc

@@ -306,7 +306,7 @@ net::CookieStore* CookieManager::GetCookieStore() {
   if (!cookie_store_) {
     content::CookieStoreConfig cookie_config(
         cookie_store_path_, true /* restore_old_session_cookies */,
-        true /* persist_session_cookies */, nullptr /* storage_policy */);
+        true /* persist_session_cookies */);
     cookie_config.client_task_runner = cookie_store_task_runner_;
     cookie_config.background_task_runner =
         cookie_store_backend_thread_.task_runner();

chrome/browser/extensions/chrome_extension_cookies.cc

@@ -36,7 +36,7 @@ ChromeExtensionCookies::ChromeExtensionCookies(Profile* profile)
     creation_config = std::make_unique<content::CookieStoreConfig>(
         profile_->GetPath().Append(chrome::kExtensionsCookieFilename),
         profile_->ShouldRestoreOldSessionCookies(),
-        profile_->ShouldPersistSessionCookies(), nullptr /* storage_policy */);
+        profile_->ShouldPersistSessionCookies());
     creation_config->crypto_delegate = cookie_config::GetCookieCryptoDelegate();
   }
   creation_config->cookieable_schemes.push_back(extensions::kExtensionScheme);

content/browser/BUILD.gn

@@ -1243,12 +1243,11 @@ jumbo_source_set("browser") {
     "navigation_subresource_loader_params.h",
     "net/browser_online_state_observer.cc",
     "net/browser_online_state_observer.h",
+    "net/cookie_store_factory.cc",
     "net/network_errors_listing_ui.cc",
     "net/network_errors_listing_ui.h",
     "net/network_quality_observer_impl.cc",
     "net/network_quality_observer_impl.h",
-    "net/quota_policy_cookie_store.cc",
-    "net/quota_policy_cookie_store.h",
     "network_context_client_base_impl.cc",
     "network_context_client_base_impl.h",
     "network_service_client.cc",

content/browser/net/quota_policy_cookie_store.cc → content/browser/net/cookie_store_factory.cc

@@ -2,45 +2,23 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/net/quota_policy_cookie_store.h"
+#include "content/public/browser/cookie_store_factory.h"
 
-#include <list>
 #include <memory>
 
-#include "base/bind.h"
-#include "base/callback.h"
 #include "base/files/file_path.h"
-#include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequenced_task_runner.h"
 #include "base/task/post_task.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/cookie_store_factory.h"
-#include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_constants.h"
-#include "net/cookies/cookie_util.h"
+#include "net/cookies/cookie_monster.h"
 #include "net/extras/sqlite/cookie_crypto_delegate.h"
-#include "storage/browser/quota/special_storage_policy.h"
+#include "net/extras/sqlite/sqlite_persistent_cookie_store.h"
 
 namespace content {
 
-QuotaPolicyCookieStore::QuotaPolicyCookieStore(
-    const scoped_refptr<net::SQLitePersistentCookieStore>& cookie_store,
-    storage::SpecialStoragePolicy* special_storage_policy)
-    : SessionCleanupCookieStore(cookie_store),
-      special_storage_policy_(special_storage_policy) {}
-
-QuotaPolicyCookieStore::~QuotaPolicyCookieStore() {
-  if (!special_storage_policy_.get() ||
-      !special_storage_policy_->HasSessionOnlyOrigins()) {
-    return;
-  }
-
-  DeleteSessionCookies(
-      special_storage_policy_->CreateDeleteCookieOnExitPredicate());
-}
-
 CookieStoreConfig::CookieStoreConfig()
     : restore_old_session_cookies(false),
       persist_session_cookies(false),
@@ -48,22 +26,18 @@ CookieStoreConfig::CookieStoreConfig()
   // Default to an in-memory cookie store.
 }
 
-CookieStoreConfig::CookieStoreConfig(
-    const base::FilePath& path,
-    bool restore_old_session_cookies,
-    bool persist_session_cookies,
-    storage::SpecialStoragePolicy* storage_policy)
+CookieStoreConfig::CookieStoreConfig(const base::FilePath& path,
+                                     bool restore_old_session_cookies,
+                                     bool persist_session_cookies)
     : path(path),
       restore_old_session_cookies(restore_old_session_cookies),
       persist_session_cookies(persist_session_cookies),
-      storage_policy(storage_policy),
       crypto_delegate(nullptr) {
   CHECK(!path.empty() ||
         (!restore_old_session_cookies && !persist_session_cookies));
 }
 
-CookieStoreConfig::~CookieStoreConfig() {
-}
+CookieStoreConfig::~CookieStoreConfig() {}
 
 std::unique_ptr<net::CookieStore> CreateCookieStore(
     const CookieStoreConfig& config,
@@ -97,13 +71,8 @@ std::unique_ptr<net::CookieStore> CreateCookieStore(
             config.path, client_task_runner, background_task_runner,
             config.restore_old_session_cookies, config.crypto_delegate));
 
-    QuotaPolicyCookieStore* persistent_store =
-        new QuotaPolicyCookieStore(
-            sqlite_store.get(),
-            config.storage_policy.get());
-
     cookie_monster =
-        std::make_unique<net::CookieMonster>(persistent_store, net_log);
+        std::make_unique<net::CookieMonster>(std::move(sqlite_store), net_log);
     if (config.persist_session_cookies)
       cookie_monster->SetPersistSessionCookies(true);
   }

content/browser/net/quota_policy_cookie_store.h

-// Copyright 2015 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.
-
-#ifndef CONTENT_BROWSER_NET_QUOTA_POLICY_COOKIE_STORE_H_
-#define CONTENT_BROWSER_NET_QUOTA_POLICY_COOKIE_STORE_H_
-
-#include <stddef.h>
-
-#include <string>
-#include <vector>
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "content/common/content_export.h"
-#include "net/extras/sqlite/sqlite_persistent_cookie_store.h"
-#include "services/network/session_cleanup_cookie_store.h"
-
-namespace storage {
-class SpecialStoragePolicy;
-}  // namespace storage
-
-namespace content {
-
-// Implements a PersistentCookieStore that deletes session cookies on
-// shutdown. For documentation about the actual member functions consult the
-// parent class |net::CookieMonster::PersistentCookieStore|. If provided, a
-// |SpecialStoragePolicy| is consulted when the SQLite database is closed to
-// decide which cookies to keep.
-class CONTENT_EXPORT QuotaPolicyCookieStore
-    : public network::SessionCleanupCookieStore {
- public:
-  // Wraps the passed-in |cookie_store|.
-  QuotaPolicyCookieStore(
-      const scoped_refptr<net::SQLitePersistentCookieStore>& cookie_store,
-      storage::SpecialStoragePolicy* special_storage_policy);
-
- private:
-  ~QuotaPolicyCookieStore() override;
-
-  scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy_;
-
-  DISALLOW_COPY_AND_ASSIGN(QuotaPolicyCookieStore);
-};
-
-}  // namespace content
-
-#endif  // CONTENT_BROWSER_NET_QUOTA_POLICY_COOKIE_STORE_H_

content/browser/net/quota_policy_cookie_store_unittest.cc

-// Copyright 2015 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 "content/browser/net/quota_policy_cookie_store.h"
-
-#include "base/bind.h"
-#include "base/files/file_util.h"
-#include "base/files/scoped_temp_dir.h"
-#include "base/memory/ref_counted.h"
-#include "base/run_loop.h"
-#include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/task/post_task.h"
-#include "base/task/thread_pool/thread_pool_instance.h"
-#include "base/time/time.h"
-#include "content/public/test/browser_task_environment.h"
-#include "net/cookies/cookie_util.h"
-#include "net/log/net_log_with_source.h"
-#include "net/ssl/ssl_client_cert_type.h"
-#include "net/test/cert_test_util.h"
-#include "net/test/test_data_directory.h"
-#include "sql/statement.h"
-#include "storage/browser/test/mock_special_storage_policy.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
-
-namespace {
-const base::FilePath::CharType kTestCookiesFilename[] =
-    FILE_PATH_LITERAL("Cookies");
-}
-
-namespace content {
-namespace {
-
-using CanonicalCookieVector =
-    std::vector<std::unique_ptr<net::CanonicalCookie>>;
-
-class QuotaPolicyCookieStoreTest : public testing::Test {
- public:
-  QuotaPolicyCookieStoreTest()
-      : loaded_event_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
-                      base::WaitableEvent::InitialState::NOT_SIGNALED),
-        destroy_event_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
-                       base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-
-  void OnLoaded(CanonicalCookieVector cookies) {
-    cookies_.swap(cookies);
-    loaded_event_.Signal();
-  }
-
-  void Load(CanonicalCookieVector* cookies) {
-    EXPECT_FALSE(loaded_event_.IsSignaled());
-    store_->Load(base::BindOnce(&QuotaPolicyCookieStoreTest::OnLoaded,
-                                base::Unretained(this)),
-                 net::NetLogWithSource());
-    loaded_event_.Wait();
-    cookies->swap(cookies_);
-  }
-
-  void ReleaseStore() {
-    EXPECT_TRUE(background_task_runner_->RunsTasksInCurrentSequence());
-    store_ = nullptr;
-    destroy_event_.Signal();
-  }
-
-  void DestroyStoreOnBackgroundThread() {
-    background_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&QuotaPolicyCookieStoreTest::ReleaseStore,
-                                  base::Unretained(this)));
-    destroy_event_.Wait();
-    DestroyStore();
-  }
-
- protected:
-  void CreateAndLoad(storage::SpecialStoragePolicy* storage_policy,
-                     CanonicalCookieVector* cookies) {
-    scoped_refptr<net::SQLitePersistentCookieStore> sqlite_store(
-        new net::SQLitePersistentCookieStore(
-            temp_dir_.GetPath().Append(kTestCookiesFilename),
-            base::CreateSequencedTaskRunner(
-                {base::ThreadPool(), base::MayBlock()}),
-            background_task_runner_, true, nullptr));
-    store_ = new QuotaPolicyCookieStore(sqlite_store.get(), storage_policy);
-    Load(cookies);
-  }
-
-  // Adds a persistent cookie to store_.
-  void AddCookie(const std::string& name,
-                 const std::string& value,
-                 const std::string& domain,
-                 const std::string& path,
-                 const base::Time& creation) {
-    store_->AddCookie(net::CanonicalCookie(name, value, domain, path, creation,
-                                           creation, base::Time(), false, false,
-                                           net::CookieSameSite::NO_RESTRICTION,
-                                           net::COOKIE_PRIORITY_DEFAULT));
-  }
-
-  void DestroyStore() {
-    store_ = nullptr;
-    // Ensure that |store_|'s destructor has run by flushing ThreadPool.
-    base::ThreadPoolInstance::Get()->FlushForTesting();
-  }
-
-  void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  }
-
-  void TearDown() override {
-    DestroyStore();
-  }
-
-  BrowserTaskEnvironment task_environment_;
-  const scoped_refptr<base::SequencedTaskRunner> background_task_runner_ =
-      base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock()});
-  base::WaitableEvent loaded_event_;
-  base::WaitableEvent destroy_event_;
-  base::ScopedTempDir temp_dir_;
-  scoped_refptr<QuotaPolicyCookieStore> store_;
-  CanonicalCookieVector cookies_;
-};
-
-// Test if data is stored as expected in the QuotaPolicy database.
-TEST_F(QuotaPolicyCookieStoreTest, TestPersistence) {
-  CanonicalCookieVector cookies;
-  CreateAndLoad(nullptr, &cookies);
-  ASSERT_EQ(0U, cookies.size());
-
-  base::Time t = base::Time::Now();
-  AddCookie("A", "B", "foo.com", "/", t);
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "persistent.com", "/", t);
-
-  // Replace the store, which forces the current store to flush data to
-  // disk. Then, after reloading the store, confirm that the data was flushed by
-  // making sure it loads successfully.  This ensures that all pending commits
-  // are made to the store before allowing it to be closed.
-  DestroyStore();
-
-  // Reload and test for persistence.
-  cookies.clear();
-  CreateAndLoad(nullptr, &cookies);
-  EXPECT_EQ(2U, cookies.size());
-  bool found_foo_cookie = false;
-  bool found_persistent_cookie = false;
-  for (const auto& cookie : cookies) {
-    if (cookie->Domain() == "foo.com")
-      found_foo_cookie = true;
-    else if (cookie->Domain() == "persistent.com")
-      found_persistent_cookie = true;
-  }
-  EXPECT_TRUE(found_foo_cookie);
-  EXPECT_TRUE(found_persistent_cookie);
-
-  // Now delete the cookies and check persistence again.
-  store_->DeleteCookie(*cookies[0]);
-  store_->DeleteCookie(*cookies[1]);
-  DestroyStore();
-
-  // Reload and check if the cookies have been removed.
-  cookies.clear();
-  CreateAndLoad(nullptr, &cookies);
-  EXPECT_EQ(0U, cookies.size());
-  cookies.clear();
-}
-
-// Test if data is stored as expected in the QuotaPolicy database.
-TEST_F(QuotaPolicyCookieStoreTest, TestPolicy) {
-  CanonicalCookieVector cookies;
-  CreateAndLoad(nullptr, &cookies);
-  ASSERT_EQ(0U, cookies.size());
-
-  base::Time t = base::Time::Now();
-  AddCookie("A", "B", "foo.com", "/", t);
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "persistent.com", "/", t);
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "nonpersistent.com", "/", t);
-
-  // Replace the store, which forces the current store to flush data to
-  // disk. Then, after reloading the store, confirm that the data was flushed by
-  // making sure it loads successfully.  This ensures that all pending commits
-  // are made to the store before allowing it to be closed.
-  DestroyStore();
-  // Specify storage policy that makes "nonpersistent.com" session only.
-  scoped_refptr<content::MockSpecialStoragePolicy> storage_policy =
-      new content::MockSpecialStoragePolicy();
-  storage_policy->AddSessionOnly(
-      net::cookie_util::CookieOriginToURL("nonpersistent.com", false));
-
-  // Reload and test for persistence.
-  cookies.clear();
-  CreateAndLoad(storage_policy.get(), &cookies);
-  EXPECT_EQ(3U, cookies.size());
-
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "nonpersistent.com", "/second", t);
-
-  // Now close the store, and "nonpersistent.com" should be deleted according to
-  // policy.
-  DestroyStore();
-  cookies.clear();
-  CreateAndLoad(nullptr, &cookies);
-
-  EXPECT_EQ(2U, cookies.size());
-  for (const auto& cookie : cookies) {
-    EXPECT_NE("nonpersistent.com", cookie->Domain());
-  }
-  cookies.clear();
-}
-
-TEST_F(QuotaPolicyCookieStoreTest, ForceKeepSessionState) {
-  CanonicalCookieVector cookies;
-  CreateAndLoad(nullptr, &cookies);
-  ASSERT_EQ(0U, cookies.size());
-
-  base::Time t = base::Time::Now();
-  AddCookie("A", "B", "foo.com", "/", t);
-
-  // Recreate |store_| with a storage policy that makes "nonpersistent.com"
-  // session only, but then instruct the store to forcibly keep all cookies.
-  DestroyStore();
-  scoped_refptr<content::MockSpecialStoragePolicy> storage_policy =
-      new content::MockSpecialStoragePolicy();
-  storage_policy->AddSessionOnly(
-      net::cookie_util::CookieOriginToURL("nonpersistent.com", false));
-
-  // Reload and test for persistence
-  cookies.clear();
-  CreateAndLoad(storage_policy.get(), &cookies);
-  EXPECT_EQ(1U, cookies.size());
-
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "persistent.com", "/", t);
-  t += base::TimeDelta::FromInternalValue(10);
-  AddCookie("A", "B", "nonpersistent.com", "/", t);
-
-  // Now close the store, but the "nonpersistent.com" cookie should not be
-  // deleted.
-  store_->SetForceKeepSessionState();
-  DestroyStore();
-  cookies.clear();
-  CreateAndLoad(nullptr, &cookies);
-
-  EXPECT_EQ(3U, cookies.size());
-  cookies.clear();
-}
-
-// Tests that the special storage policy is properly applied even when the store
-// is destroyed on a background thread.
-TEST_F(QuotaPolicyCookieStoreTest, TestDestroyOnBackgroundThread) {
-  // Specify storage policy that makes "nonpersistent.com" session only.
-  scoped_refptr<content::MockSpecialStoragePolicy> storage_policy =
-      new content::MockSpecialStoragePolicy();
-  storage_policy->AddSessionOnly(
-      net::cookie_util::CookieOriginToURL("nonpersistent.com", false));
-
-  CanonicalCookieVector cookies;
-  CreateAndLoad(storage_policy.get(), &cookies);
-  ASSERT_EQ(0U, cookies.size());
-
-  base::Time t = base::Time::Now();
-  AddCookie("A", "B", "nonpersistent.com", "/", t);
-
-  // Replace the store, which forces the current store to flush data to
-  // disk. Then, after reloading the store, confirm that the data was flushed by
-  // making sure it loads successfully.  This ensures that all pending commits
-  // are made to the store before allowing it to be closed.
-  DestroyStoreOnBackgroundThread();
-
-  // Reload and test for persistence.
-  cookies.clear();
-  CreateAndLoad(storage_policy.get(), &cookies);
-  EXPECT_EQ(0U, cookies.size());
-
-  cookies.clear();
-}
-
-}  // namespace
-}  // namespace content

content/public/browser/cookie_store_factory.h

@@ -23,10 +23,6 @@ class CookieStore;
 class NetLog;
 }
 
-namespace storage {
-class SpecialStoragePolicy;
-}
-
 namespace content {
 
 struct CONTENT_EXPORT CookieStoreConfig {
@@ -41,14 +37,12 @@ struct CONTENT_EXPORT CookieStoreConfig {
   // created using this config.
   CookieStoreConfig(const base::FilePath& path,
                     bool restore_old_session_cookies,
-                    bool persist_session_cookies,
-                    storage::SpecialStoragePolicy* storage_policy);
+                    bool persist_session_cookies);
   ~CookieStoreConfig();
 
   const base::FilePath path;
   const bool restore_old_session_cookies;
   const bool persist_session_cookies;
-  const scoped_refptr<storage::SpecialStoragePolicy> storage_policy;
 
   // The following are infrequently used cookie store parameters.
   // Rather than clutter the constructor API, these are assigned a default

content/test/BUILD.gn

@@ -1666,7 +1666,6 @@ test("content_unittests") {
     "../browser/native_file_system/native_file_system_handle_base_unittest.cc",
     "../browser/native_file_system/native_file_system_manager_impl_unittest.cc",
     "../browser/net/network_quality_observer_impl_unittest.cc",
-    "../browser/net/quota_policy_cookie_store_unittest.cc",
     "../browser/network_context_client_base_impl_unittest.cc",
     "../browser/notification_service_impl_unittest.cc",
     "../browser/notifications/blink_notification_service_impl_unittest.cc",