[iOS] Use canonical DeleteCookies API from CookieManager.

This allows us to outsource all cookie tracking efforts to the dedicated
Chrome implementation rather than re-implementing the cookie request
queue.

Bug: 1102794
Change-Id: I284888a796acd60cfc696598e53e7debc726b6c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346664
Commit-Queue: Nohemi Fernandez <fernandex@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803045}

components/signin/ios/browser/account_consistency_service.h

@@ -10,7 +10,6 @@
 #include <string>
 
 #include "base/callback.h"
-#include "base/containers/circular_deque.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
@@ -71,10 +70,6 @@ class AccountConsistencyService : public KeyedService,
   // Removes the handler associated with |web_state|.
   void RemoveWebStateHandler(web::WebState* web_state);
 
-  // Removes CHROME_CONNECTED cookies on all the Google domains where it was
-  // set. Calls callback once all cookies were removed.
-  void RemoveChromeConnectedCookies(base::OnceClosure callback);
-
   // Checks for the presence of Gaia cookies and if they have been deleted
   // notifies the AccountReconcilor (the class responsible for rebuilding Gaia
   // cookies if needed).
@@ -85,12 +80,12 @@ class AccountConsistencyService : public KeyedService,
 
   // Enqueues a request to set the CHROME_CONNECTED cookie for |domain|.
   // The cookie is set if it is not already on |domain|.
-  void SetChromeConnectedCookieWithDomain(const std::string& domain);
+  void SetChromeConnectedCookieWithDomains(
+      const std::vector<std::string>& domains);
 
-  // Enqueues a request to remove the CHROME_CONNECTED cookie to |domain|.
-  // Does nothing if the cookie is not set on |domain|.
-  void RemoveChromeConnectedCookieFromDomain(const std::string& domain,
-                                             base::OnceClosure callback);
+  // Removes CHROME_CONNECTED cookies on all the Google domains where it was
+  // set. Calls callback once all cookies were removed.
+  void RemoveAllChromeConnectedCookies(base::OnceClosure callback);
 
   // Notifies the AccountConsistencyService that browsing data has been removed
   // for any time period.
@@ -99,47 +94,24 @@ class AccountConsistencyService : public KeyedService,
  private:
   friend class AccountConsistencyServiceTest;
 
-  // The type of a cookie request.
-  enum CookieRequestType {
-    ADD_CHROME_CONNECTED_COOKIE,
-    REMOVE_CHROME_CONNECTED_COOKIE
-  };
-
-  // A CHROME_CONNECTED cookie request to be applied by the
-  // AccountConsistencyService.
-  struct CookieRequest {
-    static CookieRequest CreateAddCookieRequest(const std::string& domain);
-    static CookieRequest CreateRemoveCookieRequest(const std::string& domain,
-                                                   base::OnceClosure callback);
-    CookieRequest();
-    ~CookieRequest();
-
-    // Movable, not copyable.
-    CookieRequest(CookieRequest&&);
-    CookieRequest& operator=(CookieRequest&&) = default;
-    CookieRequest(const CookieRequest&) = delete;
-    CookieRequest& operator=(const CookieRequest&) = delete;
-
-    CookieRequestType request_type;
-    std::string domain;
-    base::OnceClosure callback;
-  };
-
   // Loads the domains with a CHROME_CONNECTED cookie from the prefs.
   void LoadFromPrefs();
 
   // KeyedService implementation.
   void Shutdown() override;
 
-  // Applies the pending CHROME_CONNECTED cookie requests one by one.
-  void ApplyCookieRequests();
+  // Sets the pending CHROME_CONNECTED cookie for the given |domain|.
+  void SetChromeConnectedCookieWithDomain(const std::string& domain);
 
   // Called when the request to set CHROME_CONNECTED cookie is done.
