Fix one case that can make sync backend stuck in auth error state.

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 625 matching lines @@
   // be there.
   InitializeBackend(!HasSyncSetupCompleted());
 }
 
 void ProfileSyncService::OnGetTokenSuccess(
     const OAuth2TokenService::Request* request,
     const std::string& access_token,
     const base::Time& expiration_time) {
   DCHECK_EQ(access_token_request_, request);
   access_token_request_.reset();
-  // Reset backoff time after successful response.
-  request_access_token_backoff_.Reset();
   access_token_ = access_token;
   if (backend_)
     backend_->UpdateCredentials(GetCredentials());
   else
     TryStart();
 }
 
 void ProfileSyncService::OnGetTokenFailure(
     const OAuth2TokenService::Request* request,
     const GoogleServiceAuthError& error) {
@@ skipping 446 matching lines @@
 }
 
 }  // namespace
 
 void ProfileSyncService::OnConnectionStatusChange(
     syncer::ConnectionStatus status) {
   if (status == syncer::CONNECTION_AUTH_ERROR) {
     // Sync server returned error indicating that access token is invalid. It
     // could be either expired or access is revoked. Let's request another
     // access token and if access is revoked then request for token will fail
-    // with corresponding error.
-    RequestAccessToken();
+    // with corresponding error. If access token is repeatedly reported
+    // invalid, there may be some issues with token server. In that case, we

[tim (not reviewing), 2013/11/06 19:35:32]

In fact, if the access token is repeatedly reported invalid there may be *no
issue* with the token server.  It's worth pointing out the subtlety that we need
to make additional (exponentially backed-off) requests to GAIA to work around
their token caching that causes us to get the same token if we ask twice in
quick succession.

We should also have a comment explaining that we take the somewhat unorthodox
approach of employing a single backoff state and timer for failures talking to
*either* GAIA or Sync.  Therefore, we're more likely to reach the backoff
ceiling more quickly than you would expect from looking at the BackoffPolicy, if
(for example) we get an auth error from sync and then lose connectivity and fail
to reach GAIA. We don't expect them to ping-pong (and crank the TimeUntilRelease
up even faster) because we won't make a sync request if we hit an error until
GAIA succeeds at sending a new token, and we won't request a new token (and risk
failure of request) unless sync reports a token failure.  (Correct me if I'm
wrong).

[haitaol1, 2013/11/06 22:37:30]

I think the root cause is still on server. If server works fine, sync should
succeed on first token. Added comment that this is due to inconsistency between
authentication server and issue server.
done

[tim (not reviewing), 2013/11/07 22:07:13]

But there could be *no issue* with the *token* server if the sync server is
erroneously saying invalid token. 

[haitaol1, 2013/11/08 15:23:02]

Removed token

On 2013/11/07 22:07:13, timsteele wrote:

+    // backoff token requests exponentially to avoid hammering token server
+    // too much.
+    if (request_access_token_backoff_.failure_count() == 0) {
+      RequestAccessToken();
+    } else {
+      request_access_token_backoff_.InformOfRequest(false);
+      request_access_token_retry_timer_.Start(
+            FROM_HERE,
+            request_access_token_backoff_.GetTimeUntilRelease(),
+            base::Bind(&ProfileSyncService::RequestAccessToken,
+                        weak_factory_.GetWeakPtr()));
+    }
   } else {
+    // Reset backoff time after successful connection.

[tim (not reviewing), 2013/11/06 19:35:32]

This is where it gets dangerous and bug prone in my opinion to only have a
single backoff tracker.  If we're backed-off due to failures talking to GAIA,
and sync reports a connection success (either legitimately or spuriously due to
a bug in the client! A delayed / racy notification, or something.) we'll wind up
bypassing the mechanism intended to protect GAIA communication.

[haitaol1, 2013/11/06 22:37:30]

Good point. Changed to only reset if no request is scheduled.

+    if (status == syncer::CONNECTION_OK)
+      request_access_token_backoff_.Reset();
+
     const GoogleServiceAuthError auth_error =
         ConnectionStatusToAuthError(status);
     DVLOG(1) << "Connection status change: " << auth_error.ToString();
     UpdateAuthErrorState(auth_error);
   }
 }
 
 void ProfileSyncService::OnStopSyncingPermanently() {
   sync_prefs_.SetStartSuppressed(true);
   DisableForUser();
@@ skipping 1007 matching lines @@
     NOTREACHED();
 #endif
   }
 
   return signin_->GetAuthenticatedUsername();
 }
 
 WeakHandle<syncer::JsEventHandler> ProfileSyncService::GetJsEventHandler() {
   return MakeWeakHandle(sync_js_controller_.AsWeakPtr());
 }