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

sync/engine/net/server_connection_manager.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 "sync/engine/net/server_connection_manager.h"
 
 #include <errno.h>
 
 #include <ostream>
 #include <string>
@@ skipping 215 matching lines @@
   active_connection_ = NULL;
 }
 
 bool ServerConnectionManager::SetAuthToken(const std::string& auth_token) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (previously_invalidated_token != auth_token) {
     auth_token_.assign(auth_token);
     previously_invalidated_token = std::string();
     return true;
   }
+
+  // This could happen in case like server outage/bug. Need to notify sync

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

What is server outage/bug?  Either explain the outage, reference the bug#, or
both.

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

Done.

+  // frontend again to request new token, otherwise backend will stay in
+  // SYNC_AUTH_ERROR state while fronend thinks everything is fine and takes
+  // no actions.
+  SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);

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

I guess the only case that is implicitly in the "else" clause of the above if is
if we call SetAuthToken with an empty token; now we'll always send up a
SYNC_AUTH_ERROR if we did that.  I distinctly recall us changing this a while
back to not always bubble up the error, although it may have been because we
were trying to avoid UI updates. Are you sure that has no implications?

Does your approach actually require tracking previously_invalidated_token?

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

Empty token shouldn't be possible because ProfileSyncService::GetCredentials()
overwrites empty token with "credentials_lost".
Without previous token, it takes one trip to sync server to find out if token is
valid or not.

   return false;
 }
 
 void ServerConnectionManager::OnInvalidationCredentialsRejected() {
   InvalidateAndClearAuthToken();
   SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
 }
 
 void ServerConnectionManager::InvalidateAndClearAuthToken() {
   DCHECK(thread_checker_.CalledOnValidThread());
   // Copy over the token to previous invalid token.
   if (!auth_token_.empty()) {
     previously_invalidated_token.assign(auth_token_);
     auth_token_ = std::string();
   }
 }
 
 void ServerConnectionManager::SetServerStatus(
     HttpResponse::ServerConnectionCode server_status) {
-  if (server_status_ == server_status)
+  // SYNC_AUTH_ERROR is permanent error. Need to notify observer to take
+  // action externally to resolve.
+  if (server_status != HttpResponse::SYNC_AUTH_ERROR &&
+      server_status_ == server_status) {

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

Pavel and Richard should review this part carefully, we've had several bugs in
this area that they have diagnosed.

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

acked

[haitaol1, 2013/11/07 15:23:52]

Another option is to leverage CONNECTION_NOT_ATTEMPTED added in 60703002. So
SetAuthToken() always resets status to CONNECTION_NOT_ATTEMPTED at beginning,
then exception can be removed here.

On 2013/11/06 22:37:30, haitaol1 wrote:

     return;
+  }
   server_status_ = server_status;
   NotifyStatusChanged();
 }
 
 void ServerConnectionManager::NotifyStatusChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(ServerConnectionEventListener, listeners_,
      OnServerConnectionEvent(
          ServerConnectionEvent(server_status_)));
 }
@@ skipping 106 matching lines @@
 
 std::ostream& operator << (std::ostream& s, const struct HttpResponse& hr) {
   s << " Response Code (bogus on error): " << hr.response_code;
   s << " Content-Length (bogus on error): " << hr.content_length;
   s << " Server Status: "
     << HttpResponse::GetServerConnectionCodeString(hr.server_status);
   return s;
 }
 
 }  // namespace syncer