Make CookieMonster give a consistent picture of the world while loading.

Previously, tasks that affected each other could have been executed out
of the order they were started in.  While it's unclear if this resulted
in any real bugs, it seems something that could well cause them. This
CL makes sure that all CookieMosnter tasks that can affect each other
are run in the order they are queued.

It should have a minimal impact on performance - tasks that affect all
cookies in the store are pretty rare, so as long as none of them are
executed, there's no impact.

BUG=588305

Review URL: https://codereview.chromium.org/1717433004

Cr-Commit-Position: refs/heads/master@{#379815}

net/cookies/cookie_monster.cc

@@ -349,6 +349,7 @@ CookieMonster::CookieMonster(PersistentCookieStore* store,
       started_fetching_all_cookies_(false),
       finished_fetching_all_cookies_(false),
       fetch_strategy_(kUnknownFetch),
+      seen_global_task_(false),
       store_(store),
       last_access_threshold_(base::TimeDelta::FromMilliseconds(
           last_access_threshold_milliseconds)),
@@ -1444,12 +1445,27 @@ void CookieMonster::StoreLoadedCookies(
 void CookieMonster::InvokeQueue() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // Move all per-key tasks into the global queue, if there are any.  This is
+  // protection about a race where the store learns about all cookies loading
+  // before it learned about the cookies for a key loading.
+
+  // Needed to prevent any recursively queued tasks from going back into the
+  // per-key queues.
+  seen_global_task_ = true;
+  for (const auto& tasks_for_key : tasks_pending_for_key_) {
+    tasks_pending_.insert(tasks_pending_.begin(), tasks_for_key.second.begin(),
+                          tasks_for_key.second.end());
+  }
+  tasks_pending_for_key_.clear();
+
   while (!tasks_pending_.empty()) {
     scoped_refptr<CookieMonsterTask> request_task = tasks_pending_.front();
-    tasks_pending_.pop();
+    tasks_pending_.pop_front();
     request_task->Run();
   }
 
+  DCHECK(tasks_pending_for_key_.empty());
+
   finished_fetching_all_cookies_ = true;
   creation_times_.clear();
   keys_loaded_.clear();
@@ -2223,9 +2239,10 @@ void CookieMonster::DoCookieTask(
 
   MarkCookieStoreAsInitialized();
   FetchAllCookiesIfNecessary();
+  seen_global_task_ = true;
 
   if (!finished_fetching_all_cookies_ && store_.get()) {
-    tasks_pending_.push(task_item);
+    tasks_pending_.push_back(task_item);
     return;
   }
 
@@ -2235,15 +2252,22 @@ void CookieMonster::DoCookieTask(
 void CookieMonster::DoCookieTaskForURL(
     const scoped_refptr<CookieMonsterTask>& task_item,
     const GURL& url) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   MarkCookieStoreAsInitialized();
   if (ShouldFetchAllCookiesWhenFetchingAnyCookie())
     FetchAllCookiesIfNecessary();
 
   // If cookies for the requested domain key (eTLD+1) have been loaded from DB
   // then run the task, otherwise load from DB.
   if (!finished_fetching_all_cookies_ && store_.get()) {
+    // If a global task has been previously seen, queue the task as a global
+    // task. Note that the CookieMonster may be in the middle of executing
+    // the global queue, |tasks_pending_| may be empty, which is why another
+    // bool is needed.
+    if (seen_global_task_) {
+      tasks_pending_.push_back(task_item);
+      return;
+    }
+
     // Checks if the domain key has been loaded.
     std::string key(cookie_util::GetEffectiveDomain(url.scheme(), url.host()));
     if (keys_loaded_.find(key) == keys_loaded_.end()) {

net/cookies/cookie_monster.h

@@ -652,7 +652,14 @@ class NET_EXPORT CookieMonster : public CookieStore {
 
   // Queues tasks that are blocked until all cookies are loaded from the backend
   // store.
-  std::queue<scoped_refptr<CookieMonsterTask>> tasks_pending_;
+  std::deque<scoped_refptr<CookieMonsterTask>> tasks_pending_;
+
+  // Once a global cookie task has been seen, all per-key tasks must be put in
+  // |tasks_pending_| instead of |tasks_pending_for_key_| to ensure a reasonable
+  // view of the cookie store. This more to ensure fancy cookie export/import
+  // code has a consistent view of the CookieStore, rather than out of concern
+  // for typical use.
+  bool seen_global_task_;
 
   scoped_refptr<PersistentCookieStore> store_;
 

net/cookies/cookie_monster_store_test.cc

@@ -25,9 +25,19 @@ LoadedCallbackTask::LoadedCallbackTask(LoadedCallback loaded_callback,
 LoadedCallbackTask::~LoadedCallbackTask() {
 }
 
+CookieStoreCommand::CookieStoreCommand(
+    Type type,
+    const CookieMonster::PersistentCookieStore::LoadedCallback& loaded_callback,
+    const std::string& key)
+    : type(type), loaded_callback(loaded_callback), key(key) {}
+
+CookieStoreCommand::CookieStoreCommand(Type type, const CanonicalCookie& cookie)
+    : type(type), cookie(cookie) {}
+
+CookieStoreCommand::~CookieStoreCommand() {}
+
 MockPersistentCookieStore::MockPersistentCookieStore()
-    : load_return_value_(true), loaded_(false) {
-}
+    : store_load_commands_(false), load_return_value_(true), loaded_(false) {}
 
 void MockPersistentCookieStore::SetLoadExpectation(
     bool return_value,
@@ -37,6 +47,11 @@ void MockPersistentCookieStore::SetLoadExpectation(
 }
 
 void MockPersistentCookieStore::Load(const LoadedCallback& loaded_callback) {
+  if (store_load_commands_) {
+    commands_.push_back(
+        CookieStoreCommand(CookieStoreCommand::LOAD, loaded_callback, ""));
+    return;
+  }
   std::vector<CanonicalCookie*> out_cookies;
   if (load_return_value_) {
     out_cookies = load_result_;
@@ -51,6 +66,11 @@ void MockPersistentCookieStore::Load(const LoadedCallback& loaded_callback) {
 void MockPersistentCookieStore::LoadCookiesForKey(
     const std::string& key,
     const LoadedCallback& loaded_callback) {
+  if (store_load_commands_) {
+    commands_.push_back(CookieStoreCommand(
+        CookieStoreCommand::LOAD_COOKIES_FOR_KEY, loaded_callback, key));
+    return;
+  }
   if (!loaded_) {
     Load(loaded_callback);
   } else {

net/cookies/cookie_monster_store_test.h

@@ -51,20 +51,39 @@ class LoadedCallbackTask
   DISALLOW_COPY_AND_ASSIGN(LoadedCallbackTask);
 };  // Wrapper class LoadedCallbackTask
 
-// Describes a call to one of the 3 functions of PersistentCookieStore.
+// Describes a call to one of the 5 functions of PersistentCookieStore.
 struct CookieStoreCommand {
   enum Type {
+    LOAD,
+    LOAD_COOKIES_FOR_KEY,
     // UPDATE_ACCESS_TIME is not included in this list, because get cookie
     // commands may or may not end updating the access time, unless they have
     // the option set not to do so.
     ADD,
     REMOVE,
   };
 
-  CookieStoreCommand(Type type, const CanonicalCookie& cookie)
-      : type(type), cookie(cookie) {}
+  // Constructor for LOAD and LOAD_COOKIES_FOR_KEY calls.  |key| should be empty
+  // for LOAD_COOKIES_FOR_KEY.
+  CookieStoreCommand(Type type,
+                     const CookieMonster::PersistentCookieStore::LoadedCallback&
+                         loaded_callback,
+                     const std::string& key);
+
+  // Constructor for ADD, UPDATE_ACCESS_TIME, and REMOVE calls.
+  CookieStoreCommand(Type type, const CanonicalCookie& cookie);
+
+  ~CookieStoreCommand();
 
   Type type;
+
+  // Only non-null for LOAD and LOAD_COOKIES_FOR_KEY.
+  CookieMonster::PersistentCookieStore::LoadedCallback loaded_callback;
+
+  // Only non-empty for LOAD_COOKIES_FOR_KEY.
+  std::string key;
+
+  // Only non-null for ADD, UPDATE_ACCESS_TIME, and REMOVE.
   CanonicalCookie cookie;
 };
 
@@ -77,6 +96,12 @@ class MockPersistentCookieStore : public CookieMonster::PersistentCookieStore {
 
   MockPersistentCookieStore();
 
+  // When set, Load() and LoadCookiesForKey() calls are store in the command
+  // list, rather than being automatically executed. Defaults to false.
+  void set_store_load_commands(bool store_load_commands) {
+    store_load_commands_ = store_load_commands;
+  }
+
   void SetLoadExpectation(bool return_value,
                           const std::vector<CanonicalCookie*>& result);
 
@@ -103,6 +128,8 @@ class MockPersistentCookieStore : public CookieMonster::PersistentCookieStore {
  private:
   CommandList commands_;
 
+  bool store_load_commands_;
+
   // Deferred result to use when Load() is called.
   bool load_return_value_;
   std::vector<CanonicalCookie*> load_result_;