Refactor ProfileOAuth2TokenServiceRequest into OAuth2TokenServiceRequest

Componentize and rename ProfileOAuth2TokenServiceRequest.

The purpose of this change is to move ProfileOAuth2TokenServiceRequest
into google_apis/gaia and drop any browser dependencies.

ProfileOAuth2TokenServiceRequest becomes OAuth2TokenServiceRequest.

Add support for invalidating tokens.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274255

[maniscalco, 2014-05-27 17:35:28]

Hi Roger, would you please review this change?

Here's the background.  I needed a way to access an OAuth2TokenService from the
IO thread.  ProfileOAuth2TokenServiceRequest looked like what I wanted, but it
has a dependency on src/chrome/browser and I need something I can access from
src/sync/internal_api (no browser dep allowed).

LMK if you feel this new class should live in sync land instead of google_apis.

Pavel, I've CC'd you on this since you've done similar stuff with OA2TS in the
past.

Thanks for looking!

[maniscalco, 2014-05-27 22:41:25]

https://codereview.chromium.org/299943003/diff/180001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_proxy.h (right):

https://codereview.chromium.org/299943003/diff/180001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_proxy.h:24: const base::Time& /*
expiration_time */)>
After talking with pavely@, I'm inclined to remove expiration time from the
callback because users of this API shouldn't worry about caching the resulting
token.  However, ProfileOAuth2TokenServiceRequest uses this callback and passes
expiration time on to its consumer.  If we remove it here, POA2TSR will need to
provide a fake expiration time.  Do you think it's worth removing and providing
a fake expiration time or leave as is?

[maniscalco, 2014-05-29 23:13:50]

Thanks for the detailed OOB feedback.  Based on your suggestions, I've written
this patch.  RFAL!

[msarda, 2014-05-30 14:37:31]