-  void FinishedSetChromeConnectedCookie(
+  void OnChromeConnectedCookieFinished(
+      const std::string& domain,
       net::CookieAccessResult cookie_access_result);
 
-  // Called when the current CHROME_CONNECTED cookie request is done.
-  void FinishedApplyingChromeConnectedCookieRequest(bool success);
+  // Called when cookie deletion is completed with the number of cookies that
+  // were removed from the cookie store.
+  void OnDeleteCookiesFinished(base::OnceClosure callback,
+                               uint32_t num_cookies_deleted);
 
   // Returns whether the CHROME_CONNECTED cookie should be added to |domain|.
   // If the cookie is not already on |domain|, it will return true. If the
@@ -152,8 +124,8 @@ class AccountConsistencyService : public KeyedService,
   // Enqueues a request to set the CHROME_CONNECTED cookie for |domain|.
   // The cookie is set if it is not already on |domain| or if it is too old
   // compared to the given |cookie_refresh_interval|.
-  void SetChromeConnectedCookieWithDomain(
-      const std::string& domain,
+  void SetChromeConnectedCookieWithDomains(
+      const std::vector<std::string>& domains,
       const base::TimeDelta& cookie_refresh_interval);
 
   // Adds CHROME_CONNECTED cookies on all the main Google domains.
@@ -167,6 +139,9 @@ class AccountConsistencyService : public KeyedService,
   // Records whether Gaia cookies were present on navigation in UMA histogram.
   static void LogIOSGaiaCookiesPresentOnNavigation(bool is_present);
 
+  // Clears all pending cookie requests and cached domains.
+  void ResetInternalState();
+
   // IdentityManager::Observer implementation.
   void OnPrimaryAccountSet(const CoreAccountInfo& account_info) override;
   void OnPrimaryAccountCleared(
@@ -189,10 +164,9 @@ class AccountConsistencyService : public KeyedService,
   // signout events.
   signin::IdentityManager* identity_manager_;
 
-  // Whether a CHROME_CONNECTED cookie request is currently being applied.
-  bool applying_cookie_requests_;
-  // The queue of CHROME_CONNECTED cookie requests to be applied.
-  base::circular_deque<CookieRequest> cookie_requests_;
+  // The number of cookie manager requests that are being processed.
+  // Used for testing purposes only.
+  int64_t active_cookie_manager_requests_for_testing_;
   // The map between domains where a CHROME_CONNECTED cookie is present and
   // the time when the cookie was last updated.
   std::map<std::string, base::Time> last_cookie_update_map_;

components/signin/ios/browser/account_consistency_service_unittest.mm

@@ -202,7 +202,8 @@ class AccountConsistencyServiceTest : public PlatformTest {
     // Spinning the runloop is needed to ensure that the cookie manager requests
     // are executed.
     base::RunLoop().RunUntilIdle();
-    EXPECT_TRUE(account_consistency_service_->cookie_requests_.empty());
+    EXPECT_EQ(0, account_consistency_service_
+                     ->active_cookie_manager_requests_for_testing_);
   }
 
   void SignIn() {
@@ -234,6 +235,16 @@ class AccountConsistencyServiceTest : public PlatformTest {
     return cookies_out;
   }
 
+  // Returns time the CHROME_CONNECTED cookie was last updated for |domain|.
+  base::Time GetCookieLastUpdateTime(const std::string& domain) {
+    return account_consistency_service_->last_cookie_update_map_[domain];
+  }
+
+  // Returns time the Gaia cookie was last updated for Google domains.
+  base::Time GetGaiaLastUpdateTime() {
+    return account_consistency_service_->last_gaia_cookie_verification_time_;
+  }
+
   void CheckDomainHasChromeConnectedCookie(const std::string& domain) {
     EXPECT_TRUE(
         ContainsCookie(GetCookiesInCookieJar(),
@@ -264,28 +275,23 @@ class AccountConsistencyServiceTest : public PlatformTest {
   // the cookies once the sign-out is done.
   void SimulateGaiaCookieManagerServiceLogout() {
     base::RunLoop run_loop;
-    account_consistency_service_->RemoveChromeConnectedCookies(
+    account_consistency_service_->RemoveAllChromeConnectedCookies(
         run_loop.QuitClosure());
     run_loop.Run();
   }
 
   // Simulates setting the CHROME_CONNECTED cookie for the Google domain at the
   // designated time interval. Returns the time at which the cookie was updated.
-  base::Time SimulateSetChromeConnectedCookie() {
-    account_consistency_service_->SetChromeConnectedCookieWithDomain(
-        kGoogleDomain);
+  void SimulateSetChromeConnectedCookieForGoogleDomain() {
+    account_consistency_service_->SetChromeConnectedCookieWithDomains(
+        {kGoogleDomain});
     WaitUntilAllCookieRequestsAreApplied();
-
-    return account_consistency_service_->last_cookie_update_map_[kGoogleDomain];
   }
 
   // Simulates updating the Gaia cookie on the Google domain at the designated
   // time interval. Returns the time at which the cookie was updated.
-  base::Time SimulateUpdateGaiaCookie() {
+  void SimulateUpdateGaiaCookie() {
     account_consistency_service_->SetGaiaCookiesIfDeleted();
-    WaitUntilAllCookieRequestsAreApplied();
-
-    return account_consistency_service_->last_gaia_cookie_verification_time_;
   }
 
   void CheckGoogleDomainHasGaiaCookie() {
@@ -489,44 +495,82 @@ TEST_F(AccountConsistencyServiceTest, DomainsClearedOnBrowsingDataRemoved) {
 
 TEST_F(AccountConsistencyServiceTest, SetChromeConnectedCookieNotUpdateTime) {
   SignIn();
-  base::Time first_cookie_update = SimulateSetChromeConnectedCookie();
 
-  // The second update will not send a cookie request, since this call is made
-  // before update time.
-  base::Time second_cookie_update = SimulateSetChromeConnectedCookie();
-  EXPECT_EQ(first_cookie_update, second_cookie_update);
+  const base::Time signin_time = base::Time::Now();
+  // Advance clock before 24-hour CHROME_CONNECTED update time.
+  task_environment_.FastForwardBy(base::TimeDelta::FromHours(2));
+  SimulateSetChromeConnectedCookieForGoogleDomain();
+
+  EXPECT_EQ(signin_time, GetCookieLastUpdateTime(kGoogleDomain));
 }
 
 TEST_F(AccountConsistencyServiceTest, SetChromeConnectedCookieAtUpdateTime) {
   SignIn();
-  base::Time first_cookie_update = SimulateSetChromeConnectedCookie();
 
   // Advance clock past 24-hour CHROME_CONNECTED update time.
   task_environment_.FastForwardBy(base::TimeDelta::FromDays(2));
+  const base::Time second_cookie_update_time = base::Time::Now();
+  SimulateSetChromeConnectedCookieForGoogleDomain();
 
-  // The second update will not send a cookie request, since CHROME_CONNECTED
-  // has already been set.
-  base::Time second_cookie_update = SimulateSetChromeConnectedCookie();
-  EXPECT_GT(second_cookie_update, first_cookie_update);
+  EXPECT_EQ(second_cookie_update_time, GetCookieLastUpdateTime(kGoogleDomain));
 }
 
 TEST_F(AccountConsistencyServiceTest, SetGaiaCookieUpdateNotUpdateTime) {
-  base::Time first_gaia_cookie_update = SimulateUpdateGaiaCookie();
+  SimulateUpdateGaiaCookie();
 
-  // The second update will not send a cookie request, since this call is made
-  // before update time.
-  base::Time second_gaia_cookie_update = SimulateUpdateGaiaCookie();
-  EXPECT_EQ(first_gaia_cookie_update, second_gaia_cookie_update);
+  // Advance clock past one-hour Gaia update time.
+  const base::Time first_update_time = base::Time::Now();
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  SimulateUpdateGaiaCookie();
+
+  EXPECT_EQ(first_update_time, GetGaiaLastUpdateTime());
 }
 
 TEST_F(AccountConsistencyServiceTest, SetGaiaCookieUpdateAtUpdateTime) {
-  base::Time first_gaia_cookie_update = SimulateUpdateGaiaCookie();
+  SimulateUpdateGaiaCookie();
 
   // Advance clock past one-hour Gaia update time.
   task_environment_.FastForwardBy(base::TimeDelta::FromHours(2));
+  const base::Time second_update_time = base::Time::Now();
+  SimulateUpdateGaiaCookie();
+
+  EXPECT_EQ(second_update_time, GetGaiaLastUpdateTime());
+}
+
+// Ensures that set and remove cookie operations are handled in the order
+// they are called resulting in no cookies.
+TEST_F(AccountConsistencyServiceTest, DeleteChromeConnectedCookiesAfterSet) {
+  SignIn();
+
+  // URLs must pass IsGoogleDomainURL and not duplicate sign-in domains
+  // |kGoogleDomain| or |kYouTubeDomain| otherwise they will not be reset since
+  // it is before the update time. Add multiple URLs to test for race conditions
+  // with remove call.
+  account_consistency_service_->SetChromeConnectedCookieWithDomains(
+      {"google.ca", "google.fr", kCountryGoogleDomain});
+  account_consistency_service_->RemoveAllChromeConnectedCookies(
+      base::OnceClosure());
+
+  WaitUntilAllCookieRequestsAreApplied();
+  CheckNoChromeConnectedCookies();
+}
+
+// Ensures that set and remove cookie operations are handled in the order
+// they are called resulting in one cookie.
+TEST_F(AccountConsistencyServiceTest, SetChromeConnectedCookiesAfterDelete) {
+  SignIn();
 
-  // The second update will send a cookie request, since update time is not
-  // considered for the call.
-  base::Time second_gaia_cookie_update = SimulateUpdateGaiaCookie();
-  EXPECT_GT(second_gaia_cookie_update, first_gaia_cookie_update);
+  // URLs must pass IsGoogleDomainURL and not duplicate sign-in domains
+  // |kGoogleDomain| or |kYouTubeDomain| otherwise they will not be reset since
+  // it is before the update time. Add multiple URLs to test for race conditions
+  // with remove call.
+  account_consistency_service_->SetChromeConnectedCookieWithDomains(
+      {"google.ca", "google.fr", kCountryGoogleDomain});
+  account_consistency_service_->RemoveAllChromeConnectedCookies(
+      base::OnceClosure());
+  account_consistency_service_->SetChromeConnectedCookieWithDomains(
+      {"google.ca"});
+
+  WaitUntilAllCookieRequestsAreApplied();
+  CheckDomainHasChromeConnectedCookie("google.ca");
 }

ios/chrome/browser/signin/ios_chrome_signin_client.mm

@@ -86,5 +86,6 @@ std::unique_ptr<GaiaAuthFetcher> IOSChromeSigninClient::CreateGaiaAuthFetcher(
 void IOSChromeSigninClient::PreGaiaLogout(base::OnceClosure callback) {
   AccountConsistencyService* accountConsistencyService =
       ios::AccountConsistencyServiceFactory::GetForBrowserState(browser_state_);
-  accountConsistencyService->RemoveChromeConnectedCookies(std::move(callback));
+  accountConsistencyService->RemoveAllChromeConnectedCookies(
+      std::move(callback));
 }