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

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289222

[maniscalco, 2014-08-11 17:51:04]

Roger, Mihai, would you two please review this change?

[rohitrao (ping after 24h), 2014-08-11 18:07:17]

Is it possible for token_service_ to be destroyed before the
OAuth2TokenServiceRequest, or is that guaranteed to be impossible?

[maniscalco, 2014-08-11 18:15:21]

On 2014/08/11 18:07:17, rohitrao wrote:
> Is it possible for token_service_ to be destroyed before the
> OAuth2TokenServiceRequest, or is that guaranteed to be impossible?

Guaranteed as a long as the user of OAuth2TokenServiceRequest is following the
rules.  The OAuth2TokenService returned by GetTokenService (pointed to by
token_service_) must outlive OAuth2TokenServiceRequest (as mentioned in the
method comment for GetTokenService).  In other words, users of
OAuth2TokenServiceRequest must guarantee that the OAuth2TokenService outlives
the OAuth2TokenServiceRequest.  I believe all user of OAuth2TokenServiceRequest
correctly guarantee it.

[Roger Tawa OOO till Jul 10th, 2014-08-11 18:37:38]

Is there still a good reason to keep the TokenServiceProvider interface, or
would it be better to just pass the service and task runner directly to the
ctor?

[maniscalco, 2014-08-11 19:30:26]

On 2014/08/11 18:37:38, Roger Tawa wrote:
> Is there still a good reason to keep the TokenServiceProvider interface, or
> would it be better to just pass the service and task runner directly to the
> ctor?

Good idea.  TokenServiceProvider isn't adding much value.  Removed (see patch
set 3).  WDYT?

[msarda, 2014-08-12 07:55:52]

On 2014/08/11 19:30:26, maniscalco wrote:
> On 2014/08/11 18:37:38, Roger Tawa wrote:
> > Is there still a good reason to keep the TokenServiceProvider interface, or
> > would it be better to just pass the service and task runner directly to the
> > ctor?
> 
> Good idea.  TokenServiceProvider isn't adding much value.  Removed (see patch
> set 3).  WDYT?

I explained this in the bug. IMHO, the whole idea of the
OAuth2TokenServiceRequest in to allow a client to make an OAuth2 authorized call
from any thread. OAuth2TokenService and the correspoinding factory
ProfileOAuth2TokenServiceFactory are single-threaded and must only be called
from the main thread (UI). So I think that we need a provider that can only be
called from the main thread to provide the core with the token service. Ideally
the Core should own this provider or the provider needs to be ref-counted.

[msarda, 2014-08-12 07:56:52]

+Colin who actually wrote the ProfileTokenServiceProvider for CloundPrint.

[blundell, 2014-08-12 13:33:05]

On 2014/08/12 07:56:52, msarda wrote:
> +Colin who actually wrote the ProfileTokenServiceProvider for CloundPrint.

I agree with Mihai that the point of the provider is to enable the client to
call the TokenService from a thread where they cannot obtain an instance of the
TokenService themselves, so eliminating taking in the provider in favor of
taking in the TokenService directly doesn't work.

I think that by far the simplest solution is to make TokenServiceProvider be
RefCountedThreadSafe and have the Core hold it in a scoped_refptr, like it does
with the task runner.

[maniscalco, 2014-08-12 16:40:29]

On 2014/08/12 13:33:05, blundell wrote:
> On 2014/08/12 07:56:52, msarda wrote:
> > +Colin who actually wrote the ProfileTokenServiceProvider for CloundPrint.
> 
> I agree with Mihai that the point of the provider is to enable the client to
> call the TokenService from a thread where they cannot obtain an instance of
the
> TokenService themselves, so eliminating taking in the provider in favor of
> taking in the TokenService directly doesn't work.
> 
> I think that by far the simplest solution is to make TokenServiceProvider be
> RefCountedThreadSafe and have the Core hold it in a scoped_refptr, like it
does
> with the task runner.

OK, for CloudPrint it's difficult to get access to an OAuth2TokenService*
because CP is not running on this UI thread so we really do want TSP to provide
the from-any-thread functionality.  Got it.

I'll update this CL with a patch that makes TSP RefCountedThreadSafe.

[msarda, 2014-08-12 16:56:51]

On 2014/08/12 16:40:29, maniscalco wrote:
> On 2014/08/12 13:33:05, blundell wrote:
> > On 2014/08/12 07:56:52, msarda wrote:
> > > +Colin who actually wrote the ProfileTokenServiceProvider for CloundPrint.
> > 
> > I agree with Mihai that the point of the provider is to enable the client to
> > call the TokenService from a thread where they cannot obtain an instance of
> the
> > TokenService themselves, so eliminating taking in the provider in favor of
> > taking in the TokenService directly doesn't work.
> > 
> > I think that by far the simplest solution is to make TokenServiceProvider be
> > RefCountedThreadSafe and have the Core hold it in a scoped_refptr, like it
> does
> > with the task runner.
> 
> OK, for CloudPrint it's difficult to get access to an OAuth2TokenService*
> because CP is not running on this UI thread so we really do want TSP to
provide
> the from-any-thread functionality.  Got it.
> 
> I'll update this CL with a patch that makes TSP RefCountedThreadSafe.

