Fix bug where TokenServiceProvider is used after it has been destroyed.

There was a race condition where GetTokenService could be called on an
already destroyed TokenServiceProvider (see linked bug for details).
Update Core to ensure TokenServiceProvider is not destroyed out from
under the token service task runner thread.

BUG=401119

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

Cr-Commit-Position: refs/heads/master@{#289222}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289222 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/profile_sync_components_factory_impl.cc

@@ -600,7 +600,6 @@ class TokenServiceProvider
   TokenServiceProvider(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       OAuth2TokenService* token_service);
-  virtual ~TokenServiceProvider();
 
   // OAuth2TokenServiceRequest::TokenServiceProvider implementation.
   virtual scoped_refptr<base::SingleThreadTaskRunner>
@@ -608,6 +607,8 @@ class TokenServiceProvider
   virtual OAuth2TokenService* GetTokenService() OVERRIDE;
 
  private:
+  virtual ~TokenServiceProvider();
+
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   OAuth2TokenService* token_service_;
 };
@@ -646,7 +647,7 @@ ProfileSyncComponentsFactoryImpl::CreateAttachmentService(
   // signed in sync user (e.g. sync is running in "backup" mode).
   if (!user_share.sync_credentials.email.empty() &&
       !user_share.sync_credentials.scope_set.empty()) {
-    scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+    scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
         token_service_provider(new TokenServiceProvider(
             content::BrowserThread::GetMessageLoopProxyForThread(
                 content::BrowserThread::UI),
@@ -659,18 +660,18 @@ ProfileSyncComponentsFactoryImpl::CreateAttachmentService(
         url_request_context_getter_,
         user_share.sync_credentials.email,
         user_share.sync_credentials.scope_set,
-        token_service_provider.Pass()));
+        token_service_provider));
 
-    token_service_provider.reset(new TokenServiceProvider(
+    token_service_provider = new TokenServiceProvider(
         content::BrowserThread::GetMessageLoopProxyForThread(
             content::BrowserThread::UI),
-        token_service_));
+        token_service_);
     attachment_downloader = syncer::AttachmentDownloader::Create(
         sync_service_url_,
         url_request_context_getter_,
         user_share.sync_credentials.email,
         user_share.sync_credentials.scope_set,
-        token_service_provider.Pass());
+        token_service_provider);
   }
 
   scoped_ptr<syncer::AttachmentService> attachment_service(

google_apis/gaia/oauth2_token_service_request.cc

@@ -40,7 +40,8 @@ class OAuth2TokenServiceRequest::Core
  public:
   // Note the thread where an instance of Core is constructed is referred to as
   // the "owner thread" here.
-  Core(OAuth2TokenServiceRequest* owner, TokenServiceProvider* provider);
+  Core(OAuth2TokenServiceRequest* owner,
+       const scoped_refptr<TokenServiceProvider>& provider);
 
   // Starts the core.  Must be called on the owner thread.
   void Start();
@@ -75,12 +76,17 @@ class OAuth2TokenServiceRequest::Core
 
   scoped_refptr<base::SingleThreadTaskRunner> token_service_task_runner_;
   OAuth2TokenServiceRequest* owner_;
-  TokenServiceProvider* provider_;
+
+  // It is important that provider_ is destroyed on the owner thread, not the
+  // token_service_task_runner_ thread.
+  scoped_refptr<TokenServiceProvider> provider_;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
-OAuth2TokenServiceRequest::Core::Core(OAuth2TokenServiceRequest* owner,
-                                      TokenServiceProvider* provider)
+OAuth2TokenServiceRequest::Core::Core(
+    OAuth2TokenServiceRequest* owner,
+    const scoped_refptr<TokenServiceProvider>& provider)
     : owner_(owner), provider_(provider) {
   DCHECK(owner_);
   DCHECK(provider_);
@@ -149,7 +155,8 @@ class RequestCore : public OAuth2TokenServiceRequest::Core,
                     public OAuth2TokenService::Consumer {
  public:
   RequestCore(OAuth2TokenServiceRequest* owner,
-              OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+              const scoped_refptr<
+                  OAuth2TokenServiceRequest::TokenServiceProvider>& provider,
               OAuth2TokenService::Consumer* consumer,
               const std::string& account_id,
               const OAuth2TokenService::ScopeSet& scopes);
@@ -189,7 +196,8 @@ class RequestCore : public OAuth2TokenServiceRequest::Core,
 
 RequestCore::RequestCore(
     OAuth2TokenServiceRequest* owner,
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
+        provider,
     OAuth2TokenService::Consumer* consumer,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes)
@@ -260,7 +268,8 @@ void RequestCore::InformOwnerOnGetTokenFailure(GoogleServiceAuthError error) {
 class InvalidateCore : public OAuth2TokenServiceRequest::Core {
  public:
   InvalidateCore(OAuth2TokenServiceRequest* owner,
-                 OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+                 const scoped_refptr<
+                     OAuth2TokenServiceRequest::TokenServiceProvider>& provider,
                  const std::string& access_token,
                  const std::string& account_id,
                  const OAuth2TokenService::ScopeSet& scopes);
@@ -284,7 +293,8 @@ class InvalidateCore : public OAuth2TokenServiceRequest::Core {
 
 InvalidateCore::InvalidateCore(
     OAuth2TokenServiceRequest* owner,
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
+        provider,
     const std::string& access_token,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes)
@@ -314,7 +324,7 @@ void InvalidateCore::StopOnTokenServiceThread() {
 
 // static
 scoped_ptr<OAuth2TokenServiceRequest> OAuth2TokenServiceRequest::CreateAndStart(
-    TokenServiceProvider* provider,
+    const scoped_refptr<TokenServiceProvider>& provider,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
     OAuth2TokenService::Consumer* consumer) {
@@ -328,7 +338,7 @@ scoped_ptr<OAuth2TokenServiceRequest> OAuth2TokenServiceRequest::CreateAndStart(
 
 // static
 void OAuth2TokenServiceRequest::InvalidateToken(
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<TokenServiceProvider>& provider,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
     const std::string& access_token) {

google_apis/gaia/oauth2_token_service_request.h

@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/non_thread_safe.h"
@@ -23,10 +24,16 @@ class OAuth2TokenServiceRequest : public OAuth2TokenService::Request,
   class Core;
 
   // Interface for providing an OAuth2TokenService.
-  class TokenServiceProvider {
+  //
+  // Ref-counted so that OAuth2TokenServiceRequest can ensure this object isn't
+  // destroyed out from under the token service task runner thread.  Because
+  // OAuth2TokenServiceRequest has a reference, implementations of
+  // TokenServiceProvider must be capable of being destroyed on the same thread
+  // on which the OAuth2TokenServiceRequest was created.
+  class TokenServiceProvider
+      : public base::RefCountedThreadSafe<TokenServiceProvider> {
    public:
     TokenServiceProvider();
-    virtual ~TokenServiceProvider();
 
     // Returns the task runner on which the token service lives.
     //
@@ -42,12 +49,15 @@ class OAuth2TokenServiceRequest : public OAuth2TokenService::Request,
     // This method may only be called from the task runner returned by
     // |GetTokenServiceTaskRunner|.
     virtual OAuth2TokenService* GetTokenService() = 0;
+
+   protected:
+    friend class base::RefCountedThreadSafe<TokenServiceProvider>;
+    virtual ~TokenServiceProvider();
   };
 
   // Creates and starts an access token request for |account_id| and |scopes|.
   //
-  // |provider| is used to get the OAuth2TokenService and must outlive the
-  // returned request object.
+  // |provider| is used to get the OAuth2TokenService.
   //
   // |account_id| must not be empty.
   //
@@ -59,23 +69,23 @@ class OAuth2TokenServiceRequest : public OAuth2TokenService::Request,
   // network activities may not be canceled and the cache in OAuth2TokenService
   // may be populated with the fetched results.
   static scoped_ptr<OAuth2TokenServiceRequest> CreateAndStart(
-      TokenServiceProvider* provider,
+      const scoped_refptr<TokenServiceProvider>& provider,
       const std::string& account_id,
       const OAuth2TokenService::ScopeSet& scopes,
       OAuth2TokenService::Consumer* consumer);
 
   // Invalidates |access_token| for |account_id| and |scopes|.
   //
-  // |provider| is used to get the OAuth2TokenService and must outlive the
-  // returned request object.
+  // |provider| is used to get the OAuth2TokenService.
   //
   // |account_id| must not be empty.
   //
   // |scopes| must not be empty.
-  static void InvalidateToken(TokenServiceProvider* provider,
-                              const std::string& account_id,
-                              const OAuth2TokenService::ScopeSet& scopes,
-                              const std::string& access_token);
+  static void InvalidateToken(
+      const scoped_refptr<TokenServiceProvider>& provider,
+      const std::string& account_id,
+      const OAuth2TokenService::ScopeSet& scopes,
+      const std::string& access_token);
 
   virtual ~OAuth2TokenServiceRequest();
 

google_apis/gaia/oauth2_token_service_request_unittest.cc

@@ -159,6 +159,8 @@ class OAuth2TokenServiceRequestTest : public testing::Test {
     virtual OAuth2TokenService* GetTokenService() OVERRIDE;
 
    private:
+    virtual ~Provider();
+
     scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
     OAuth2TokenService* token_service_;
   };
@@ -166,7 +168,7 @@ class OAuth2TokenServiceRequestTest : public testing::Test {
   base::MessageLoop ui_loop_;
   OAuth2TokenService::ScopeSet scopes_;
   scoped_ptr<MockOAuth2TokenService> oauth2_service_;
-  scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider> provider_;
+  scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider> provider_;
   TestingOAuth2TokenServiceConsumer consumer_;
 };
 
@@ -174,8 +176,8 @@ void OAuth2TokenServiceRequestTest::SetUp() {
   scopes_.insert(kScope);
   oauth2_service_.reset(new MockOAuth2TokenService);
   oauth2_service_->AddAccount(kAccountId);
-  provider_.reset(
-      new Provider(base::MessageLoopProxy::current(), oauth2_service_.get()));
+  provider_ =
+      new Provider(base::MessageLoopProxy::current(), oauth2_service_.get());
 }
 
 void OAuth2TokenServiceRequestTest::TearDown() {
@@ -198,6 +200,9 @@ OAuth2TokenService* OAuth2TokenServiceRequestTest::Provider::GetTokenService() {
   return token_service_;
 }
 
+OAuth2TokenServiceRequestTest::Provider::~Provider() {
+}
+
 TEST_F(OAuth2TokenServiceRequestTest, CreateAndStart_Failure) {
   oauth2_service_->SetResponse(
       GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE),

sync/internal_api/attachments/attachment_downloader.cc

@@ -20,14 +20,14 @@ scoped_ptr<AttachmentDownloader> AttachmentDownloader::Create(
         url_request_context_getter,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet scopes,
-    scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
         token_service_provider) {
   return scoped_ptr<AttachmentDownloader>(
       new AttachmentDownloaderImpl(sync_service_url,
                                    url_request_context_getter,
                                    account_id,
                                    scopes,
-                                   token_service_provider.Pass()));
+                                   token_service_provider));
 }
 
 }  // namespace syncer

sync/internal_api/attachments/attachment_downloader_impl.cc

@@ -41,14 +41,14 @@ AttachmentDownloaderImpl::AttachmentDownloaderImpl(
         url_request_context_getter,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
-    scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
         token_service_provider)
     : OAuth2TokenService::Consumer("attachment-downloader-impl"),
       sync_service_url_(sync_service_url),
       url_request_context_getter_(url_request_context_getter),
       account_id_(account_id),
       oauth2_scopes_(scopes),
-      token_service_provider_(token_service_provider.Pass()) {
+      token_service_provider_(token_service_provider) {
   DCHECK(!account_id.empty());
   DCHECK(!scopes.empty());
   DCHECK(token_service_provider_);

sync/internal_api/attachments/attachment_downloader_impl_unittest.cc

@@ -99,7 +99,6 @@ class TokenServiceProvider
       base::NonThreadSafe {
  public:
   TokenServiceProvider(OAuth2TokenService* token_service);
-  virtual ~TokenServiceProvider();
 
   // OAuth2TokenService::TokenServiceProvider implementation.
   virtual scoped_refptr<base::SingleThreadTaskRunner>
@@ -107,6 +106,8 @@ class TokenServiceProvider
   virtual OAuth2TokenService* GetTokenService() OVERRIDE;
 
  private:
+  virtual ~TokenServiceProvider();
+
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   OAuth2TokenService* token_service_;
 };
@@ -182,7 +183,7 @@ void AttachmentDownloaderImplTest::SetUp() {
   url_fetcher_factory_.set_remove_fetcher_on_delete(true);
   token_service_.reset(new MockOAuth2TokenService());
   token_service_->AddAccount(kAccountId);
-  scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+  scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
       token_service_provider(new TokenServiceProvider(token_service_.get()));
 
   OAuth2TokenService::ScopeSet scopes;
@@ -192,7 +193,7 @@ void AttachmentDownloaderImplTest::SetUp() {
                                    url_request_context_getter_,
                                    kAccountId,
                                    scopes,
-                                   token_service_provider.Pass());
+                                   token_service_provider);
 }
 
 void AttachmentDownloaderImplTest::TearDown() {

sync/internal_api/attachments/attachment_uploader_impl.cc

@@ -212,13 +212,13 @@ AttachmentUploaderImpl::AttachmentUploaderImpl(
         url_request_context_getter,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
-    scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
         token_service_provider)
     : sync_service_url_(sync_service_url),
       url_request_context_getter_(url_request_context_getter),
       account_id_(account_id),
       scopes_(scopes),
-      token_service_provider_(token_service_provider.Pass()) {
+      token_service_provider_(token_service_provider) {
   DCHECK(CalledOnValidThread());
   DCHECK(!account_id.empty());
   DCHECK(!scopes.empty());

sync/internal_api/attachments/attachment_uploader_impl_unittest.cc

@@ -135,7 +135,6 @@ class TokenServiceProvider
       base::NonThreadSafe {
  public:
   TokenServiceProvider(OAuth2TokenService* token_service);
-  virtual ~TokenServiceProvider();
 
   // OAuth2TokenService::TokenServiceProvider implementation.
   virtual scoped_refptr<base::SingleThreadTaskRunner>
@@ -143,6 +142,8 @@ class TokenServiceProvider
   virtual OAuth2TokenService* GetTokenService() OVERRIDE;
 
  private:
+  virtual ~TokenServiceProvider();
+
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   OAuth2TokenService* token_service_;
 };
@@ -268,7 +269,7 @@ void AttachmentUploaderImplTest::SetUp() {
   GURL url(base::StringPrintf("http://localhost:%d/", server_.port()));
 
   token_service_.reset(new MockOAuth2TokenService);
-  scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+  scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
       token_service_provider(new TokenServiceProvider(token_service_.get()));
 
   OAuth2TokenService::ScopeSet scopes;
@@ -277,7 +278,7 @@ void AttachmentUploaderImplTest::SetUp() {
                                               url_request_context_getter_,
                                               kAccountId,
                                               scopes,
-                                              token_service_provider.Pass()));
+                                              token_service_provider));
 
   upload_callback_ = base::Bind(&AttachmentUploaderImplTest::UploadDone,
                                 base::Unretained(this));

sync/internal_api/public/attachments/attachment_downloader.h

@@ -56,7 +56,7 @@ class SYNC_EXPORT AttachmentDownloader {
           url_request_context_getter,
       const std::string& account_id,
       const OAuth2TokenService::ScopeSet scopes,
-      scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+      const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
           token_service_provider);
 };
 

sync/internal_api/public/attachments/attachment_downloader_impl.h

@@ -36,7 +36,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
           url_request_context_getter,
       const std::string& account_id,
       const OAuth2TokenService::ScopeSet& scopes,
-      scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+      const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
           token_service_provider);
   virtual ~AttachmentDownloaderImpl();
 
@@ -73,7 +73,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
 
   std::string account_id_;
   OAuth2TokenService::ScopeSet oauth2_scopes_;
-  scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+  scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
       token_service_provider_;
   scoped_ptr<OAuth2TokenService::Request> access_token_request_;
 

sync/internal_api/public/attachments/attachment_uploader_impl.h

@@ -38,7 +38,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
           url_request_context_getter,
       const std::string& account_id,
       const OAuth2TokenService::ScopeSet& scopes,
-      scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+      const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
           token_service_provider);
   virtual ~AttachmentUploaderImpl();
 
@@ -61,7 +61,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   std::string account_id_;
   OAuth2TokenService::ScopeSet scopes_;
-  scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+  scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>
       token_service_provider_;
   StateMap state_map_;
   DISALLOW_COPY_AND_ASSIGN(AttachmentUploaderImpl);