Refactor ProfileOAuth2TokenServiceRequest into OAuth2TokenServiceRequest

google_apis/gaia/oauth2_token_service_request.cc

-// Copyright 2012 The Chromium Authors. All rights reserved.
+// 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/profile_oauth2_token_service_request.h"
+#include "google_apis/gaia/oauth2_token_service_request.h"
 
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
-#include "chrome/browser/signin/signin_manager_factory.h"
-#include "components/signin/core/browser/profile_oauth2_token_service.h"
-#include "components/signin/core/browser/signin_manager.h"
-#include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "google_apis/gaia/oauth2_access_token_consumer.h"
 
-class ProfileOAuth2TokenServiceRequest::Core
-    : public base::RefCountedThreadSafe<ProfileOAuth2TokenServiceRequest::Core>,
-      public OAuth2TokenService::Consumer {
+OAuth2TokenServiceRequest::TokenServiceProvider::TokenServiceProvider() {
+}
+
+OAuth2TokenServiceRequest::TokenServiceProvider::~TokenServiceProvider() {
+}
+
+// Core serves as the base class for OAuth2TokenService operations.  Each
+// operation should be modeled as a derived type.
+//
+// Core is used like this:
+//
+// 1. Constructed on owner thread.
+//
+// 2. Start() is called on owner thread, which calls StartOnTokenServiceThread()
+// on token service thread.
+//
+// 3. Request is executed.
+//
+// 4. Stop() is called on owner thread, which calls StopOnTokenServiceThread()
+// on token service thread.
+//
+// 5. Core is destroyed on owner thread.
+class OAuth2TokenServiceRequest::Core
+    : public base::NonThreadSafe,
+      public base::RefCountedThreadSafe<OAuth2TokenServiceRequest::Core> {
  public:
   // Note the thread where an instance of Core is constructed is referred to as
-  // the "owner thread" here. This will be the thread of |owner_task_runner_|.
-  Core(Profile* profile,
-       ProfileOAuth2TokenServiceRequest* owner);
-  // Starts fetching an OAuth2 access token for |account_id| and |scopes|. It
-  // should be called on the owner thread.
-  void Start(
-      const std::string& account_id,
-      const OAuth2TokenService::ScopeSet& scopes);
-  // Stops the OAuth2 access token fetching. It should be called on the owner
-  // thread.
+  // the "owner thread" here.
+  Core(OAuth2TokenServiceRequest* owner, TokenServiceProvider* provider);
+
+  // Starts the core.  Must be called on the owner thread.
+  void Start();
+
+  // Stops the core.  Must be called on the owner thread.
   void Stop();
 
-  OAuth2TokenService::Request* request();
+  // Returns true if this object has been stopped.  Must be called on the owner
+  // thread.
+  bool IsStopped() const;
 
-  // OAuth2TokenService::Consumer. It should be called on the UI thread.
+ protected:
+  // Core must be destroyed on the owner thread.  If data members must be
+  // cleaned up or destroyed on the token service thread, do so in the
+  // StopOnTokenServiceThread method.
+  virtual ~Core();
+
+  // Called on the token service thread.
+  virtual void StartOnTokenServiceThread() = 0;
+
+  // Called on the token service thread.
+  virtual void StopOnTokenServiceThread() = 0;
+
+  base::SingleThreadTaskRunner* token_service_task_runner();
+  OAuth2TokenService* token_service();
+  OAuth2TokenServiceRequest* owner();
+
+ private:
+  friend class base::RefCountedThreadSafe<OAuth2TokenServiceRequest::Core>;
+
+  scoped_refptr<base::SingleThreadTaskRunner> token_service_task_runner_;
+  OAuth2TokenServiceRequest* owner_;
+  TokenServiceProvider* provider_;
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+OAuth2TokenServiceRequest::Core::Core(OAuth2TokenServiceRequest* owner,
+                                      TokenServiceProvider* provider)
+    : owner_(owner), provider_(provider) {
+  DCHECK(owner_);
+  DCHECK(provider_);
+  token_service_task_runner_ = provider_->GetTokenServiceTaskRunner();
+  DCHECK(token_service_task_runner_);
+}
+
+OAuth2TokenServiceRequest::Core::~Core() {
+}
+
+void OAuth2TokenServiceRequest::Core::Start() {
+  DCHECK(CalledOnValidThread());
+  token_service_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OAuth2TokenServiceRequest::Core::StartOnTokenServiceThread,
+                 this));
+}
+
+void OAuth2TokenServiceRequest::Core::Stop() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!IsStopped());
+
+  // Detaches |owner_| from this instance so |owner_| will be called back only
+  // if |Stop()| has never been called.
+  owner_ = NULL;
+  token_service_task_runner_->PostTask(

[blundell, 2014/06/04 13:55:14]

There's a bug here. This object requires that it's deleted on the thread it was
created on. However, this PostTask() call is creating a reference to |this| that
lives on |token_service_task_runner_|. There is no ordering guaranteed between
the elimination of the last reference to |this| on this thread and the
elimination of the reference on |token_service_task_runner_|. In fact , in the
existing code you're almost certain to have the last reference on this thread be
eliminated first, since it's eliminated immediately after calling |Stop()|.

An easy but crude solution to this problem would be to change this |PostTask()|
to a |PostTaskAndReply()| with the reply being a do-nothing instance method. I
don't know this code well enough to know what the best solution is.

[maniscalco, 2014/06/04 16:41:38]

Good catch.

If we use PostTaskAndReply, the "owner" thread would be blocked waiting for the
token service thread to eventually execute StopOnTokenServiceThread.  I don't
have a feel for how "bad" that would be.  If would be nice to avoid having one
thread block on the other.

Another option is to destroy the Core on the token service thread (probably
using AddRef/ReleaseSoon).  This means Core would be created on the owner
thread, but destroyed on the token service thread.  Given *Core's data members
and base classes, I think it's doable.  I don't love creating on one destroying
on the other, but it would allow us to destroy the OAT2TSR without waiting for
the token service thread.  I'll write a patch to see what it would look like.


On 2014/06/04 13:55:14, blundell wrote:

[maniscalco, 2014/06/04 17:29:42]

blundell, I want to make sure I understand the PostTaskAndReply approach you
described.

I assumed you were talking about changing Core::Stop such that it would not
return until it StopOnTokenServiceThread had finished executing (e.g. use
WaitableEvent).  However, after re-reading your comment, I think you may have
meant something different.  I need to read up on PTAR to make sure I understand
how it could be used to fix this race condition. 

On 2014/06/04 16:41:38, maniscalco wrote:

+      FROM_HERE,
+      base::Bind(&OAuth2TokenServiceRequest::Core::StopOnTokenServiceThread,
+                 this));
+}
+
+bool OAuth2TokenServiceRequest::Core::IsStopped() const {
+  DCHECK(CalledOnValidThread());
+  return owner_ == NULL;
+}
+
+base::SingleThreadTaskRunner*
+OAuth2TokenServiceRequest::Core::token_service_task_runner() {
+  return token_service_task_runner_;
+}
+
+OAuth2TokenService* OAuth2TokenServiceRequest::Core::token_service() {
+  DCHECK(token_service_task_runner_->BelongsToCurrentThread());
+  return provider_->GetTokenService();
+}
+
+OAuth2TokenServiceRequest* OAuth2TokenServiceRequest::Core::owner() {
+  DCHECK(CalledOnValidThread());
+  return owner_;
+}
+
+namespace {
+
+// An implementation of Core for getting an access token.
+class RequestCore : public OAuth2TokenServiceRequest::Core,
+                    public OAuth2TokenService::Consumer {
+ public:
+  RequestCore(OAuth2TokenServiceRequest* owner,
+              OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+              OAuth2TokenService::Consumer* consumer,
+              const std::string& account_id,
+              const OAuth2TokenService::ScopeSet& scopes);
+
+  // OAuth2TokenService::Consumer.  Must be called on the token service thread.
   virtual void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
                                  const std::string& access_token,
                                  const base::Time& expiration_time) OVERRIDE;
   virtual void OnGetTokenFailure(const OAuth2TokenService::Request* request,
                                  const GoogleServiceAuthError& error) OVERRIDE;
 
  private:
-  friend class
-      base::RefCountedThreadSafe<ProfileOAuth2TokenServiceRequest::Core>;
+  friend class base::RefCountedThreadSafe<RequestCore>;
 
-  // Note this can be destructed on the owner thread or on the UI thread,
-  // depending on the reference count.
-  virtual ~Core();
+  // Must be destroyed on the owner thread.
+  virtual ~RequestCore();
 
-  // Starts an OAuth2TokenService::Request on the UI thread.
-  void StartOnUIThread(
-      const std::string& account_id,
-      const OAuth2TokenService::ScopeSet& scopes);
-  // Stops the OAuth2TokenService::Request on the UI thread.
-  void StopOnUIThread();
+  // Core implementation.
+  virtual void StartOnTokenServiceThread() OVERRIDE;
+  virtual void StopOnTokenServiceThread() OVERRIDE;
 
-  // Method posted to the owner thread to call back |owner_|. Note when this
-  // posted task is actually running on the owner thread, it is possible that
-  // |owner_| has been reset NULL.
   void InformOwnerOnGetTokenSuccess(std::string access_token,
                                     base::Time expiration_time);
   void InformOwnerOnGetTokenFailure(GoogleServiceAuthError error);
 
-  // The profile with which this instance was initialized.
-  Profile* const profile_;
-
-  // The object to call back when fetching completes. |owner_| should be
-  // called back only on the owner thread.
-  ProfileOAuth2TokenServiceRequest* owner_;
-  // Task runner on which |owner_| should be called back.
   scoped_refptr<base::SingleThreadTaskRunner> owner_task_runner_;
+  OAuth2TokenService::Consumer* const consumer_;
+  std::string account_id_;
+  OAuth2TokenService::ScopeSet scopes_;
 
   // OAuth2TokenService request for fetching OAuth2 access token; it should be
-  // created, reset and accessed only on the UI thread.
+  // created, reset and accessed only on the token service thread.
   scoped_ptr<OAuth2TokenService::Request> request_;
 
-  DISALLOW_COPY_AND_ASSIGN(Core);
+  DISALLOW_COPY_AND_ASSIGN(RequestCore);
 };
 