I've looked again at the code and updated the bug. I think we can get by without
TSP RefCountedThreadSafe.

[maniscalco, 2014-08-12 17:35:14]

Mihai, understood, thanks.  Sounds like patch set 3 is still a candidate.

I've now added patch set 5, which makes TSP RefCountedThreadSafe.  At this point
both 3 and 5 are candidates.  This bug is causing crashes in M37 so it's
important to land a fix today.

Roger, PTAL.  LMK if you have a strong preference for one or the other.  My
preference is for patch set 5 because it makes life easier for users of OA2TSR.


On 2014/08/12 16:56:51, msarda wrote:
> On 2014/08/12 16:40:29, maniscalco wrote:
> > On 2014/08/12 13:33:05, blundell wrote:
> > > On 2014/08/12 07:56:52, msarda wrote:
> > > > +Colin who actually wrote the ProfileTokenServiceProvider for
CloundPrint.
> > > 
> > > I agree with Mihai that the point of the provider is to enable the client
to
> > > call the TokenService from a thread where they cannot obtain an instance
of
> > the
> > > TokenService themselves, so eliminating taking in the provider in favor of
> > > taking in the TokenService directly doesn't work.
> > > 
> > > I think that by far the simplest solution is to make TokenServiceProvider
be
> > > RefCountedThreadSafe and have the Core hold it in a scoped_refptr, like it
> > does
> > > with the task runner.
> > 
> > OK, for CloudPrint it's difficult to get access to an OAuth2TokenService*
> > because CP is not running on this UI thread so we really do want TSP to
> provide
> > the from-any-thread functionality.  Got it.
> > 
> > I'll update this CL with a patch that makes TSP RefCountedThreadSafe.
> 
> I've looked again at the code and updated the bug. I think we can get by
without
> TSP RefCountedThreadSafe.

[Roger Tawa OOO till Jul 10th, 2014-08-12 18:09:56]

lgtm for patchset 5.

https://codereview.chromium.org/458753006/diff/80001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service_request.h (right):

https://codereview.chromium.org/458753006/diff/80001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service_request.h:32: // OAuth2TokenServiceRequest
thread.
How is this guaranteed?  By having the last reference be held by the Core class,
and having this reference reset in the Core dtor?

This might not happen if the caller of CreateAndStart() or InvalidateToken()
holds on to the scoped_refptr too.  Maybe put a DCHECK in the Core dtor that
this is indeed the last reference?

[maniscalco, 2014-08-12 18:27:03]

Thanks for the feedback.  I've uploaded patch set 6 which clarifies a comment
about who may call TSP's dtor.

https://codereview.chromium.org/458753006/diff/80001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service_request.h (right):

https://codereview.chromium.org/458753006/diff/80001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service_request.h:32: // OAuth2TokenServiceRequest
thread.
On 2014/08/12 18:09:56, Roger Tawa wrote:
> How is this guaranteed?  By having the last reference be held by the Core
class,
> and having this reference reset in the Core dtor?
> 
> This might not happen if the caller of CreateAndStart() or InvalidateToken()
> holds on to the scoped_refptr too.  Maybe put a DCHECK in the Core dtor that
> this is indeed the last reference?

You're right, the user of OA2TSR could keep a reference to the TSP and release
it on any thread they choose.  I'll update the comment to indicate that TSP must
be capable of being destroyed on the owner thread.  That way, CloudPrint's use
of OA2TSR still has the option of creating/destroying the TSR on some thread
other than the owner thread (which it may do actually).

[maniscalco, 2014-08-12 20:38:08]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-08-12 20:42:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/458753006/100001

[commit-bot: I haz the power, 2014-08-12 22:48:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 00:42:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 00:42:30]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[maniscalco, 2014-08-13 02:35:37]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-08-13 02:37:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/458753006/100001

