Account reconcilor: Use cookie store cookie changed subscription.

This CL changes the account reconcilor to use the new cookie store
API that allows clients to adds callback for cookie changed events.
It adds logic to the chrome sign-in client to register for cookie
changed events on the IO thread and call the cookie changed callback
on the main thread.

Note that in order to access the cookie store, we need to use the
URL context which can only be used on the IO thread. Subscribing for
cookie change events requires thus a thread jump to the IO thread on
all platforms.

BUG=NONE

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

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

chrome/browser/signin/chrome_signin_client.cc

@@ -8,22 +8,21 @@
 #include "base/guid.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/net/chrome_cookie_notification_details.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/local_auth.h"
+#include "chrome/browser/signin/signin_cookie_changed_subscription.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
 #include "chrome/common/chrome_version_info.h"
 #include "components/metrics/metrics_service.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "components/signin/core/common/signin_pref_names.h"
 #include "components/signin/core/common/signin_switches.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/child_process_host.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "url/gurl.h"
 
 #if defined(ENABLE_MANAGED_USERS)
@@ -49,14 +48,9 @@ const char kGoogleAccountsUrl[] = "https://accounts.google.com";
 
 ChromeSigninClient::ChromeSigninClient(Profile* profile)
     : profile_(profile), signin_host_id_(ChildProcessHost::kInvalidUniqueID) {
-  callbacks_.set_removal_callback(
-    base::Bind(&ChromeSigninClient::UnregisterForCookieChangedNotification,
-               base::Unretained(this)));
 }
 
 ChromeSigninClient::~ChromeSigninClient() {
-  UnregisterForCookieChangedNotification();
-
   std::set<RenderProcessHost*>::iterator i;
   for (i = signin_hosts_observed_.begin(); i != signin_hosts_observed_.end();
        ++i) {
@@ -200,12 +194,16 @@ base::Time ChromeSigninClient::GetInstallDate() {
       g_browser_process->metrics_service()->GetInstallDate());
 }
 
-scoped_ptr<SigninClient::CookieChangedCallbackList::Subscription>
+scoped_ptr<SigninClient::CookieChangedSubscription>
 ChromeSigninClient::AddCookieChangedCallback(
-    const CookieChangedCallback& callback) {
-  scoped_ptr<SigninClient::CookieChangedCallbackList::Subscription>
-    subscription = callbacks_.Add(callback);
-  RegisterForCookieChangedNotification();
+    const GURL& url,
+    const std::string& name,
+    const net::CookieStore::CookieChangedCallback& callback) {
+  scoped_refptr<net::URLRequestContextGetter> context_getter =
+      profile_->GetRequestContext();
+  DCHECK(context_getter.get());
+  scoped_ptr<SigninCookieChangedSubscription> subscription(
+      new SigninCookieChangedSubscription(context_getter, url, name, callback));
   return subscription.Pass();
 }
 
@@ -218,41 +216,3 @@ void ChromeSigninClient::GoogleSigninSucceeded(const std::string& account_id,
     chrome::SetLocalAuthCredentials(profile_, password);
 #endif
 }
-
-void ChromeSigninClient::Observe(int type,
-                                 const content::NotificationSource& source,
-                                 const content::NotificationDetails& details) {
-  switch (type) {
-    case chrome::NOTIFICATION_COOKIE_CHANGED: {
-      DCHECK(!callbacks_.empty());
-      const net::CanonicalCookie* cookie =
-          content::Details<ChromeCookieDetails>(details).ptr()->cookie;
-      callbacks_.Notify(cookie);
-      break;
-    }
-    default:
-      NOTREACHED();
-      break;
-  }
-}
-
-void ChromeSigninClient::RegisterForCookieChangedNotification() {
-  if (callbacks_.empty())
-    return;
-  content::Source<Profile> source(profile_);
-  if (!registrar_.IsRegistered(
-      this, chrome::NOTIFICATION_COOKIE_CHANGED, source))
-  registrar_.Add(this, chrome::NOTIFICATION_COOKIE_CHANGED, source);
-}
-
-void ChromeSigninClient::UnregisterForCookieChangedNotification() {
-  if (!callbacks_.empty())
-    return;
-  // Note that it's allowed to call this method multiple times without an
-  // intervening call to |RegisterForCookieChangedNotification()|.
-  content::Source<Profile> source(profile_);
-  if (!registrar_.IsRegistered(
-      this, chrome::NOTIFICATION_COOKIE_CHANGED, source))
-    return;
-  registrar_.Remove(this, chrome::NOTIFICATION_COOKIE_CHANGED, source);
-}

chrome/browser/signin/chrome_signin_client.h

@@ -8,15 +8,12 @@
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "components/signin/core/browser/signin_client.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/render_process_host_observer.h"
 
 class CookieSettings;
 class Profile;
 
 class ChromeSigninClient : public SigninClient,
-                           public content::NotificationObserver,
                            public content::RenderProcessHostObserver {
  public:
   explicit ChromeSigninClient(Profile* profile);
@@ -57,27 +54,16 @@ class ChromeSigninClient : public SigninClient,
   // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">
   // If version information is unavailable, returns "invalid."
   std::string GetProductVersion() override;
-  scoped_ptr<CookieChangedCallbackList::Subscription> AddCookieChangedCallback(
-      const CookieChangedCallback& callback) override;
+  scoped_ptr<CookieChangedSubscription> AddCookieChangedCallback(
+      const GURL& url,
+      const std::string& name,
+      const net::CookieStore::CookieChangedCallback& callback) override;
   void GoogleSigninSucceeded(const std::string& account_id,
                              const std::string& username,
                              const std::string& password) override;
 
-  // content::NotificationObserver implementation.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
  private:
-  void RegisterForCookieChangedNotification();
-  void UnregisterForCookieChangedNotification();
-
   Profile* profile_;
-  content::NotificationRegistrar registrar_;
-
-  // The callbacks that will be called when notifications about cookie changes
-  // are received.
-  base::CallbackList<void(const net::CanonicalCookie* cookie)> callbacks_;
 
   // See SetSigninProcess. Tracks the currently active signin process
   // by ID, if there is one.

chrome/browser/signin/signin_cookie_changed_subscription.cc

+// Copyright 2014 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 "chrome/browser/signin/signin_cookie_changed_subscription.h"
+
+#include "net/cookies/cookie_store.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+
+SigninCookieChangedSubscription::SubscriptionHolder::SubscriptionHolder() {
+}
+
+SigninCookieChangedSubscription::SubscriptionHolder::~SubscriptionHolder() {
+}
+
+SigninCookieChangedSubscription::SigninCookieChangedSubscription(
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    const GURL& url,
+    const std::string& name,
+    const net::CookieStore::CookieChangedCallback& callback)
+    : context_getter_(context_getter),
+      subscription_holder_io_(new SubscriptionHolder),
+      callback_(callback) {
+  RegisterForCookieChangedNotifications(url, name);
+}
+
+SigninCookieChangedSubscription::~SigninCookieChangedSubscription() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner =
+      context_getter_->GetNetworkTaskRunner();
+  if (network_task_runner->BelongsToCurrentThread()) {
+    subscription_holder_io_.reset();
+  } else {
+    network_task_runner->DeleteSoon(FROM_HERE,
+                                    subscription_holder_io_.release());
+  }
+}
+
+void SigninCookieChangedSubscription::RegisterForCookieChangedNotifications(
+    const GURL& url,
+    const std::string& name) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // The cookie store can only be accessed from the context getter which lives
+  // on the network thread. As |AddCookieChangedCallback| is called from the
+  // main thread, a thread jump is needed to register for cookie changed
+  // notifications.
+  net::CookieStore::CookieChangedCallback run_on_current_thread_callback =
+      base::Bind(&SigninCookieChangedSubscription::RunAsyncOnCookieChanged,
+                 base::MessageLoopProxy::current(),
+                 this->AsWeakPtr());
+  base::Closure register_closure =
+      base::Bind(&RegisterForCookieChangesOnIOThread,
+                 context_getter_,
+                 url,
+                 name,
+                 run_on_current_thread_callback,
+                 base::Unretained(subscription_holder_io_.get()));
+  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner =
+      context_getter_->GetNetworkTaskRunner();
+  if (network_task_runner->BelongsToCurrentThread()) {
+    register_closure.Run();
+  } else {
+    network_task_runner->PostTask(FROM_HERE, register_closure);
+  }
+}
+
+// static
+void SigninCookieChangedSubscription::RegisterForCookieChangesOnIOThread(
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    const GURL url,
+    const std::string name,
+    const net::CookieStore::CookieChangedCallback callback,
+    SigninCookieChangedSubscription::SubscriptionHolder*
+        out_subscription_holder) {
+  DCHECK(out_subscription_holder);
+  net::CookieStore* cookie_store =
+      context_getter->GetURLRequestContext()->cookie_store();
+  DCHECK(cookie_store);
+  out_subscription_holder->subscription =
+      cookie_store->AddCallbackForCookie(url, name, callback);
+}
+
+// static
+void SigninCookieChangedSubscription::RunAsyncOnCookieChanged(
+    scoped_refptr<base::TaskRunner> proxy,
+    base::WeakPtr<SigninCookieChangedSubscription> subscription,
+    const net::CanonicalCookie& cookie,
+    bool removed) {
+  proxy->PostTask(FROM_HERE,
+                  base::Bind(&SigninCookieChangedSubscription::OnCookieChanged,
+                             subscription,
+                             cookie,
+                             removed));
+}
+
+void SigninCookieChangedSubscription::OnCookieChanged(
+    const net::CanonicalCookie& cookie,
+    bool removed) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!callback_.is_null()) {
+    callback_.Run(cookie, removed);
+  }
+}