-ProfileOAuth2TokenServiceRequest::Core::Core(
-    Profile* profile,
-    ProfileOAuth2TokenServiceRequest* owner)
-    : OAuth2TokenService::Consumer("oauth2_token_service"),
-      profile_(profile),
-      owner_(owner),
-      owner_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
-  DCHECK(profile);
-  DCHECK(owner);
+RequestCore::RequestCore(
+    OAuth2TokenServiceRequest* owner,
+    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    OAuth2TokenService::Consumer* consumer,
+    const std::string& account_id,
+    const OAuth2TokenService::ScopeSet& scopes)
+    : OAuth2TokenServiceRequest::Core(owner, provider),
+      OAuth2TokenService::Consumer("oauth2_token_service"),
+      owner_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      consumer_(consumer),
+      account_id_(account_id),
+      scopes_(scopes) {
+  DCHECK(consumer_);
+  DCHECK(!account_id_.empty());
+  DCHECK(!scopes_.empty());
 }
 
-ProfileOAuth2TokenServiceRequest::Core::~Core() {
+RequestCore::~RequestCore() {
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::Start(
-    const std::string& account_id,
-    const OAuth2TokenService::ScopeSet& scopes) {
-  DCHECK(owner_task_runner_->BelongsToCurrentThread());
+void RequestCore::StartOnTokenServiceThread() {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  request_ = token_service()->StartRequest(account_id_, scopes_, this).Pass();
+}
 
-  if (content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
-    StartOnUIThread(account_id, scopes);
-  } else {
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&ProfileOAuth2TokenServiceRequest::Core::StartOnUIThread,
-                   this, account_id, scopes));
+void RequestCore::StopOnTokenServiceThread() {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  request_.reset();
+}
+
+void RequestCore::OnGetTokenSuccess(const OAuth2TokenService::Request* request,
+                                    const std::string& access_token,
+                                    const base::Time& expiration_time) {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  DCHECK_EQ(request_.get(), request);
+  owner_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestCore::InformOwnerOnGetTokenSuccess,
+                 this,
+                 access_token,
+                 expiration_time));
+  request_.reset();
+}
+
+void RequestCore::OnGetTokenFailure(const OAuth2TokenService::Request* request,
+                                    const GoogleServiceAuthError& error) {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  DCHECK_EQ(request_.get(), request);
+  owner_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestCore::InformOwnerOnGetTokenFailure, this, error));
+  request_.reset();
+}
+
+void RequestCore::InformOwnerOnGetTokenSuccess(std::string access_token,
+                                               base::Time expiration_time) {
+  DCHECK(CalledOnValidThread());
+  if (!IsStopped()) {
+    consumer_->OnGetTokenSuccess(owner(), access_token, expiration_time);
   }
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::Stop() {
-  DCHECK(owner_task_runner_->BelongsToCurrentThread());
-
-  // Detaches |owner_| from this instance so |owner_| will be called back only
-  // if |Stop()| has never been called.
-  owner_ = NULL;
-  if (content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
-    StopOnUIThread();
-  } else {
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&ProfileOAuth2TokenServiceRequest::Core::StopOnUIThread,
-                   this));
+void RequestCore::InformOwnerOnGetTokenFailure(GoogleServiceAuthError error) {
+  DCHECK(CalledOnValidThread());
+  if (!IsStopped()) {
+    consumer_->OnGetTokenFailure(owner(), error);
   }
 }
 
-OAuth2TokenService::Request* ProfileOAuth2TokenServiceRequest::Core::request() {
-  return request_.get();
+// An implementation of Core for invalidating an access token.
+class InvalidateCore : public OAuth2TokenServiceRequest::Core {
+ public:
+  InvalidateCore(OAuth2TokenServiceRequest* owner,
+                 OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+                 const std::string& access_token,
+                 const std::string& account_id,
+                 const OAuth2TokenService::ScopeSet& scopes);
+
+ private:
+  friend class base::RefCountedThreadSafe<InvalidateCore>;
+
+  // Must be destroyed on the owner thread.
+  virtual ~InvalidateCore();
+
+  // Core implementation.
+  virtual void StartOnTokenServiceThread() OVERRIDE;
+  virtual void StopOnTokenServiceThread() OVERRIDE;
+
+  std::string access_token_;
+  std::string account_id_;
+  OAuth2TokenService::ScopeSet scopes_;
+
+  DISALLOW_COPY_AND_ASSIGN(InvalidateCore);
+};
+
+InvalidateCore::InvalidateCore(
+    OAuth2TokenServiceRequest* owner,
+    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const std::string& access_token,
+    const std::string& account_id,
+    const OAuth2TokenService::ScopeSet& scopes)
+    : OAuth2TokenServiceRequest::Core(owner, provider),
+      access_token_(access_token),
+      account_id_(account_id),
+      scopes_(scopes) {
+  DCHECK(!access_token_.empty());
+  DCHECK(!account_id_.empty());
+  DCHECK(!scopes.empty());
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::StopOnUIThread() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  request_.reset();
+InvalidateCore::~InvalidateCore() {
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::StartOnUIThread(
-    const std::string& account_id,
-    const OAuth2TokenService::ScopeSet& scopes) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
-  ProfileOAuth2TokenService* service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
-  DCHECK(service);
-  SigninManagerBase* signin_manager =
-      SigninManagerFactory::GetForProfile(profile_);
-  DCHECK(signin_manager);
-  std::string account_id_to_use =
-      account_id.empty() ? signin_manager->GetAuthenticatedAccountId()
-                         : account_id;
-  request_.reset(
-      service->StartRequest(account_id_to_use, scopes, this).release());
+void InvalidateCore::StartOnTokenServiceThread() {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  token_service()->InvalidateToken(account_id_, scopes_, access_token_);
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::OnGetTokenSuccess(
-    const OAuth2TokenService::Request* request,
-    const std::string& access_token,
-    const base::Time& expiration_time) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  DCHECK_EQ(request_.get(), request);
-  owner_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &ProfileOAuth2TokenServiceRequest::Core::InformOwnerOnGetTokenSuccess,
-      this,
-      access_token,
-      expiration_time));
-  request_.reset();
+void InvalidateCore::StopOnTokenServiceThread() {
+  DCHECK(token_service_task_runner()->BelongsToCurrentThread());
+  // Nothing to do.
 }
 
-void ProfileOAuth2TokenServiceRequest::Core::OnGetTokenFailure(
-    const OAuth2TokenService::Request* request,
-    const GoogleServiceAuthError& error) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  DCHECK_EQ(request_.get(), request);
-  owner_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &ProfileOAuth2TokenServiceRequest::Core::InformOwnerOnGetTokenFailure,
-      this,
-      error));
-  request_.reset();
-}
+}  // namespace
 