[commit-bot: I haz the power, 2014-08-13 05:50:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

lgtm with a question about a comment

This code is like an onion...

Now that I look at it more deeply, it seems inherently wrong to me that calling
core_->Stop()  doesn't cancel outstanding tasks that core_ has posted. That's
really the root of this problem IMO. I wonder if core_ should be using a
base::CancelableTaskTracker to cancel all its outstanding tasks when Stop() is
called...I haven't thought it through in detail though.

https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request.cc (right):

https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
provider_ is destroyed on the owner thread, not the
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_|.

[blundell, 2014-08-13 07:55:52]

On 2014/08/13 06:58:05, blundell wrote:
> lgtm with a question about a comment
> 
> This code is like an onion...
> 
> Now that I look at it more deeply, it seems inherently wrong to me that
calling
> core_->Stop()  doesn't cancel outstanding tasks that core_ has posted. That's
> really the root of this problem IMO. I wonder if core_ should be using a
> base::CancelableTaskTracker to cancel all its outstanding tasks when Stop() is
> called...I haven't thought it through in detail though.

Mihai pointed out that since CancelableTaskTracker can only do best-effort
cancelling in the cross-thread case, we'd still have to deal with this problem
even if we did that.

> 
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> File google_apis/gaia/oauth2_token_service_request.cc (right):
> 
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
> provider_ is destroyed on the owner thread, not the
> 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_|.

[commit-bot: I haz the power, 2014-08-13 09:06:39]

Change committed as 289222

[maniscalco, 2014-08-13 16:10:30]

I want to make sure I understand.  Do you see a bug or are you commenting on how
this class could be simplified to make it easier to understand and maintain?

Guaranteed cancelling of a task executing in another thread seems "a hard
problem".  We could do it if we were willing to block the cancelling thread
until the task reaches a stopping point, but we don't want to do that.

On 2014/08/13 07:55:52, blundell wrote:
> On 2014/08/13 06:58:05, blundell wrote:
> > lgtm with a question about a comment
> > 
> > This code is like an onion...
> > 
> > Now that I look at it more deeply, it seems inherently wrong to me that
> calling
> > core_->Stop()  doesn't cancel outstanding tasks that core_ has posted.
That's
> > really the root of this problem IMO. I wonder if core_ should be using a
> > base::CancelableTaskTracker to cancel all its outstanding tasks when Stop()
is
> > called...I haven't thought it through in detail though.
> 
> Mihai pointed out that since CancelableTaskTracker can only do best-effort
> cancelling in the cross-thread case, we'd still have to deal with this problem
> even if we did that.
> 
> > 
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > File google_apis/gaia/oauth2_token_service_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
> > provider_ is destroyed on the owner thread, not the
> > 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:04]

https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request.cc (right):

https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
provider_ is destroyed on the owner thread, not the
On 2014/08/13 06:58:05, blundell wrote:
> 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_|.

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.

[blundell, 2014-08-14 07:14:34]

On 2014/08/13 16:10:30, maniscalco wrote:
> I want to make sure I understand.  Do you see a bug or are you commenting on
how
> this class could be simplified to make it easier to understand and maintain?
> 
> Guaranteed cancelling of a task executing in another thread seems "a hard
> problem".  We could do it if we were willing to block the cancelling thread
> until the task reaches a stopping point, but we don't want to do that.
> 

No bug. It was a suggestion for simplifying this class, but the suggestion
doesn't work precisely because base::CancelableTaskTracker provides only
best-effort cancelling of tasks posted to a different thread.

> On 2014/08/13 07:55:52, blundell wrote:
> > On 2014/08/13 06:58:05, blundell wrote:
> > > lgtm with a question about a comment
> > > 
> > > This code is like an onion...
> > > 
> > > Now that I look at it more deeply, it seems inherently wrong to me that
> > calling
> > > core_->Stop()  doesn't cancel outstanding tasks that core_ has posted.
> That's
> > > really the root of this problem IMO. I wonder if core_ should be using a
> > > base::CancelableTaskTracker to cancel all its outstanding tasks when
Stop()
> is
> > > called...I haven't thought it through in detail though.
> > 
> > Mihai pointed out that since CancelableTaskTracker can only do best-effort
> > cancelling in the cross-thread case, we'd still have to deal with this
problem
> > even if we did that.
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > > File google_apis/gaia/oauth2_token_service_request.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > > google_apis/gaia/oauth2_token_service_request.cc:80: // It is important
that
> > > provider_ is destroyed on the owner thread, not the
> > > 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_|.

[blundell, 2014-08-14 07:16:54]

On 2014/08/13 16:13:04, maniscalco wrote:
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> File google_apis/gaia/oauth2_token_service_request.cc (right):
> 
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
> provider_ is destroyed on the owner thread, not the
> On 2014/08/13 06:58:05, blundell wrote:
> > 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_|.
> 
> 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.

Maybe something like "OA2TokenServiceRequest guarantees to clear its last
reference to the passed-in TSP on the owner thread, so that the client can
ensure that the TSP instance is destroyed on the owner thread if desired." ?

[maniscalco, 2014-08-14 16:12:20]

On 2014/08/14 07:16:54, blundell (OOO until August 25) wrote:
> On 2014/08/13 16:13:04, maniscalco wrote:
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > File google_apis/gaia/oauth2_token_service_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/458753006/diff/100001/google_apis/gaia/oauth2...
> > google_apis/gaia/oauth2_token_service_request.cc:80: // It is important that
> > provider_ is destroyed on the owner thread, not the
> > On 2014/08/13 06:58:05, blundell wrote:
> > > 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_|.
> > 
> > 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.
> 
> Maybe something like "OA2TokenServiceRequest guarantees to clear its last
> reference to the passed-in TSP on the owner thread, so that the client can
> ensure that the TSP instance is destroyed on the owner thread if desired." ?

I like that.  I'll send a CL.