Fix use after free bug by calling GetTokenService in Core's ctor.

google_apis/gaia/oauth2_token_service_request.cc

Index: google_apis/gaia/oauth2_token_service_request.cc
diff --git a/google_apis/gaia/oauth2_token_service_request.cc b/google_apis/gaia/oauth2_token_service_request.cc
index 672015241779a45d4e8f1dea882947d0844c3011..b652c2b26c1e5f75798a96ed9e6481852d80e4ec 100644
--- a/google_apis/gaia/oauth2_token_service_request.cc
+++ b/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

[blundell, 2014/08/13 06:58:05]

This comment is puzzling. Since |provider_| is a ref-counted object that is
passed to this object from the outside, we actually don't have control over the
destruction of |provider_|.

[maniscalco, 2014/08/13 16:13:03]

You're right, this comment is incorrect.  The class comment for TSP is related
and was also incorrect until Roger pointed it out and I fixed it.  Looks like I
missed this one.

The idea is that we don't want a future maintainer to cause provider_ to be
destroyed on any thread other than the owner thread.  OAuth2TokenServiceRequest
is promising to clear references to TSP only on the owner thread.

Why does OA2TSP care about not causing TSP to be destroyed on a thread other
than the owner thread?  Only because it promised that in the TSP class comment.

Happy to fix this comment in a follow up CL.  Open to suggestions on how to word
it clearly.

+  // 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) {