-void ProfileOAuth2TokenServiceRequest::Core::InformOwnerOnGetTokenSuccess(
-    std::string access_token,
-    base::Time expiration_time) {
-  DCHECK(owner_task_runner_->BelongsToCurrentThread());
-
-  if (owner_)
-    owner_->consumer_->OnGetTokenSuccess(owner_, access_token, expiration_time);
-}
-
-void ProfileOAuth2TokenServiceRequest::Core::InformOwnerOnGetTokenFailure(
-    GoogleServiceAuthError error) {
-  DCHECK(owner_task_runner_->BelongsToCurrentThread());
-
-  if (owner_)
-    owner_->consumer_->OnGetTokenFailure(owner_, error);
+// static
+scoped_ptr<OAuth2TokenServiceRequest> OAuth2TokenServiceRequest::CreateAndStart(
+    TokenServiceProvider* provider,
+    const std::string& account_id,
+    const OAuth2TokenService::ScopeSet& scopes,
+    OAuth2TokenService::Consumer* consumer) {
+  scoped_ptr<OAuth2TokenServiceRequest> request(
+      new OAuth2TokenServiceRequest(account_id));
+  scoped_refptr<Core> core(
+      new RequestCore(request.get(), provider, consumer, account_id, scopes));
+  request->StartWithCore(core);
+  return request.Pass();
 }
 
 // static
-ProfileOAuth2TokenServiceRequest*
-    ProfileOAuth2TokenServiceRequest::CreateAndStart(
-        Profile* profile,
-        const std::string& account_id,
-        const OAuth2TokenService::ScopeSet& scopes,
-        OAuth2TokenService::Consumer* consumer) {
-  return new ProfileOAuth2TokenServiceRequest(profile, account_id, scopes,
-      consumer);
+void OAuth2TokenServiceRequest::InvalidateToken(
+    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const std::string& account_id,
+    const OAuth2TokenService::ScopeSet& scopes,
+    const std::string& access_token) {
+  scoped_ptr<OAuth2TokenServiceRequest> request(
+      new OAuth2TokenServiceRequest(account_id));
+  scoped_refptr<Core> core(new InvalidateCore(
+      request.get(), provider, access_token, account_id, scopes));
+  request->StartWithCore(core);
 }
 
-ProfileOAuth2TokenServiceRequest::ProfileOAuth2TokenServiceRequest(
-    Profile* profile,
-    const std::string& account_id,
-    const OAuth2TokenService::ScopeSet& scopes,
-    OAuth2TokenService::Consumer* consumer)
-    : consumer_(consumer),
-      core_(new Core(profile, this)) {
-  core_->Start(account_id, scopes);
-}
-
-ProfileOAuth2TokenServiceRequest::~ProfileOAuth2TokenServiceRequest() {
-  DCHECK(CalledOnValidThread());
+OAuth2TokenServiceRequest::~OAuth2TokenServiceRequest() {
   core_->Stop();
 }
 
-std::string ProfileOAuth2TokenServiceRequest::GetAccountId() const {
-  return core_->request()->GetAccountId();
+std::string OAuth2TokenServiceRequest::GetAccountId() const {
+  return account_id_;
 }
 
+OAuth2TokenServiceRequest::OAuth2TokenServiceRequest(
+    const std::string& account_id)
+    : account_id_(account_id) {
+  DCHECK(!account_id_.empty());
+}
+
+void OAuth2TokenServiceRequest::StartWithCore(const scoped_refptr<Core>& core) {
+  DCHECK(core);
+  core_ = core;
+  core_->Start();
+}