CookieMonster: Don't load entire cookie jar on SetCanonicalCookieAsync

Before this change, the SQLite store and the cookie
monster had different notions of how exactly |key| passed to
PersistentStore::LoadCookiesForKey should be computed[1], and the
version used by CookieMonster was not really computable from a
CanonicalCookie.  This changes both of these to use
CookieMonster::GetKey, which is already used with both
CanonicalCookie's and URLs, and can therefore be easily called from
SetCanonicalCookieAsync as well.

[1] In particular they disagreed on chrome-extension:// cookies, but
that appeared to have been deterministically masked by implementation
details of the SQLite cookie store, so doesn't seem to have been a bug
that actually affected our users.

Bug: 813141
Change-Id: Iaa3840a1e1e28992c99413142a62fddff97c7f78
Reviewed-on: https://chromium-review.googlesource.com/940141
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541036}

net/cookies/cookie_monster.cc

@@ -56,6 +56,7 @@
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -414,17 +415,16 @@ void CookieMonster::SetCanonicalCookieAsync(
     SetCookiesCallback callback) {
   DCHECK(cookie->IsCanonical());
 
-  // TODO(rdsmith): Switch to DoCookieCallbackForURL (or the equivalent).
-  // This is tricky because we don't have the scheme in this routine
-  // and DoCookieCallbackForURL uses
-  // cookie_util::GetEffectiveDomain(scheme, host)
-  // to generate the database key to block behind.
-  DoCookieCallback(base::BindOnce(
-      // base::Unretained is safe as DoCookieCallbackForURL stores
-      // the callback on |*this|, so the callback will not outlive
-      // the object.
-      &CookieMonster::SetCanonicalCookie, base::Unretained(this),
-      std::move(cookie), secure_source, modify_http_only, std::move(callback)));
+  std::string domain = cookie->Domain();
+  DoCookieCallbackForHostOrDomain(
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::SetCanonicalCookie, base::Unretained(this),
+          std::move(cookie), secure_source, modify_http_only,
+          std::move(callback)),
+      domain);
 }
 
 void CookieMonster::SetCookieWithOptionsAsync(const GURL& url,
@@ -1052,7 +1052,7 @@ void CookieMonster::FindCookiesForHostAndDomain(
   RecordPeriodicStats(current_time);
 
   // Can just dispatch to FindCookiesForKey
-  const std::string key(GetKey(url.host()));
+  const std::string key(GetKey(url.host_piece()));
   FindCookiesForKey(key, url, options, current_time, cookies);
 }
 
@@ -1623,9 +1623,9 @@ size_t CookieMonster::GarbageCollectLeastRecentlyAccessed(
 }
 
 // A wrapper around registry_controlled_domains::GetDomainAndRegistry
-// to make clear we're creating a key for our local map.  Here and
-// in FindCookiesForHostAndDomain() are the only two places where
-// we need to conditionalize based on key type.
+// to make clear we're creating a key for our local map or for the persistent
+// store's use. Here and in FindCookiesForHostAndDomain() are the only two
+// places where we need to conditionalize based on key type.
 //
 // Note that this key algorithm explicitly ignores the scheme.  This is
 // because when we're entering cookies into the map from the backing store,
@@ -1645,14 +1645,14 @@ size_t CookieMonster::GarbageCollectLeastRecentlyAccessed(
 // thus restricting each scheme to a single cookie monster (which might
 // be worth it, but is still too much trouble to solve what is currently a
 // non-problem).
-std::string CookieMonster::GetKey(const std::string& domain) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
+//
+// static
+std::string CookieMonster::GetKey(base::StringPiece domain) {
   std::string effective_domain(
       registry_controlled_domains::GetDomainAndRegistry(
           domain, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
   if (effective_domain.empty())
-    effective_domain = domain;
+    domain.CopyToString(&effective_domain);
 
   if (!effective_domain.empty() && effective_domain[0] == '.')
     return effective_domain.substr(1);
@@ -1782,6 +1782,12 @@ void CookieMonster::DoCookieCallback(base::OnceClosure callback) {
 
 void CookieMonster::DoCookieCallbackForURL(base::OnceClosure callback,
                                            const GURL& url) {
+  DoCookieCallbackForHostOrDomain(std::move(callback), url.host_piece());
+}
+
+void CookieMonster::DoCookieCallbackForHostOrDomain(
+    base::OnceClosure callback,
+    base::StringPiece host_or_domain) {
   MarkCookieStoreAsInitialized();
   FetchAllCookiesIfNecessary();
 
@@ -1798,7 +1804,7 @@ void CookieMonster::DoCookieCallbackForURL(base::OnceClosure callback,
     }
 
     // Checks if the domain key has been loaded.
-    std::string key(cookie_util::GetEffectiveDomain(url.scheme(), url.host()));
+    std::string key = GetKey(host_or_domain);
     if (keys_loaded_.find(key) == keys_loaded_.end()) {
       std::map<std::string, base::circular_deque<base::OnceClosure>>::iterator
           it = tasks_pending_for_key_.find(key);

net/cookies/cookie_monster.h

@@ -23,6 +23,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/strings/string_piece.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
@@ -50,11 +51,12 @@ class CookieChangeDispatcher;
 // backing store. Otherwise, callbacks may be invoked immediately.
 //
 // A cookie task is either pending loading of the entire cookie store, or
-// loading of cookies for a specific domain key(eTLD+1). In the former case, the
-// cookie callback will be queued in tasks_pending_ while PersistentCookieStore
-// chain loads the cookie store on DB thread. In the latter case, the cookie
-// callback will be queued in tasks_pending_for_key_ while PermanentCookieStore
-// loads cookies for the specified domain key(eTLD+1) on DB thread.
+// loading of cookies for a specific domain key (GetKey(), roughly eTLD+1). In
+// the former case, the cookie callback will be queued in tasks_pending_ while
+// PersistentCookieStore chain loads the cookie store on DB thread. In the
+// latter case, the cookie callback will be queued in tasks_pending_for_key_
+// while PermanentCookieStore loads cookies for the specified domain key on DB
+// thread.
 //
 // TODO(deanm) Implement CookieMonster, the cookie database.
 //  - Verify that our domain enforcement and non-dotted handling is correct
@@ -205,6 +207,12 @@ class NET_EXPORT CookieMonster : public CookieStore {
 
   bool IsEphemeral() override;
 
+  // Find a key based on the given domain, which will be used to find all
+  // cookies potentially relevant to it. This is used for lookup in cookies_ as
+  // well as for PersistentCookieStore::LoadCookiesForKey. See comment on keys
+  // before the CookieMap typedef.
+  static std::string GetKey(base::StringPiece domain);
+
  private:
   CookieMonster(PersistentCookieStore* store,
                 ChannelIDService* channel_id_service,
@@ -417,9 +425,9 @@ class NET_EXPORT CookieMonster : public CookieStore {
 
   // Stores cookies loaded from the backing store and invokes the deferred
   // task(s) pending loading of cookies associated with the domain key
-  // (eTLD+1). Called when all cookies for the domain key(eTLD+1) have been
-  // loaded from DB. See PersistentCookieStore::Load for details on the contents
-  // of cookies.
+  // (GetKey, roughly eTLD+1). Called when all cookies for the domain key have
+  // been loaded from DB. See PersistentCookieStore::Load for details on the
+  // contents of cookies.
   void OnKeyLoaded(const std::string& key,
                    std::vector<std::unique_ptr<CanonicalCookie>> cookies);
 
@@ -545,10 +553,6 @@ class NET_EXPORT CookieMonster : public CookieStore {
                                              CookieItVector cookie_its,
                                              base::Time* earliest_time);
 
-  // Find the key (for lookup in cookies_) based on the given domain.
-  // See comment on keys before the CookieMap typedef.
-  std::string GetKey(const std::string& domain) const;
-
   bool HasCookieableScheme(const GURL& url);
 
   // Statistics support
@@ -565,14 +569,20 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // ugly and increment when we've seen the same time twice.
   base::Time CurrentTime();
 
-  // Runs the callback if, or defers the callback until, the full cookie
-  // database is loaded.
+  // Defers the callback until the full coookie database has been loaded. If
+  // it's already been loaded, runs the callback synchronously.
   void DoCookieCallback(base::OnceClosure callback);
 
-  // Runs the callback if, or defers the callback until, the cookies for the
-  // given URL are loaded.
+  // Defers the callback until the cookies relevant to given URL have been
+  // loaded. If they've already been loaded, runs the callback synchronously.
   void DoCookieCallbackForURL(base::OnceClosure callback, const GURL& url);
 
+  // Defers the callback until the cookies relevant to given host or domain
+  // have been loaded. If they've already been loaded, runs the callback
+  // synchronously.
+  void DoCookieCallbackForHostOrDomain(base::OnceClosure callback,
+                                       base::StringPiece host_or_domain);
+
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
   base::HistogramBase* histogram_expiration_duration_minutes_;

net/cookies/cookie_monster_unittest.cc

@@ -3117,6 +3117,48 @@ TEST_F(CookieMonsterTest, EquivalentCookies) {
   EXPECT_FALSE(SetCookie(cm.get(), http_url, "A=D; domain=foo.com"));
 }
 
+TEST_F(CookieMonsterTest, SetCanonicalCookieDoesNotBlockForLoadAll) {
+  scoped_refptr<MockPersistentCookieStore> persistent_store =
+      base::MakeRefCounted<MockPersistentCookieStore>();
+  // Collect load commands so we have control over their execution.
+  persistent_store->set_store_load_commands(true);
+  CookieMonster cm(persistent_store.get());
+
+  // Start of a canonical cookie set.
+  ResultSavingCookieCallback<bool> callback_set;
+  cm.SetCanonicalCookieAsync(
+      CanonicalCookie::Create(GURL("http://a.com/"), "A=B", base::Time::Now(),
+                              CookieOptions()),
+      false /* secure_source */, false /* modify_httponly */,
+      base::BindOnce(&ResultSavingCookieCallback<bool>::Run,
+                     base::Unretained(&callback_set)));
+
+  // Get cookies for a different URL.
+  GetCookieListCallback callback_get;
+  cm.GetCookieListWithOptionsAsync(
+      GURL("http://b.com/"), CookieOptions(),
+      base::BindOnce(&GetCookieListCallback::Run,
+                     base::Unretained(&callback_get)));
+
+  // Now go through the store commands, and execute individual loads.
+  for (const CookieStoreCommand& command : persistent_store->commands()) {
+    if (command.type == CookieStoreCommand::LOAD_COOKIES_FOR_KEY)
+      command.loaded_callback.Run(
+          std::vector<std::unique_ptr<CanonicalCookie>>());
+  }
+
+  // This should be enough for both individual commands.
+  callback_set.WaitUntilDone();
+  callback_get.WaitUntilDone();
+
+  // Now execute full-store loads as well.
+  for (const CookieStoreCommand& command : persistent_store->commands()) {
+    if (command.type == CookieStoreCommand::LOAD)
+      command.loaded_callback.Run(
+          std::vector<std::unique_ptr<CanonicalCookie>>());
+  }
+}
+
 class CookieMonsterNotificationTest : public CookieMonsterTest {
  public:
   CookieMonsterNotificationTest()

net/extras/sqlite/sqlite_persistent_cookie_store.cc

@@ -23,7 +23,6 @@
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "base/time/time.h"
-#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_constants.h"
 #include "net/cookies/cookie_util.h"
@@ -718,9 +717,7 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
   // Build a map of domain keys (always eTLD+1) to domains.
   for (size_t idx = 0; idx < host_keys.size(); ++idx) {
     const std::string& domain = host_keys[idx];
-    std::string key = registry_controlled_domains::GetDomainAndRegistry(
-        domain, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
-
+    std::string key = CookieMonster::GetKey(domain);
     keys_to_load_[key].insert(domain);
   }
 

net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

@@ -25,8 +25,10 @@
 #include "base/time/time.h"
 #include "crypto/encryptor.h"
 #include "crypto/symmetric_key.h"
+#include "net/base/test_completion_callback.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_constants.h"
+#include "net/cookies/cookie_store_test_callbacks.h"
 #include "net/extras/sqlite/cookie_crypto_delegate.h"
 #include "net/test/net_test_suite.h"
 #include "sql/connection.h"
@@ -129,12 +131,16 @@ class SQLitePersistentCookieStoreTest : public testing::Test {
     NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
   }
 
-  void Create(bool crypt_cookies, bool restore_old_session_cookies) {
+  void Create(bool crypt_cookies,
+              bool restore_old_session_cookies,
+              bool use_current_thread) {
     if (crypt_cookies)
       cookie_crypto_delegate_.reset(new CookieCryptor());
 
     store_ = new SQLitePersistentCookieStore(
-        temp_dir_.GetPath().Append(kCookieFilename), client_task_runner_,
+        temp_dir_.GetPath().Append(kCookieFilename),
+        use_current_thread ? base::ThreadTaskRunnerHandle::Get()
+                           : client_task_runner_,
         background_task_runner_, restore_old_session_cookies,
         cookie_crypto_delegate_.get());
   }
@@ -142,7 +148,8 @@ class SQLitePersistentCookieStoreTest : public testing::Test {
   void CreateAndLoad(bool crypt_cookies,
                      bool restore_old_session_cookies,
                      CanonicalCookieVector* cookies) {
-    Create(crypt_cookies, restore_old_session_cookies);
+    Create(crypt_cookies, restore_old_session_cookies,
+           false /* use_current_thread */);
     Load(cookies);
   }
 
@@ -364,10 +371,8 @@ TEST_F(SQLitePersistentCookieStoreTest, TestLoadCookiesForKey) {
   // can't run. To allow precise control of |background_task_runner_| without
   // preventing client tasks to run, use base::ThreadTaskRunnerHandle::Get()
   // instead of |client_task_runner_| for this test.
-  store_ = new SQLitePersistentCookieStore(
-      temp_dir_.GetPath().Append(kCookieFilename),
-      base::ThreadTaskRunnerHandle::Get(), background_task_runner_, false,
-      nullptr);
+  Create(false /* crypt_cookies */, false /* restore_old_session_cookies */,
+         true /* use_current_thread */);
 
   // Posting a blocking task to db_thread_ makes sure that the DB thread waits
   // until both Load and LoadCookiesForKey have been posted to its task queue.
@@ -881,7 +886,7 @@ TEST_F(SQLitePersistentCookieStoreTest, EmptyLoadAfterClose) {
   DestroyStore();
 
   // Create the cookie store, but immediately close it.
-  Create(false, false);
+  Create(false, false, false);
   store_->Close(base::Closure());
 
   // Expect any attempt to call Load() to synchronously respond with an empty
@@ -1143,4 +1148,78 @@ TEST_F(SQLitePersistentCookieStoreTest, IdenticalCreationTimes) {
   EXPECT_EQ("/", read_in_cookies[i]->Path());
 }
 
+TEST_F(SQLitePersistentCookieStoreTest, KeyInconsistency) {
+  // Regression testcase for previous disagreement between CookieMonster
+  // and SQLitePersistentCookieStoreTest as to what keys to LoadCookiesForKey
+  // mean. The particular example doesn't, of course, represent an actual in-use
+  // scenario, but while the inconstancy could happen with chrome-extension
+  // URLs in real life, it was irrelevant for them in practice since their
+  // rows would get key = "" which would get sorted before actual domains,
+  // and therefore get loaded first by CookieMonster::FetchAllCookiesIfNecessary
+  // with the task runners involved ensuring that would finish before the
+  // incorrect LoadCookiesForKey got the chance to run.
+  //
+  // This test uses a URL that used to be treated differently by the two
+  // layers that also sorts after other rows to avoid this scenario.
+
+  // SQLitePersistentCookieStore will run its callbacks on what's passed to it
+  // as |client_task_runner|, and CookieMonster expects to get callbacks from
+  // its PersistentCookieStore on the same thread as its methods are invoked on;
+  // so to avoid needing to post every CookieMonster API call, this uses the
+  // current thread for SQLitePersistentCookieStore's |client_task_runner|.
+  Create(false, false, true /* use_current_thread */);
+
+  // Create a cookie on a scheme that doesn't handle cookies by default,
+  // and save it.
+  std::unique_ptr<CookieMonster> cookie_monster =
+      std::make_unique<CookieMonster>(store_.get());
+  cookie_monster->SetCookieableSchemes({"gopher", "http"});
+  ResultSavingCookieCallback<bool> set_cookie_callback;
+  cookie_monster->SetCookieWithOptionsAsync(
+      GURL("gopher://subdomain.gopheriffic.com/page"), "A=B; max-age=3600",
+      CookieOptions(),
+      base::BindOnce(&ResultSavingCookieCallback<bool>::Run,
+                     base::Unretained(&set_cookie_callback)));
+  set_cookie_callback.WaitUntilDone();
+  EXPECT_TRUE(set_cookie_callback.result());
+
+  // Also insert a whole bunch of cookies to slow down the background loading of
+  // all the cookies.
+  for (int i = 0; i < 50; ++i) {
+    ResultSavingCookieCallback<bool> set_cookie_callback2;
+    cookie_monster->SetCookieWithOptionsAsync(
+        GURL(base::StringPrintf("http://example%d.com/", i)),
+        "A=B; max-age=3600", CookieOptions(),
+        base::BindOnce(&ResultSavingCookieCallback<bool>::Run,
+                       base::Unretained(&set_cookie_callback2)));
+    set_cookie_callback2.WaitUntilDone();
+    EXPECT_TRUE(set_cookie_callback2.result());
+  }
+
+  net::TestClosure flush_closure;
+  cookie_monster->FlushStore(flush_closure.closure());
+  flush_closure.WaitForResult();
+  cookie_monster = nullptr;
+
+  // Re-create the PersistentCookieStore & CookieMonster. Note that the
+  // destroyed store's ops will happen on same runners as the previous
+  // instances, so they should complete before the new PersistentCookieStore
+  // starts looking at the state on disk.
+  Create(false, false, true /* want current thread to invoke cookie monster */);
+  cookie_monster = std::make_unique<CookieMonster>(store_.get());
+  cookie_monster->SetCookieableSchemes({"gopher", "http"});
+
+  // Now try to get the cookie back.
+  GetCookieListCallback get_callback;
+  cookie_monster->GetCookieListWithOptionsAsync(
+      GURL("gopher://subdomain.gopheriffic.com/page"), CookieOptions(),
+      base::BindOnce(&GetCookieListCallback::Run,
+                     base::Unretained(&get_callback)));
+  get_callback.WaitUntilDone();
+  ASSERT_EQ(1u, get_callback.cookies().size());
+  EXPECT_EQ("A", get_callback.cookies()[0].Name());
+  EXPECT_EQ("B", get_callback.cookies()[0].Value());
+  EXPECT_EQ("subdomain.gopheriffic.com", get_callback.cookies()[0].Domain());
+};
+
 }  // namespace net