Use OAuth2 token for sync

Use OAuth2 token for sync

ProfileSyncService requests access token from OAuth2TokenService and passes it 
to ServerConnectionManager through UpdateCredentials. When server returns
AUTH_ERROR it gets propagated to ProfileSyncService through OnGetStatusChange 
call. At this point ProfileSyncService needs to invalidate old token with 
OAuth2TokenService and request a new one.
Access token is requested in PSS::StartUp since this is the place where all
preconditions are verified. There is a call to pre-request access token in
Initialize and Observe after Login token is loaded.

There are still two tests disabled, I'll fix them and update this CR.

BUG=226464
TBR=jhawkins@chromium.org

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

[tim (not reviewing), 2013-05-23 19:03:41]

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:443: if (token_iterator !=
token_cache_.end() &&
I guess this wasn't being used very commonly?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:109: static const char*
kOAuth2Scopes[] = {
These belong in google_apis / GaiaConstants.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:151: access_token_(),
Remove these two initializers as they are not necessary and convention in
Chromium is only to have an initializer if necessary.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:184: bool
ProfileSyncService::IsSyncTokenAvailable() {
This function should either be removed or renamed to be
IsOAuthRefreshTokenAvailable, depending on what the TokenService function is
called (see comment below).

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:188: return
token_service->HasOAuthLoginToken();
Is there a better name than just "OAuthLoginToken" for this function? 
"OAuth[Uber, Refresh]Token"?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:418: if (!access_token_.empty()) {
Can you explain the semantics here in a comment if access_token_ is empty?  What
happens to credentials.sync_token?  Is this not equivalent to assigning even in
that case?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:501: if (access_token_.empty()) {
The relationship between TryStart and StartUp is that *all* preconditions for
starting are true by the time StartUp is called.  Everything sync needs should
be there and ready, but all logic of figuring out when to start should be
handled by TryStart.  It seems like this should be made a
DCHECK(!access_token_.empty()).

In TryStart, we shouldn't call StartUp if access_token_ is empty. But we also
shouldn't have to call RequestAccessToken, right?  It would have been called by
Initialize.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:618: access_token_request_.reset();
DCHECK(access_token_request_.get()) in both these functions would be nice.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1055: if (status ==
syncer::CONNECTION_AUTH_ERROR) {
Comment this - is this known to be a transient access_token expiration?  Or do
we optimistically try that first regardless?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1810: {
nit: { on previous line.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1812: if (access_token_request_ !=
NULL)
Think this should be a DCHECK?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1823: access_token_ = std::string();
use access_token_.clear()

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1976: access_token_ = std::string();
use .clear()

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.h:737: bool invalidate_previous_token,
style-nit:  Functions with bool parameters (especially multiple bool parameters)
whose purpose isn't immediately evident from the function name (i.e
SetEnabledState(bool enabled)) should prefer enums to bools.  

e.g, so we'd write RequestAccessToken(INVALIDATE_PREVIOUS_TOKEN,
DONT_INVOKE_CALLBACK);

Also, what is the callback here?  And can you explain 'downscoped' in your
comment?  Should that be part of the function name?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:231: // 
EXPECT_CALL(*data_type_manager, Configure(_, _)).Times(0);
Tidy this up.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:253: //
EXPECT_CALL(*sync_->components_factory_mock(),
Tidy up these commented out expectations.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:442: printf("{PAV}
1\n");
Remove printfs.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:495: //
TEST_F(ProfileSyncServiceStartupTest, StartDownloadFailed) {
Fix this.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:190: //   
HandleInitializationCompletedOnFrontendLoop(false);
Tidy up.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:247: message_loop_runner_() {
Remove unnecessary initializer.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:276: invoke_callback);
nit - indent is off (4 spaces).

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:303:
message_loop_runner_.Quit();
:/  This seems to make it worse than before, it's even more subtle.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:352: base::Time::Max());
indent is off.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
File chrome/browser/sync/test_profile_sync_service.h (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.h:36: // MessageLoopRunner::Run()
may be called multiple times recursively. Only first
Hmm.. this kind of thing (nested Run() calls -- see comment in message_loop.h @
Run() declaration) is strongly discouraged.  As is writing your own ML wrapper
vs using a RunLoop or existing tool.

I'm not sure yet why this is necessary, but it seems to take us farther away
from where we'd like to be with these tests, using a more standard approach.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:342:
error_(GoogleServiceAuthError::NONE),
indent is off (4 spaces)

https://codereview.chromium.org/15421011/diff/1/jingle/notifier/base/notifier...
File jingle/notifier/base/notifier_options_util.cc (right):

https://codereview.chromium.org/15421011/diff/1/jingle/notifier/base/notifier...
jingle/notifier/base/notifier_options_util.cc:30: if
(notifier_options.auth_mechanism == buzz::AUTH_MECHANISM_OAUTH2)
We should use a constant (this one or otherwise) for defining
AUTH_MECHANISM_OAUTH2 as well (vs using X-OAUTH2 literally in the code).

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
sync/engine/net/server_connection_manager.h:72: // connection after AUTH_ERROR
when new access token was issued but not yet
Issued by who?  Not sure I know what this is about?

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
sync/engine/net/server_connection_manager.h:264: public:
Move the call up to the public section instead.

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
sync/engine/net/server_connection_manager.h:268: protected:
indent is off.

[Andrew T Wilson (Slow), 2013-05-24 14:10:15]

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:443: if (token_iterator !=
token_cache_.end() &&
On 2013/05/23 19:03:41, timsteele wrote:
> I guess this wasn't being used very commonly?

I guess not. It's mostly an optimization anyway (if someone gets an error using
a token they can invalidate it so we try to re-fetch in the future - the fact
that this didn't work probably was masked by the fact that sync already
caught/reported auth errors).

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:36: #include
"chrome/browser/signin/token_service_factory.h"
Can we remove our direct dependency on TokenService? Even if we still need to
listen for token_available notifications, we shouldn't depend on the class any
more.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:188: return
token_service->HasOAuthLoginToken();
I wonder if it's better to call OAuth2TokenService::RefreshTokenIsAvailable()
here instead of talking to TokenService directly. Ideally we would remove our
direct dependency on TokenService.

And if we can remove this function entirely, that'd be even niftier, in favor of
callers just calling RefreshTokenIsAvailable().

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:418: if (!access_token_.empty()) {
On 2013/05/23 19:03:41, timsteele wrote:
> Can you explain the semantics here in a comment if access_token_ is empty? 
What
> happens to credentials.sync_token?  Is this not equivalent to assigning even
in
> that case?

Note the varying UMAs being logged. I do wonder if this is useful at all any
more - this code was trying to trigger an auth error (instead of some kind of
unrecoverable error in sync) in the case that for some reason the TokenDB did
not have a chromiumsync token.

What are the situations where access_token_ can be empty? I can imagine a bunch
of cases:

* Temporary network error
* Persistent auth error (OAuthTokenService can't mint a token for you due to
INVALID_GAIA_CREDENTIALS)
* Local tokendb corruption (user is signed in, but has no oauth login token -
ideally we'd catch this elswhere and report as an auth error)
* "chromiumsync" service is disabled on the server via the dasher admin panel

I think we only need to report an auth error in case #2 - actually doing so in
any other case is bad.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:618: access_token_request_.reset();
On 2013/05/23 19:03:42, timsteele wrote:
> DCHECK(access_token_request_.get()) in both these functions would be nice.

Actually, I think you should do DCHECK_EQ(access_token_request, request).

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1812: if (access_token_request_ !=
NULL)
On 2013/05/23 19:03:42, timsteele wrote:
> Think this should be a DCHECK?

I think this can happen in practice - there's nothing in the contract that says
you'll get exactly one OnConnectionStatusChange call, for example.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1828: access_token_request_.reset();
I am confused by this - on startup, won't this cause two access_token_requests
to be initiated, since we aren't tracking our first request? Why do we ever want
to pass invoke_callback == true - why not just let the fetch complete and invoke
OnGetTokenSuccess()?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1939:
GaiaConstants::kGaiaOAuth2LoginRefreshToken &&
I'd really like to see us get out of the "listen for TOKEN_REQUEST_FAILED"
business. Can you put a TODO(atwilson) in the code here? There's no reason for
sync to be reporting auth errors on behalf of TokenService - this is at best a
leftover from our legacy signin architecture, and at worst obsolete and
superfluous.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1957:
GaiaConstants::kGaiaOAuth2LoginRefreshToken)
We should really move NOTIFICATION_TOKEN_AVAILABLE to OAuth2TokenService. I've
filed a bug for this - can you add a TODO(atwilson): Listen for notifications on
OAuth2TokenService (crbug.com/243737) here?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:275: //
TEST_F(ProfileSyncServiceStartupTest, StartInvalidCredentials) {
Should this be commented out - still seems like it's useful to test this case.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:424: // MessageLoop
instance is required to work with OneShotTimer.
Is this comment now obsolete?

https://codereview.chromium.org/15421011/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/15421011/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:605: 
Hmm. I'm not sure why it's safe to move
"observing_network_connectivity_changes_=true" down here after the early exit
above.

I'm also not sure why we need this TOKEN_REFRESHED state - from the standpoint
of the SyncManager why didn't the old implementation work?

[pavely, 2013-05-30 07:42:12]

Andrew, please take a look.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:443: if (token_iterator !=
token_cache_.end() &&
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> On 2013/05/23 19:03:41, timsteele wrote:
> > I guess this wasn't being used very commonly?
> 
> I guess not. It's mostly an optimization anyway (if someone gets an error
using
> a token they can invalidate it so we try to re-fetch in the future - the fact
> that this didn't work probably was masked by the fact that sync already
> caught/reported auth errors).
If server returns AUTH_ERROR before token expires then sync should invalidate
access token with OAuth2TokenService, otherwise it is guaranteed to get the same
access token on consecutive token requests.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:36: #include
"chrome/browser/signin/token_service_factory.h"
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> Can we remove our direct dependency on TokenService? Even if we still need to
> listen for token_available notifications, we shouldn't depend on the class any
> more.

There are two places where TokenService is still used:
- To register for notifications
- To check if tokens are loaded from DB in TryStart.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:109: static const char*
kOAuth2Scopes[] = {
On 2013/05/23 19:03:41, timsteele wrote:
> These belong in google_apis / GaiaConstants.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:151: access_token_(),
On 2013/05/23 19:03:41, timsteele wrote:
> Remove these two initializers as they are not necessary and convention in
> Chromium is only to have an initializer if necessary.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:184: bool
ProfileSyncService::IsSyncTokenAvailable() {
On 2013/05/23 19:03:41, timsteele wrote:
> This function should either be removed or renamed to be
> IsOAuthRefreshTokenAvailable, depending on what the TokenService function is
> called (see comment below).

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:188: return
token_service->HasOAuthLoginToken();
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> I wonder if it's better to call OAuth2TokenService::RefreshTokenIsAvailable()
> here instead of talking to TokenService directly. Ideally we would remove our
> direct dependency on TokenService.
> 
> And if we can remove this function entirely, that'd be even niftier, in favor
of
> callers just calling RefreshTokenIsAvailable().

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:418: if (!access_token_.empty()) {
On 2013/05/23 19:03:41, timsteele wrote:
> Can you explain the semantics here in a comment if access_token_ is empty? 
What
> happens to credentials.sync_token?  Is this not equivalent to assigning even
in
> that case?

Like it was before if access token is empty then sync sends some known bad value
to server ("credentials_lost"). It is equivalent to assigning empty
access_token_, only difference is value that will be seen on the server. I just
wasn't sure if changing this value matters.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:418: if (!access_token_.empty()) {
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> On 2013/05/23 19:03:41, timsteele wrote:
> > Can you explain the semantics here in a comment if access_token_ is empty? 
> What
> > happens to credentials.sync_token?  Is this not equivalent to assigning even
> in
> > that case?
> 
> Note the varying UMAs being logged. I do wonder if this is useful at all any
> more - this code was trying to trigger an auth error (instead of some kind of
> unrecoverable error in sync) in the case that for some reason the TokenDB did
> not have a chromiumsync token.
> 
> What are the situations where access_token_ can be empty? I can imagine a
bunch
> of cases:
> 
> * Temporary network error
> * Persistent auth error (OAuthTokenService can't mint a token for you due to
> INVALID_GAIA_CREDENTIALS)
> * Local tokendb corruption (user is signed in, but has no oauth login token -
> ideally we'd catch this elswhere and report as an auth error)
> * "chromiumsync" service is disabled on the server via the dasher admin panel
> 
> I think we only need to report an auth error in case #2 - actually doing so in
> any other case is bad.

Let me explain cases that you listed:
- In temporary network error either PSS wouldn't start backend or token wouldn't
be cleared. 
- In case of auth error refreshing token will cause OnGetTokenFailure which will
display error.
- With local storage corruption PSS won't be able to request access token and
will display error.
- disabled service will be handled differently. I haven't found a way to find
out from client that account is disabled without actually sending RPC to sync
server.

UMA histograms in failure case would be "increment if user is signed in but
login token is not present.
Do you mind if I remove these two histogram statements?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:501: if (access_token_.empty()) {
On 2013/05/23 19:03:41, timsteele wrote:
> The relationship between TryStart and StartUp is that *all* preconditions for
> starting are true by the time StartUp is called.  Everything sync needs should
> be there and ready, but all logic of figuring out when to start should be
> handled by TryStart.  It seems like this should be made a
> DCHECK(!access_token_.empty()).
> 
> In TryStart, we shouldn't call StartUp if access_token_ is empty. But we also
> shouldn't have to call RequestAccessToken, right?  It would have been called
by
> Initialize.

TryStart gathers all local information to decide if it is time to start up
backend and start sending RPCs. Among other things it checks whenever chrome has
oauth2 refresh token. Obtaining access token grom gaia is not local operation.
Requesting access token in Initialize is not enough, there are scenarios when it
needs to be requested in TryStart codepath, for example
PSS::UnsuppressAndStart(). Also requesting access token from Initialize is not
always appropriate. If tokens are not loaded yet this request is guaranteed to
fail.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:618: access_token_request_.reset();
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> On 2013/05/23 19:03:42, timsteele wrote:
> > DCHECK(access_token_request_.get()) in both these functions would be nice.
> 
> Actually, I think you should do DCHECK_EQ(access_token_request, request).

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1055: if (status ==
syncer::CONNECTION_AUTH_ERROR) {
On 2013/05/23 19:03:42, timsteele wrote:
> Comment this - is this known to be a transient access_token expiration?  Or do
> we optimistically try that first regardless?

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1810: {
On 2013/05/23 19:03:42, timsteele wrote:
> nit: { on previous line.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1812: if (access_token_request_ !=
NULL)
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> On 2013/05/23 19:03:42, timsteele wrote:
> > Think this should be a DCHECK?
> 
> I think this can happen in practice - there's nothing in the contract that
says
> you'll get exactly one OnConnectionStatusChange call, for example.
Yes, there could be multiple calls to RequestAccessToken and request might take
long time.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1823: access_token_ = std::string();
On 2013/05/23 19:03:42, timsteele wrote:
> use access_token_.clear()

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1828: access_token_request_.reset();
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> I am confused by this - on startup, won't this cause two access_token_requests
> to be initiated, since we aren't tracking our first request? Why do we ever
want
> to pass invoke_callback == true - why not just let the fetch complete and
invoke
> OnGetTokenSuccess()?
I discussed this with Tim, the question was whenever to prerequest access token
upfront hoping that access token will be in OAuth2TokenService cache by the time
it is needed. It saves roundtrip to GAIA when PSS starts up backend.
The way how I implemented it is by requesting token in Initialize but not
requesting OnGetTokenSuccess to be called because at that time it is not obvious
that backend needs to be started. When PSS is about to start up backend it will
call RequestAccessToken with invoke_callback == true.
If PSS initiates two requests OAuth2TokenService will only initiate one RPC to
GAIA for them.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1939:
GaiaConstants::kGaiaOAuth2LoginRefreshToken &&
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> I'd really like to see us get out of the "listen for TOKEN_REQUEST_FAILED"
> business. Can you put a TODO(atwilson) in the code here? There's no reason for
> sync to be reporting auth errors on behalf of TokenService - this is at best a
> leftover from our legacy signin architecture, and at worst obsolete and
> superfluous.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1957:
GaiaConstants::kGaiaOAuth2LoginRefreshToken)
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> We should really move NOTIFICATION_TOKEN_AVAILABLE to OAuth2TokenService. I've
> filed a bug for this - can you add a TODO(atwilson): Listen for notifications
on
> OAuth2TokenService (crbug.com/243737) here?

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1976: access_token_ = std::string();
On 2013/05/23 19:03:42, timsteele wrote:
> use .clear()

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:231: // 
EXPECT_CALL(*data_type_manager, Configure(_, _)).Times(0);
On 2013/05/23 19:03:42, timsteele wrote:
> Tidy this up.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:253: //
EXPECT_CALL(*sync_->components_factory_mock(),
On 2013/05/23 19:03:42, timsteele wrote:
> Tidy up these commented out expectations.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:275: //
TEST_F(ProfileSyncServiceStartupTest, StartInvalidCredentials) {
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> Should this be commented out - still seems like it's useful to test this case.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:442: printf("{PAV}
1\n");
On 2013/05/23 19:03:42, timsteele wrote:
> Remove printfs.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:495: //
TEST_F(ProfileSyncServiceStartupTest, StartDownloadFailed) {
On 2013/05/23 19:03:42, timsteele wrote:
> Fix this.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:190: //   
HandleInitializationCompletedOnFrontendLoop(false);
On 2013/05/23 19:03:42, timsteele wrote:
> Tidy up.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:247: message_loop_runner_() {
On 2013/05/23 19:03:42, timsteele wrote:
> Remove unnecessary initializer.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:276: invoke_callback);
On 2013/05/23 19:03:42, timsteele wrote:
> nit - indent is off (4 spaces).

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/test_prof...
chrome/browser/sync/test_profile_sync_service.cc:352: base::Time::Max());
On 2013/05/23 19:03:42, timsteele wrote:
> indent is off.

Done.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:342:
error_(GoogleServiceAuthError::NONE),
On 2013/05/23 19:03:42, timsteele wrote:
> indent is off (4 spaces)

Done.

https://codereview.chromium.org/15421011/diff/1/jingle/notifier/base/notifier...
File jingle/notifier/base/notifier_options_util.cc (right):

https://codereview.chromium.org/15421011/diff/1/jingle/notifier/base/notifier...
jingle/notifier/base/notifier_options_util.cc:30: if
(notifier_options.auth_mechanism == buzz::AUTH_MECHANISM_OAUTH2)
On 2013/05/23 19:03:42, timsteele wrote:
> We should use a constant (this one or otherwise) for defining
> AUTH_MECHANISM_OAUTH2 as well (vs using X-OAUTH2 literally in the code).

I changed sync_backend_host.cc to use AUTH_MECHANISM_OAUTH2.

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
File sync/engine/net/server_connection_manager.h (right):

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
sync/engine/net/server_connection_manager.h:72: // connection after AUTH_ERROR
when new access token was issued but not yet
On 2013/05/23 19:03:42, timsteele wrote:
> Issued by who?  Not sure I know what this is about?

Removed this.

https://codereview.chromium.org/15421011/diff/1/sync/engine/net/server_connec...
sync/engine/net/server_connection_manager.h:264: public:
On 2013/05/23 19:03:42, timsteele wrote:
> Move the call up to the public section instead.

I reverted this change.

https://codereview.chromium.org/15421011/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/15421011/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:605: 
On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> Hmm. I'm not sure why it's safe to move
> "observing_network_connectivity_changes_=true" down here after the early exit
> above.
> 
> I'm also not sure why we need this TOKEN_REFRESHED state - from the standpoint
> of the SyncManager why didn't the old implementation work?
if(set_auth_token) condition means "New credentials are still no good for
talking to server. I won't even try". Presumably connecting to server when
connection is restored will not succeed, thus moving
observing_network_connectivity_changes_=true.

TOKEN_REFRESHED was used in more places in earlier version of this change. I
didn't realize i don't need it anymore.

I'll revert both changes.

[Andrew T Wilson (Slow), 2013-05-31 12:57:27]

I'd really like to convince you to get rid of the token prefetch, unless Tim
insists on it. It's already causing bugs and you haven't landed the CL yet :)
Tim, do you feel strongly we need this?

I also don't really understand the need for MessageLoopRunner, but I'm going to
defer to Tim on this since he can have a more high-bandwidth conversation with
you about it than I can.

Anyhow, this is pretty close - I had a few nits and questions in the code. The
big open question for me is what we do in the case where we restart chrome, but
can't mint a sync access token - what happens then? Hopefully we don't cause an
auth error. And if a request for an access token fails for some transient
condition (network error) what causes us to try to refetch the token later? Is
sync basically broken until the next restart?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.cc:443: if (token_iterator !=
token_cache_.end() &&
On 2013/05/30 07:42:12, pavely wrote:
> On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> > On 2013/05/23 19:03:41, timsteele wrote:
> > > I guess this wasn't being used very commonly?
> > 
> > I guess not. It's mostly an optimization anyway (if someone gets an error
> using
> > a token they can invalidate it so we try to re-fetch in the future - the
fact
> > that this didn't work probably was masked by the fact that sync already
> > caught/reported auth errors).
> If server returns AUTH_ERROR before token expires then sync should invalidate
> access token with OAuth2TokenService, otherwise it is guaranteed to get the
same
> access token on consecutive token requests.

Yes. Since every token in OAuth2TokenService has an associated expiration date,
the idea is that it shouldn't be possible for the server to return an auth error
for a token prior to the OAuth2TokenService already figuring out that the token
is expired (except in the case that the user has invalid GAIA credentials/the
refresh token has been revoked).

So my point was, it's good to detect this case and tell OAuth2TokenService to
invalidate the token, but in general most services don't do this since in theory
this only happens from a race condition (service grabs token before it expires,
but then the token expires during the server request at which point
OAuth2TokenService has already expired it). That's why it doesn't surprise me
that there are bugs in this API - people aren't calling it. It's definitely more
correct to do what you are doing in Sync, though.

BTW, it'd be nice to add a test for this so it doesn't break in the future, but
I don't want to hold up this CL for that.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:109: static const char*
kOAuth2Scopes[] = {
On 2013/05/30 07:42:12, pavely wrote:
> On 2013/05/23 19:03:41, timsteele wrote:
> > These belong in google_apis / GaiaConstants.
> 
> Done.

That's too bad - I guess we need to move a little more functionality into
OAuth2TokenService then.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:111:
"https://www.googleapis.com/auth/googletalk"
So, this means we *cannot* sync if the user has the talk service disabled since
we can't mint a token. I think that's probably OK, but wanted to make sure
nobody was shocked by this (I think in some past release, we would still be able
to sync but just wouldn't get notifications but this might not be the current
behavior any more).

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:418: if (!access_token_.empty()) {
On 2013/05/30 07:42:12, pavely wrote:
> Like it was before if access token is empty then sync sends some known bad
value to server ("credentials_lost"). It is equivalent to assigning empty
access_token_, only difference is value that will be seen on the server.

Is this true? I thought that sync would generate an unrecoverable error if we
had an empty access token (that was why I added this known-bad value to begin
with - so users would have some way to fix this problem by signing back in).
Does the sync backend no longer generate an unrecoverable error when there's an
empty token?

> Let me explain cases that you listed:
- In temporary network error either PSS wouldn't start backend or token wouldn't
be cleared. 

Just curious what happens if we setup sync, start chrome, and on the restart we
can't mint a new token because of a networking error. What happens to sync? We
don't have an access token, so I don't think we can start the backend, which
seems bad (sync should in theory be able to run even if the device is offline -
it just can't hit the server).

> - In case of auth error refreshing token will cause OnGetTokenFailure which
will
> display error.

> - With local storage corruption PSS won't be able to request access token and
> will display error.

Will it? I thought we just act like the user isn't signed in now, but the logic
flow is complex so if you say it results in a visible error that's good.

> 
> UMA histograms in failure case would be "increment if user is signed in but
> login token is not present.
> Do you mind if I remove these two histogram statements?
The important thing I want is to understand the percentage of clients that don't
have a login token. I think I can get that from your proposed UMA, so if you
want to replace the existing UMA with your new one I have no objection.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:502: RequestAccessToken(false,
true);
Should we only do this if deferred_option = STARTUP_IMMEDIATE?

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:1828: access_token_request_.reset();
On 2013/05/30 07:42:12, pavely wrote:
> On 2013/05/24 14:10:15, Andrew T Wilson wrote:
> > I am confused by this - on startup, won't this cause two
access_token_requests
> > to be initiated, since we aren't tracking our first request? Why do we ever
> want
> > to pass invoke_callback == true - why not just let the fetch complete and
> invoke
> > OnGetTokenSuccess()?
> I discussed this with Tim, the question was whenever to prerequest access
token
> upfront hoping that access token will be in OAuth2TokenService cache by the
time
> it is needed. It saves roundtrip to GAIA when PSS starts up backend.

Is this a real concern? I didn't think we were that concerned about starting up
the backend immediately except in the case where we are signing in with "choose
what to sync" in which case we'd be issuing a RequestAccessToken call in short
order anyway. It just seems like we're optimizing something that doesn't need
optimization, and making the code more complex. I defer to Tim if he really
thinks this is worth the added complexity.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:117: // GoogleTalk scope is needed
for notifications
nit: add period at the end for sentence punctuation.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1881: // Invalidate previous token,
othervise token service will return the same
nit:othervise->otherwise. Also add a period at the end of the sentence.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:2034: RequestAccessToken(true,
false);
Doesn't this fetch a token on every startup even if sync is disabled? That seems
bad. Also, note that this else statement is broken (there are no brackets where
there should be).

Have I mentioned that I think doing this token prefetch is unnecessarily complex
and will lead to maintenance headaches? ;-P

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:39: 
Doesn't MessageLoop already expose is_running()? Can we use this instead of
defining this new class?

https://codereview.chromium.org/15421011/diff/14001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/15421011/diff/14001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:42: 
nit: remove extra line.

[pavely, 2013-06-04 00:49:58]

+jhawkins for browser_options_handler.cc
+zea for jingle/notifier/base/notifier_options_util.cc and UMA counter code in
PSS::OnGetTokenFailure

Drew, could you take a look?

I think I addressed all issues except for condition when to call
RequestAccessToken in PSS::StartUp. I don't want to copy condition from bottom
of function. I'll think about better solution tomorrow.

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:111:
"https://www.googleapis.com/auth/googletalk"
On 2013/05/31 12:57:28, Andrew T Wilson wrote:
> So, this means we *cannot* sync if the user has the talk service disabled
since
> we can't mint a token. I think that's probably OK, but wanted to make sure
> nobody was shocked by this (I think in some past release, we would still be
able
> to sync but just wouldn't get notifications but this might not be the current
> behavior any more).
Currently if notifications server returns auth error then SyncManager will
invalidate token for both sync and notifications so in this sense this is not a
regression.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:117: // GoogleTalk scope is needed
for notifications
On 2013/05/31 12:57:28, Andrew T Wilson wrote:
> nit: add period at the end for sentence punctuation.

Done.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1881: // Invalidate previous token,
othervise token service will return the same
On 2013/05/31 12:57:28, Andrew T Wilson wrote:
> nit:othervise->otherwise. Also add a period at the end of the sentence.

Done.

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/14001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:39: 
On 2013/05/31 12:57:28, Andrew T Wilson wrote:
> Doesn't MessageLoop already expose is_running()? Can we use this instead of
> defining this new class?

Thank you. I was reading through RunLoop trying to figure out if I can use it
here. Didn't realize the answer was so simple.

https://codereview.chromium.org/15421011/diff/14001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/15421011/diff/14001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:42: 
On 2013/05/31 12:57:28, Andrew T Wilson wrote:
> nit: remove extra line.

Done.

[Andrew T Wilson (Slow), 2013-06-04 14:37:31]

LGTM with some nits/questions.

Also looping in nyquist/qsr to comment on impact for Android/iOS.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:138: 0,
Do we want some kind of fuzzing here? I think the idea is that in the case of a
server outage we'll spread requests out over a large time period rather than
having every chrome instance try repeatedly bunched up around the 4 hour mark?

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:141: 1000 * 3600 * 4, // 4 hours.
I'm OK with this, but I wonder if maybe we should make this shorter? I'm
thinking specifically about my phone being out of cell range for a while, and
then when I get back sync won't work for 4 hours?

I think we can use a base::net::NetworkChangeNotifier to tell us if the network
state changed, which should probably trigger a retry if a retry is pending.

BTW, I think we should build this retry logic in to OAuth2TokenService rather
than having it live here - that way, on platforms like Android which have their
own retry logic built in to the AccountManager we can leverage the built-in
AccountManager retry logic instead of doing it here. I don't want to block your
change waiting on this, but please log a bug and add a TODO() here to move the
retry logic to OAuth2TokenService.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:717:
request_access_token_retry_timer_.Start(
Should we stop this timer if someone else calls RequestAccessToken() in the
meantime?

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:726: if (error.state() ==
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS) {
It's weird to check this here given that we're in a switch statement for
error.state() already. Perhaps you should add an INVALID_GAIA_CREDENTIALS case,
which does a FALL_THROUGH to call UpdateAuthErrorState()?

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1947:
token_service->InvalidateToken(oauth2_scopes, access_token_);
Should we only do this if access_token_ is non-empty?

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:532:
sync_->SetSetupInProgress(true);
Why is this necessary now?

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:262:
base::MessageLoop::current()->Quit();
Should we also add an OnGetTokenSuccess() loop that quits the loop? I'm fuzzy on
why we need one here but not in that case.

Also, I'm OK with this, but I thought the preferred way to do this is to use a
RunLoop and to invoke RunLoop::Quit() instead of talking to the messageloop
directly (this is to more gracefully handle nested runloops without exiting the
nested loop prematurely).

[qsr, 2013-06-04 15:16:45]

+chenyu

[pavely, 2013-06-04 20:11:13]

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:138: 0,
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> Do we want some kind of fuzzing here? I think the idea is that in the case of
a
> server outage we'll spread requests out over a large time period rather than
> having every chrome instance try repeatedly bunched up around the 4 hour mark?

Done.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:141: 1000 * 3600 * 4, // 4 hours.
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> I'm OK with this, but I wonder if maybe we should make this shorter? I'm
> thinking specifically about my phone being out of cell range for a while, and
> then when I get back sync won't work for 4 hours?
> 
> I think we can use a base::net::NetworkChangeNotifier to tell us if the
network
> state changed, which should probably trigger a retry if a retry is pending.
> 
> BTW, I think we should build this retry logic in to OAuth2TokenService rather
> than having it live here - that way, on platforms like Android which have
their
> own retry logic built in to the AccountManager we can leverage the built-in
> AccountManager retry logic instead of doing it here. I don't want to block
your
> change waiting on this, but please log a bug and add a TODO() here to move the
> retry logic to OAuth2TokenService.
I put 4 hours here because that's max backoff time for sync, but you are right,
sync monitors network change notifications and so should this code.
I'll put TODO here.

There is already some retry logic in OAuth2TokenService, the problem is for
transient errors it gives up after short time (about couple minutes) and code
should work if device is out of network for hours.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:717:
request_access_token_retry_timer_.Start(
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> Should we stop this timer if someone else calls RequestAccessToken() in the
> meantime?

Done.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:726: if (error.state() ==
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS) {
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> It's weird to check this here given that we're in a switch statement for
> error.state() already. Perhaps you should add an INVALID_GAIA_CREDENTIALS
case,
> which does a FALL_THROUGH to call UpdateAuthErrorState()?

Done.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1947:
token_service->InvalidateToken(oauth2_scopes, access_token_);
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> Should we only do this if access_token_ is non-empty?
OAuth2TokenService handles this case correctly and empty access token is not a
special case in OAuth2TokenService code. I wish it wasn't special cased in PSS
too.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:532:
sync_->SetSetupInProgress(true);
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> Why is this necessary now?
I think this test was initially broken. With SyncHasSetupCompleted cleared and
SetupInProgress=false PSS::TryStart wouldn't invoke StartUp at all and initial
download that is supposed to fail wouldn't be called.
I commented out this test initially because I didn't understand what this test
was testing and I needed time to understand how this whole thing works.

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:262:
base::MessageLoop::current()->Quit();
On 2013/06/04 14:37:32, Andrew T Wilson wrote:
> Should we also add an OnGetTokenSuccess() loop that quits the loop? I'm fuzzy
on
> why we need one here but not in that case.
> 
> Also, I'm OK with this, but I thought the preferred way to do this is to use a
> RunLoop and to invoke RunLoop::Quit() instead of talking to the messageloop
> directly (this is to more gracefully handle nested runloops without exiting
the
> nested loop prematurely).
In success case TryStart will be called and will result in backend
initialization and that other MessageLoop::Run call in
SyncBackendHostForProfileSyncTest::InitCore.

I actually spent quite some time trying to make it work with RunLoop, running in
different kinds of issues. I think the right approach is different.
TestProfileSyncService shouldn't invoke MessageLoop at all. Instead test should
create RunLoop on the stack, get QuitClosure from it and pass it into
TestProfileSyncService. Then TestProfileSyncService should just invoke that
Closure when it feels it is done and it's time for test to do something.
I want to implement this change but in separate CL.

[Nicolas Zea, 2013-06-04 20:13:58]

jingle and PSS UMA LGTM, with one question

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:747: 
nit: remove newline

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (left):

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:232:
UMA_HISTOGRAM_CUSTOM_TIMES("Sync.AuthInvalidationRejectedTokenAgeShort",
Is this histogram completely unnecessary now?

[pavely, 2013-06-04 21:30:45]

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (left):

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:232:
UMA_HISTOGRAM_CUSTOM_TIMES("Sync.AuthInvalidationRejectedTokenAgeShort",
On 2013/06/04 20:13:59, Nicolas Zea wrote:
> Is this histogram completely unnecessary now?
SetServerStatus(AuthError) will trigger PSS::OnConnectionStatusChange which will
call RequestAccessToken which will report the same histogram in
OnGetTokenFailure.

[commit-bot: I haz the power, 2013-06-04 21:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/39001

[Nicolas Zea, 2013-06-04 21:33:28]

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
File sync/engine/net/server_connection_manager.cc (left):

https://codereview.chromium.org/15421011/diff/22001/sync/engine/net/server_co...
sync/engine/net/server_connection_manager.cc:232:
UMA_HISTOGRAM_CUSTOM_TIMES("Sync.AuthInvalidationRejectedTokenAgeShort",
On 2013/06/04 21:30:45, pavely wrote:
> On 2013/06/04 20:13:59, Nicolas Zea wrote:
> > Is this histogram completely unnecessary now?
> SetServerStatus(AuthError) will trigger PSS::OnConnectionStatusChange which
will
> call RequestAccessToken which will report the same histogram in
> OnGetTokenFailure.

Note that this was a different histogram measuring auth failures due to the
invalidation service. It appears it no longer exists?

[commit-bot: I haz the power, 2013-06-04 21:40:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[pavely, 2013-06-04 22:32:33]

+ Roger for gaia_constants.h/gaia_constants.cc

[Roger Tawa OOO till Jul 10th, 2013-06-05 13:02:31]

Gaia constants lgtm
On Jun 4, 2013 6:32 PM, <pavely@chromium.org> wrote:

> + Roger for gaia_constants.h/gaia_**constants.cc
>
>
https://codereview.chromium.**org/15421011/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-06-05 20:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/46001

[commit-bot: I haz the power, 2013-06-05 20:25:19]

Failed to apply patch for chrome/browser/sync/profile_sync_service.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/sync/profile_sync_service.h
  Hunk #1 succeeded at 21 with fuzz 1 (offset -1 lines).
  Hunk #4 FAILED at 166.
  Hunk #5 succeeded at 252 (offset 1 line).
  Hunk #6 succeeded at 623 (offset -2 lines).
  Hunk #7 succeeded at 742 (offset -2 lines).
  Hunk #8 succeeded at 971 (offset -1 lines).
  1 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/sync/profile_sync_service.h.rej

Patch:       chrome/browser/sync/profile_sync_service.h
Index: chrome/browser/sync/profile_sync_service.h
diff --git a/chrome/browser/sync/profile_sync_service.h
b/chrome/browser/sync/profile_sync_service.h
index
a42cf167e9f3db3c56a73c36f8a1778bea739800..ef7a717cf31ff65c2945eabdf6db0235fd3323b7
100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -22,6 +22,7 @@
 #include "base/timer.h"
 #include "chrome/browser/invalidation/invalidation_frontend.h"
 #include "chrome/browser/invalidation/invalidator_storage.h"
+#include "chrome/browser/signin/oauth2_token_service.h"
 #include "chrome/browser/signin/signin_global_error.h"
 #include "chrome/browser/sync/backend_unrecoverable_error_handler.h"
 #include "chrome/browser/sync/failed_datatypes_handler.h"
@@ -38,6 +39,7 @@
 #include "content/public/browser/notification_types.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/backoff_entry.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/sync_manager_factory.h"
@@ -62,6 +64,7 @@ namespace sessions { class SyncSessionSnapshot; }
 }
 
 namespace syncer {
+class BackoffDelayProvider;
 class BaseTransaction;
 class InvalidatorRegistrar;
 struct SyncCredentials;
@@ -163,7 +166,8 @@ class ProfileSyncService : public ProfileSyncServiceBase,
                            public syncer::UnrecoverableErrorHandler,
                            public content::NotificationObserver,
                            public BrowserContextKeyedService,
-                           public invalidation::InvalidationFrontend {
+                           public invalidation::InvalidationFrontend,
+                           public OAuth2TokenService::Consumer {
  public:
   typedef browser_sync::SyncBackendHost::Status Status;
 
@@ -248,9 +252,9 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // Virtual to enable mocking in tests.
   virtual bool IsSyncEnabledAndLoggedIn();
 
-  // Return whether all sync tokens are loaded and available for the backend to
-  // start up. Virtual to enable mocking in tests.
-  virtual bool IsSyncTokenAvailable();
+  // Return whether OAuth2 refresh token is loaded and available for the
backend
+  // to start up. Virtual to enable mocking in tests.
+  virtual bool IsOAuthRefreshTokenAvailable();
 
   // Registers a data type controller with the sync service.  This
   // makes the data type controller available for use, it does not
@@ -622,6 +626,15 @@ class ProfileSyncService : public ProfileSyncServiceBase,
 
   virtual syncer::InvalidatorState GetInvalidatorState() const OVERRIDE;
 
+  // OAuth2TokenService::Consumer implementation
+  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;
+
   // BrowserContextKeyedService implementation.  This must be called exactly
   // once (before this object is destroyed).
   virtual void Shutdown() OVERRIDE;
@@ -732,6 +745,12 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // cached passphrase and clear it out when it is done.
   void ConsumeCachedPassphraseIfPossible();
 
+  // RequestAccessToken initiates RPC to request downscoped access token from
+  // refresh token. This happens when TokenService loads OAuth2 login token and
+  // when sync server returns AUTH_ERROR which indicates it is time to refresh
+  // token.
+  virtual void RequestAccessToken();
+
   // If |delete_sync_data_folder| is true, then this method will delete all
   // previous "Sync Data" folders. (useful if the folder is partial/corrupt).
   void InitializeBackend(bool delete_sync_data_folder);
@@ -954,6 +973,19 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // and association information.
   syncer::WeakHandle<syncer::DataTypeDebugInfoListener> debug_info_listener_;
 
+  // ProfileSyncService needs to remember access token in order to invalidate
it
+  // with OAuth2TokenService.
+  std::string access_token_;
+
+  // ProfileSyncService needs to hold reference to access_token_request_ for
+  // the duration of request in order to receive callbacks.
+  scoped_ptr<OAuth2TokenService::Request> access_token_request_;
+
+  // If RequestAccessToken fails with transient error then retry requesting
+  // access token with exponential backoff.
+  base::OneShotTimer<ProfileSyncService> request_access_token_retry_timer_;
+  net::BackoffEntry request_access_token_backoff_;
+
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };

[commit-bot: I haz the power, 2013-06-05 20:56:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/52001

[commit-bot: I haz the power, 2013-06-05 21:17:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/70001

[commit-bot: I haz the power, 2013-06-05 21:38:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-05 21:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/70001

[commit-bot: I haz the power, 2013-06-05 22:51:32]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-05 22:57:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/84001

[commit-bot: I haz the power, 2013-06-06 00:53:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-06 01:10:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/102001

[commit-bot: I haz the power, 2013-06-06 01:56:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-06 04:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/120001

[commit-bot: I haz the power, 2013-06-06 06:25:52]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Andrew T Wilson (Slow), 2013-06-07 13:04:42]

https://codereview.chromium.org/15421011/diff/120001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/15421011/diff/120001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:20: GaiaConstants::kSyncService,
BTW, should we stop fetching the kSyncService token and stop checking for it
here? Or do we need to leave it in place for other platforms?

[nyquist, 2013-06-07 22:39:17]

On 2013/06/07 13:04:42, Andrew T Wilson wrote:
>
https://codereview.chromium.org/15421011/diff/120001/chrome/browser/signin/si...
> File chrome/browser/signin/signin_tracker.cc (right):
> 
>
https://codereview.chromium.org/15421011/diff/120001/chrome/browser/signin/si...
> chrome/browser/signin/signin_tracker.cc:20: GaiaConstants::kSyncService,
> BTW, should we stop fetching the kSyncService token and stop checking for it
> here? Or do we need to leave it in place for other platforms?

For Android, we still request an auth token for chromiumsync. Shouldn't that be
fixed before we submit this? See SyncStatusHelper.java

[tim (not reviewing), 2013-06-12 20:07:37]

https://codereview.chromium.org/15421011/diff/135002/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/135002/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:449: use_oauth2_token_ =
command_line.HasSwitch(switches::kSyncEnableOAuth2Token);
Should the flag not be to disable OAuth2, and then set it to true on Android? 
Likely in chrome/browser/android/chrome_startup_flags.cc,
SetChromeSpecificStartupFlags?

[pavely, 2013-06-12 21:09:57]

Tim, PTAL.

[tim (not reviewing), 2013-06-12 21:54:49]

https://codereview.chromium.org/15421011/diff/153001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/153001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:221: bool
ProfileSyncService::IsOAuthRefreshTokenAvailable() {
Are we sure this isn't called from any place that depends on your new command
line flag?  I know the webui/ places are safe, because they don't exist on
Android either, but perhaps a DCHECK here that the flag is not passed.

https://codereview.chromium.org/15421011/diff/153001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:321:
UMA_HISTOGRAM_BOOLEAN("Sync.CredentialsLost",
What's the rationale for moving this from GetCredentials?

[pavely, 2013-06-12 23:12:31]

Tim,
please take a look.

https://codereview.chromium.org/15421011/diff/153001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15421011/diff/153001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:321:
UMA_HISTOGRAM_BOOLEAN("Sync.CredentialsLost",
On 2013/06/12 21:54:49, timsteele wrote:
> What's the rationale for moving this from GetCredentials?
In OAuth2 world if access_token_ is empty in GetCredentials doesn't mean that
credentials are lost. The criteria should be if refresh token is available. By
the time GetCredentials is called access_token_ is already populated and there
is no need to query TokenService for anything. So I thought that checking for
refresh token in GetCredentials is not the right place and this would be better
place.

[tim (not reviewing), 2013-06-12 23:28:45]

LGTM

+mpearson for tools/metrics/histograms OWNERS review.

[Mark P, 2013-06-12 23:31:10]

lgtm

[commit-bot: I haz the power, 2013-06-12 23:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/160001

[commit-bot: I haz the power, 2013-06-13 02:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/160001

[commit-bot: I haz the power, 2013-06-13 06:52:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 07:01:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/160001

[commit-bot: I haz the power, 2013-06-13 07:07:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Andrew T Wilson (Slow), 2013-06-13 07:57:02]

I'm concerned we've lost all test coverage for the non-oauth-token codepath. How
can we verify that we haven't broken that codepath?

Have we at least done manual testing with --sync-disable-oauth2-token (make sure
we can still sign in, sync, handle/resolve auth errors, etc)?

[commit-bot: I haz the power, 2013-06-13 13:12:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/160001

[commit-bot: I haz the power, 2013-06-13 13:12:26]

Failed to apply patch for chrome/browser/signin/signin_tracker.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/signin/signin_tracker.cc
  Hunk #1 FAILED at 175.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/signin/signin_tracker.cc.rej

Patch:       chrome/browser/signin/signin_tracker.cc
Index: chrome/browser/signin/signin_tracker.cc
diff --git a/chrome/browser/signin/signin_tracker.cc
b/chrome/browser/signin/signin_tracker.cc
index
cb3312057e4d61f09b545dde33e94bde039b8ca5..b9f80462c4155e793a20116258b9b048f20fbca4
100644
--- a/chrome/browser/signin/signin_tracker.cc
+++ b/chrome/browser/signin/signin_tracker.cc
@@ -175,7 +175,7 @@ bool SigninTracker::AreServicesSignedIn(Profile* profile) {
   // because they are transient and do not signify a failure of the signin
   // process.
   return (service->IsSyncEnabledAndLoggedIn() &&
-          service->IsSyncTokenAvailable() &&
+          service->IsOAuthRefreshTokenAvailable() &&
           (service->GetAuthError().state() == GoogleServiceAuthError::NONE ||
            service->GetAuthError().state() ==
                GoogleServiceAuthError::CONNECTION_FAILED) &&

[commit-bot: I haz the power, 2013-06-13 17:49:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/15421011/197001

[commit-bot: I haz the power, 2013-06-13 23:52:05]

Change committed as 206224