Kudos for the architecture of Core / RequestCore and InvalidateCore.

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

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:31: // 3. ...
Maybe:
s/.../Request is executed.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:64:
base::SingleThreadTaskRunner* owner_task_runner();
It seems that owner_task_runner() is just used to check the current thread.
Could you just use  CalledOnValidThread() for that?

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:133: }
I would add a method IsStopped() here and use it below instead of checking
owner() != NULL.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:170:
OAuth2TokenService::Consumer* const consumer_;
I think we prefer: const OAuth2TokenService::Consumer*

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:192: scopes_(scopes) {
Roger: I remember there were some problems on Android when using empty scopes.
Maybe we should DCHECK here that scopes cannot be empty.

Same for |account_id|.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:215:
owner_task_runner()->PostTask(
There is slight difference with the previous implementation: if
|owner_task_runner| runs on the current thread, you're actually posting the task
later instead of calling |RequestCore::InformOwnerOnGetTokenSuccess|
synchronously.

I do not think that's an issue, I just want to point out the difference in
behavior.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:237: if (owner()) {
If you add IsStopped() to Core, then you could use here if (!IsStopped())

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:324:
OAuth2TokenServiceRequest::OAuth2TokenServiceRequest(
Constructors / destructor are at the top of the class.
Static methods come after the constructors.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:331: core_(new
RequestCore(owner_task_runner,
The two constructors take very similar arguments, but the core they build is
very different.

I vote for having a single constructor that takes as argument |core| and just
create and pass the |core| from the static methods. You may need to have a
SetCore method as |Core|'s constructor takes "this" as argument.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request.h (right):

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.h:50: //
|account_id|| must not be empty?

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request_unittest.cc (right):

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request_unittest.cc:74: int
num_invalidate_token() const;
Simple accessor methods should be inlined.

[Roger Tawa OOO till Jul 10th, 2014-05-30 15:39:00]

lgtm

Thanks for the awesome refactor Nick!

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

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:80: const
scoped_refptr<base::SingleThreadTaskRunner>& owner_task_runner,
|owner_task_runner| does not seem to be used.  I think you might have wanted to
use it to initialize |owner_task_runner_|, but probably better to remove the arg
and initialize |owner_task_runner_| the way you do it at line 83 below.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:192: scopes_(scopes) {
On 2014/05/30 14:37:31, msarda wrote:
> Roger: I remember there were some problems on Android when using empty scopes.
> Maybe we should DCHECK here that scopes cannot be empty.
> 
> Same for |account_id|.

Adding a DCHECK for empty scopes is a good idea.  Also in InvalidateCore.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:197:
DCHECK(owner_task_runner()->BelongsToCurrentThread());
Already done in base class dtor.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:293:
DCHECK(owner_task_runner()->BelongsToCurrentThread());
Already done in base class dtor.

[maniscalco, 2014-05-30 17:48:07]

Thanks for the great feedback and speedy review!  All addressed.  RFAL.

Note: I changed the public static methods to return scoped_ptrs instead of *'s. 
That way, it's less likely a user of this class will inadvertently leak OA2TSRs.

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

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:31: // 3. ...
On 2014/05/30 14:37:31, msarda wrote:
> Maybe:
> s/.../Request is executed.

Done.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:64:
base::SingleThreadTaskRunner* owner_task_runner();
On 2014/05/30 14:37:31, msarda wrote:
> It seems that owner_task_runner() is just used to check the current thread.
> Could you just use  CalledOnValidThread() for that?

Good idea.  Done.  Core now inherits from NonThreadSafe and I've removed
owner_task_runner from all but RequestCore.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:80: const
scoped_refptr<base::SingleThreadTaskRunner>& owner_task_runner,
On 2014/05/30 15:39:00, Roger Tawa wrote:
> |owner_task_runner| does not seem to be used.  I think you might have wanted
to
> use it to initialize |owner_task_runner_|, but probably better to remove the
arg
> and initialize |owner_task_runner_| the way you do it at line 83 below.

Good catch.  Parameter removed.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:133: }
On 2014/05/30 14:37:31, msarda wrote:
> I would add a method IsStopped() here and use it below instead of checking
> owner() != NULL.

Done.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:170:
OAuth2TokenService::Consumer* const consumer_;
On 2014/05/30 14:37:31, msarda wrote:
> I think we prefer: const OAuth2TokenService::Consumer*

In this case, the consumer object itself must be non-const.  The const is there
just to make sure the pointer itself doesn't change.  LMK if I should drop the
const entirely.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:192: scopes_(scopes) {
On 2014/05/30 15:39:00, Roger Tawa wrote:
> On 2014/05/30 14:37:31, msarda wrote:
> > Roger: I remember there were some problems on Android when using empty
scopes.
> > Maybe we should DCHECK here that scopes cannot be empty.
> > 
> > Same for |account_id|.
> 
> Adding a DCHECK for empty scopes is a good idea.  Also in InvalidateCore.

Done.  I've added DCHECKs for empty scopes and made the DCHECKing more
consistent.  We now DCHECK in all the ctors when we copy/assign parameters into
member variables.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:192: scopes_(scopes) {
On 2014/05/30 14:37:31, msarda wrote:
> Roger: I remember there were some problems on Android when using empty scopes.
> Maybe we should DCHECK here that scopes cannot be empty.
> 
> Same for |account_id|.

Done.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:197:
DCHECK(owner_task_runner()->BelongsToCurrentThread());
On 2014/05/30 15:39:00, Roger Tawa wrote:
> Already done in base class dtor.

Removed here and elsewhere.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:215:
owner_task_runner()->PostTask(
On 2014/05/30 14:37:31, msarda wrote:
> There is slight difference with the previous implementation: if
> |owner_task_runner| runs on the current thread, you're actually posting the
task
> later instead of calling |RequestCore::InformOwnerOnGetTokenSuccess|
> synchronously.
> 
> I do not think that's an issue, I just want to point out the difference in
> behavior.

Yes, I should have pointed that out.  I removed the synchronous call branches
because I figure having one path (always post) is simpler and will help expose
any hidden bugs.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:237: if (owner()) {
On 2014/05/30 14:37:31, msarda wrote:
> If you add IsStopped() to Core, then you could use here if (!IsStopped())

Done.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:293:
DCHECK(owner_task_runner()->BelongsToCurrentThread());
On 2014/05/30 15:39:00, Roger Tawa wrote:
> Already done in base class dtor.

Removed.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:324:
OAuth2TokenServiceRequest::OAuth2TokenServiceRequest(
On 2014/05/30 14:37:31, msarda wrote:
> Constructors / destructor are at the top of the class.
> Static methods come after the constructors.

Done.  I've reordered the method definitions to match their declaration order.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:331: core_(new
RequestCore(owner_task_runner,
Done.  OA2TSR now has just one ctor that takes a Core.  I added private
StartWithCore method to kick things off.

On 2014/05/30 14:37:31, msarda wrote:
> The two constructors take very similar arguments, but the core they build is
> very different.
> 
> I vote for having a single constructor that takes as argument |core| and just
> create and pass the |core| from the static methods. You may need to have a
> SetCore method as |Core|'s constructor takes "this" as argument.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request.h (right):

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.h:50: //
On 2014/05/30 14:37:31, msarda wrote:
> |account_id|| must not be empty?

Done.

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_token_service_request_unittest.cc (right):

https://codereview.chromium.org/299943003/diff/240001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request_unittest.cc:74: int
num_invalidate_token() const;
On 2014/05/30 14:37:31, msarda wrote:
> Simple accessor methods should be inlined.

Done.

[msarda, 2014-06-01 22:19:35]

lgtm

[maniscalco, 2014-06-02 03:22:52]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-06-02 03:23:18]

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

[commit-bot: I haz the power, 2014-06-02 03:39:49]

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

[commit-bot: I haz the power, 2014-06-02 03:40:22]

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

[commit-bot: I haz the power, 2014-06-02 03:40:22]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[maniscalco, 2014-06-02 16:07:12]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-06-02 16:07:50]

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

[commit-bot: I haz the power, 2014-06-02 16:09:07]

Change committed as 274255

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

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

https://codereview.chromium.org/299943003/diff/280001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:107:
token_service_task_runner_->PostTask(
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]

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

https://codereview.chromium.org/299943003/diff/280001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:107:
token_service_task_runner_->PostTask(
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:
> 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 17:29:41]

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

https://codereview.chromium.org/299943003/diff/280001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_token_service_request.cc:107:
token_service_task_runner_->PostTask(
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:
> 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:
> > 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 18:31:40]

Ah, I think I see what you meant about PostTaskAndReply.

Please see https://codereview.chromium.org/317773003/ for a patch.