chrome/browser/signin/signin_cookie_changed_subscription.h

+// Copyright 2014 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 CHROME_BROWSER_SIGNIN_SIGNIN_COOKIE_CHANGED_SUBSCRIPTION_H_
+#define CHROME_BROWSER_SIGNIN_SIGNIN_COOKIE_CHANGED_SUBSCRIPTION_H_
+
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
+#include "components/signin/core/browser/signin_client.h"
+#include "net/url_request/url_request_context_getter.h"
+
+// The subscription for a cookie changed events. This class lives on the
+// main thread.
+class SigninCookieChangedSubscription
+    : public SigninClient::CookieChangedSubscription,
+      public base::SupportsWeakPtr<SigninCookieChangedSubscription> {
+ public:
+  // Creates a cookie changed subscription and registers for cookie changed
+  // events.
+  SigninCookieChangedSubscription(
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      const GURL& url,
+      const std::string& name,
+      const net::CookieStore::CookieChangedCallback& callback);
+  ~SigninCookieChangedSubscription() override;
+
+ private:
+  // Holder of a cookie store cookie changed subscription.
+  struct SubscriptionHolder {
+    scoped_ptr<net::CookieStore::CookieChangedSubscription> subscription;
+    SubscriptionHolder();
+    ~SubscriptionHolder();
+  };
+
+  // Adds a callback for cookie changed events. This method is called on the
+  // network thread, so it is safe to access the cookie store.
+  static void RegisterForCookieChangesOnIOThread(
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      const GURL url,
+      const std::string name,
+      const net::CookieStore::CookieChangedCallback callback,
+      SigninCookieChangedSubscription::SubscriptionHolder*
+          out_subscription_holder);
+
+  void RegisterForCookieChangedNotifications(const GURL& url,
+                                             const std::string& name);
+
+  // Posts a task on the |proxy| task runner that calls |OnCookieChanged| on
+  // |subscription|.
+  // Note that this method is called on the network thread, so |subscription|
+  // must not be used here, it is only passed around.
+  static void RunAsyncOnCookieChanged(
+      scoped_refptr<base::TaskRunner> proxy,
+      base::WeakPtr<SigninCookieChangedSubscription> subscription,
+      const net::CanonicalCookie& cookie,
+      bool removed);
+
+  // Handler for cookie changed events.
+  void OnCookieChanged(const net::CanonicalCookie& cookie, bool removed);
+
+  // The context getter.
+  scoped_refptr<net::URLRequestContextGetter> context_getter_;
+
+  // The holder of a cookie changed subscription. Must be destroyed on the
+  // network thread.
+  scoped_ptr<SubscriptionHolder> subscription_holder_io_;
+
+  // Callback to be run on cookie changed events.
+  net::CookieStore::CookieChangedCallback callback_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(SigninCookieChangedSubscription);
+};
+
+#endif  // CHROME_BROWSER_SIGNIN_SIGNIN_COOKIE_CHANGED_SUBSCRIPTION_H_

chrome/chrome_browser.gypi

@@ -1152,6 +1152,8 @@
       'browser/signin/profile_oauth2_token_service_factory.h',
       'browser/signin/screenlock_bridge.cc',
       'browser/signin/screenlock_bridge.h',
+      'browser/signin/signin_cookie_changed_subscription.cc',
+      'browser/signin/signin_cookie_changed_subscription.h',
       'browser/signin/signin_manager_factory.cc',
       'browser/signin/signin_manager_factory.h',
       'browser/signin/signin_names_io_thread.cc',

components/signin/core/browser/about_signin_internals.cc

@@ -209,8 +209,10 @@ void AboutSigninInternals::Initialize(SigninClient* client) {
   signin_manager_->AddSigninDiagnosticsObserver(this);
   token_service_->AddDiagnosticsObserver(this);
   cookie_changed_subscription_ = client_->AddCookieChangedCallback(
-     base::Bind(&AboutSigninInternals::OnCookieChanged,
-     base::Unretained(this)));
+      GaiaUrls::GetInstance()->gaia_url(),
+      "LSID",
+      base::Bind(&AboutSigninInternals::OnCookieChanged,
+                 base::Unretained(this)));
 }
 
 void AboutSigninInternals::Shutdown() {
@@ -285,12 +287,11 @@ void AboutSigninInternals::OnAuthenticationResultReceived(std::string status) {
   NotifySigninValueChanged(AUTHENTICATION_RESULT_RECEIVED, status);
 }
 
-void AboutSigninInternals::OnCookieChanged(
-    const net::CanonicalCookie* cookie) {
-  if (cookie->Name() == "LSID" &&
-      cookie->Domain() == GaiaUrls::GetInstance()->gaia_url().host() &&
-      cookie->IsSecure() &&
-      cookie->IsHttpOnly()) {
+void AboutSigninInternals::OnCookieChanged(const net::CanonicalCookie& cookie,
+                                           bool removed) {
+  DCHECK_EQ("LSID", cookie.Name());
+  DCHECK_EQ(GaiaUrls::GetInstance()->gaia_url().host(), cookie.Domain());
+  if (cookie.IsSecure() && cookie.IsHttpOnly()) {
     GetCookieAccountsAsync();
   }
 }

components/signin/core/browser/about_signin_internals.h

@@ -182,7 +182,7 @@ class AboutSigninInternals
 
   // Called when a cookie changes. If the cookie relates to a GAIA LSID cookie,
   // then we call ListAccounts and update the UI element.
-  void OnCookieChanged(const net::CanonicalCookie* cookie);
+  void OnCookieChanged(const net::CanonicalCookie& cookie, bool removed);
 
   // Weak pointer to the token service.
   ProfileOAuth2TokenService* token_service_;
@@ -202,7 +202,7 @@ class AboutSigninInternals
 
   ObserverList<Observer> signin_observers_;
 
-  scoped_ptr<SigninClient::CookieChangedCallbackList::Subscription>
+  scoped_ptr<SigninClient::CookieChangedSubscription>
       cookie_changed_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(AboutSigninInternals);

components/signin/core/browser/account_reconcilor.cc

@@ -121,6 +121,8 @@ void AccountReconcilor::RegisterForCookieChanges() {
   // go off in some embedders on reauth (e.g., ChromeSigninClient).
   UnregisterForCookieChanges();
   cookie_changed_subscription_ = client_->AddCookieChangedCallback(
+      GaiaUrls::GetInstance()->gaia_url(),
+      "LSID",
       base::Bind(&AccountReconcilor::OnCookieChanged, base::Unretained(this)));
 }
 
@@ -160,10 +162,11 @@ bool AccountReconcilor::IsProfileConnected() {
   return signin_manager_->IsAuthenticated();
 }
 
-void AccountReconcilor::OnCookieChanged(const net::CanonicalCookie* cookie) {
-  if (cookie->Name() == "LSID" &&
-      cookie->Domain() == GaiaUrls::GetInstance()->gaia_url().host() &&
-      cookie->IsSecure() && cookie->IsHttpOnly()) {
+void AccountReconcilor::OnCookieChanged(const net::CanonicalCookie& cookie,
+                                        bool removed) {
+  DCHECK_EQ("LSID", cookie.Name());
+  DCHECK_EQ(GaiaUrls::GetInstance()->gaia_url().host(), cookie.Domain());
+  if (cookie.IsSecure() && cookie.IsHttpOnly()) {
     VLOG(1) << "AccountReconcilor::OnCookieChanged: LSID changed";
 
     // It is possible that O2RT is not available at this moment.

components/signin/core/browser/account_reconcilor.h

@@ -135,7 +135,7 @@ class AccountReconcilor : public KeyedService,
   // Note internally that this |account_id| is added to the cookie jar.
   bool MarkAccountAsAddedToCookie(const std::string& account_id);
 
-  void OnCookieChanged(const net::CanonicalCookie* cookie);
+  void OnCookieChanged(const net::CanonicalCookie& cookie, bool removed);
 
   // Overriden from GaiaAuthConsumer.
   void OnListAccountsSuccess(const std::string& data) override;
@@ -194,7 +194,7 @@ class AccountReconcilor : public KeyedService,
 
   std::deque<GetAccountsFromCookieCallback> get_gaia_accounts_callbacks_;
 
-  scoped_ptr<SigninClient::CookieChangedCallbackList::Subscription>
+  scoped_ptr<SigninClient::CookieChangedSubscription>
       cookie_changed_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(AccountReconcilor);

components/signin/core/browser/signin_client.h

@@ -10,13 +10,14 @@
 #include "base/time/time.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/signin/core/browser/webdata/token_web_data.h"
+#include "net/cookies/cookie_store.h"
+#include "url/gurl.h"
 
 class PrefService;
 class SigninManagerBase;
 class TokenWebData;
 
 namespace net {
-class CanonicalCookie;
 class URLRequestContextGetter;
 }
 
@@ -32,11 +33,11 @@ class ProfileOAuth2TokenServiceIOSProvider;
 // embedder.
 class SigninClient : public KeyedService {
  public:
-  typedef base::Callback<void(const net::CanonicalCookie* cookie)>
-      CookieChangedCallback;
-
-  typedef base::CallbackList<void(const net::CanonicalCookie* cookie)>
-      CookieChangedCallbackList;
+  // The subcription for cookie changed notifications.
+  class CookieChangedSubscription {
+   public:
+    virtual ~CookieChangedSubscription() {};
+  };
 
   ~SigninClient() override {}
 
@@ -70,12 +71,14 @@ class SigninClient : public KeyedService {
   // Signin component is being used.
   virtual std::string GetProductVersion() = 0;
 
-  // Adds or removes a callback that should be called when a cookie changes.
-  // TODO(blundell): Eliminate this interface in favor of having core signin
-  // code observe cookie changes once //chrome/browser/net has been
-  // componentized.
-  virtual scoped_ptr<CookieChangedCallbackList::Subscription>
-      AddCookieChangedCallback(const CookieChangedCallback& callback) = 0;
+  // Adds a callback to be called each time a cookie for |url| with name |name|
+  // changes.
+  // Note that |callback| will always be called on the thread that
+  // |AddCookieChangedCallback| was called on.
+  virtual scoped_ptr<CookieChangedSubscription> AddCookieChangedCallback(
+      const GURL& url,
+      const std::string& name,
+      const net::CookieStore::CookieChangedCallback& callback) = 0;
 
   // Called when Google signin has succeeded.
   virtual void GoogleSigninSucceeded(const std::string& account_id,

components/signin/core/browser/test_signin_client.cc

@@ -79,10 +79,13 @@ bool TestSigninClient::ShouldMergeSigninCredentialsIntoCookieJar() {
   return true;
 }
 
-scoped_ptr<SigninClient::CookieChangedCallbackList::Subscription>
+scoped_ptr<SigninClient::CookieChangedSubscription>
 TestSigninClient::AddCookieChangedCallback(
-    const SigninClient::CookieChangedCallback& callback) {
-  return cookie_callbacks_.Add(callback);
+    const GURL& url,
+    const std::string& name,
+    const net::CookieStore::CookieChangedCallback& callback) {
+  return scoped_ptr<SigninClient::CookieChangedSubscription>(
+      new SigninClient::CookieChangedSubscription);
 }
 
 #if defined(OS_IOS)

components/signin/core/browser/test_signin_client.h

@@ -65,9 +65,12 @@ class TestSigninClient : public SigninClient {
   // Returns true.
   bool ShouldMergeSigninCredentialsIntoCookieJar() override;
 
-  // Does nothing.
-  scoped_ptr<CookieChangedCallbackList::Subscription> AddCookieChangedCallback(
-      const CookieChangedCallback& callback) override;
+  // Registers |callback| and returns the subscription.
+  // Note that |callback| will never be called.
+  scoped_ptr<SigninClient::CookieChangedSubscription> AddCookieChangedCallback(
+      const GURL& url,
+      const std::string& name,
+      const net::CookieStore::CookieChangedCallback& callback) override;
 
 #if defined(OS_IOS)
   ios::FakeProfileOAuth2TokenServiceIOSProvider* GetIOSProviderAsFake();
@@ -89,8 +92,6 @@ class TestSigninClient : public SigninClient {
   scoped_refptr<net::URLRequestContextGetter> request_context_;
   scoped_refptr<TokenWebData> database_;
   int signin_host_id_;
-  CookieChangedCallbackList cookie_callbacks_;
-
   PrefService* pref_service_;
 
 #if defined(OS_IOS)