Make SyncerThread stop polling if an AUTH_INVALID response is parsed.

BUG=34396
TEST=SyncerTHreadWithSyncerTest.AuthInvalid


Review URL: http://codereview.chromium.org/596007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38513 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 62 insertions, 57 deletions

diff --git a/chrome/browser/sync/engine/net/server_connection_manager.cc b/chrome/browser/sync/engine/net/server_connection_manager.cc
index 95a4701..e89e104 100644
--- a/chrome/browser/sync/engine/net/server_connection_manager.cc
+++ b/chrome/browser/sync/engine/net/server_connection_manager.cc
@@ -108,38 +108,29 @@ int ServerConnectionManager::Post::ReadResponse(string* out_buffer,
   return bytes_read;
 }
 
-// A helper class that automatically notifies when the status changes.
-class WatchServerStatus {
- public:
-  WatchServerStatus(ServerConnectionManager* conn_mgr, HttpResponse* response)
-      : conn_mgr_(conn_mgr),
-        response_(response),
-        reset_count_(conn_mgr->reset_count_),
-        server_reachable_(conn_mgr->server_reachable_) {
-    response->server_status = conn_mgr->server_status_;
-  }
-  ~WatchServerStatus() {
-    // Don't update the status of the connection if it has been reset.
-    // TODO(timsteele): Do we need this? Is this used by multiple threads?
-    if (reset_count_ != conn_mgr_->reset_count_)
-      return;
-    if (conn_mgr_->server_status_ != response_->server_status) {
-      conn_mgr_->server_status_ = response_->server_status;
-      conn_mgr_->NotifyStatusChanged();
-      return;
-    }
-    // Notify if we've gone on or offline.
-    if (server_reachable_ != conn_mgr_->server_reachable_)
-      conn_mgr_->NotifyStatusChanged();
-  }
+ScopedServerStatusWatcher::ScopedServerStatusWatcher(
+    ServerConnectionManager* conn_mgr, HttpResponse* response)
+    : conn_mgr_(conn_mgr),
+      response_(response),
+      reset_count_(conn_mgr->reset_count_),
+      server_reachable_(conn_mgr->server_reachable_) {
+  response->server_status = conn_mgr->server_status_;
+}
 
- private:
-  ServerConnectionManager* const conn_mgr_;
-  HttpResponse* const response_;
-  // TODO(timsteele): Should this be Barrier:AtomicIncrement?
-  base::subtle::AtomicWord reset_count_;
-  bool server_reachable_;
-};
+ScopedServerStatusWatcher::~ScopedServerStatusWatcher() {
+  // Don't update the status of the connection if it has been reset.
+  // TODO(timsteele): Do we need this? Is this used by multiple threads?
+  if (reset_count_ != conn_mgr_->reset_count_)
+    return;
+  if (conn_mgr_->server_status_ != response_->server_status) {
+    conn_mgr_->server_status_ = response_->server_status;
+    conn_mgr_->NotifyStatusChanged();
+    return;
+  }
+  // Notify if we've gone on or offline.
+  if (server_reachable_ != conn_mgr_->server_reachable_)
+    conn_mgr_->NotifyStatusChanged();
+}
 
 ServerConnectionManager::ServerConnectionManager(
     const string& server,
@@ -175,24 +166,24 @@ void ServerConnectionManager::NotifyStatusChanged() {
 
 // Uses currently set auth token. Set by AuthWatcher.
 bool ServerConnectionManager::PostBufferWithCachedAuth(
-    const PostBufferParams* params) {
+    const PostBufferParams* params, ScopedServerStatusWatcher* watcher) {
   string path =
       MakeSyncServerPath(proto_sync_path(), MakeSyncQueryString(client_id_));
-  return PostBufferToPath(params, path, auth_token_);
+  return PostBufferToPath(params, path, auth_token_, watcher);
 }
 
 bool ServerConnectionManager::PostBufferWithAuth(const PostBufferParams* params,
-                                                 const string& auth_token) {
+    const string& auth_token, ScopedServerStatusWatcher* watcher) {
   string path = MakeSyncServerPath(proto_sync_path(),
                                    MakeSyncQueryString(client_id_));
 
-  return PostBufferToPath(params, path, auth_token);
+  return PostBufferToPath(params, path, auth_token, watcher);
 }
 
 bool ServerConnectionManager::PostBufferToPath(const PostBufferParams* params,
-                                               const string& path,
-                                               const string& auth_token) {
-  WatchServerStatus watcher(this, params->response);
+    const string& path, const string& auth_token,
+    ScopedServerStatusWatcher* watcher) {
+  DCHECK(watcher != NULL);
   scoped_ptr<Post> post(MakePost());
   post->set_timing_info(params->timing_info);
   bool ok = post->Init(path.c_str(), auth_token, params->buffer_in,
@@ -216,7 +207,7 @@ bool ServerConnectionManager::CheckTime(int32* out_time) {
   // to do this because of wifi interstitials that intercept messages from the
   // client and return HTTP OK instead of a redirect.
   HttpResponse response;
-  WatchServerStatus watcher(this, &response);
+  ScopedServerStatusWatcher watcher(this, &response);
   string post_body = "command=get_time";
 
   // We only retry the CheckTime call if we were reset during the CheckTime
@@ -288,12 +279,6 @@ void ServerConnectionManager::kill() {
   }
 }
 
-void ServerConnectionManager::ResetAuthStatus() {
-  ResetConnection();
-  server_status_ = HttpResponse::NONE;
-  NotifyStatusChanged();
-}
-
 void ServerConnectionManager::ResetConnection() {
   base::subtle::NoBarrier_AtomicIncrement(&reset_count_, 1);
 }
diff --git a/chrome/browser/sync/engine/net/server_connection_manager.h b/chrome/browser/sync/engine/net/server_connection_manager.h
index 28a2e4e..200fa2e 100644
--- a/chrome/browser/sync/engine/net/server_connection_manager.h
+++ b/chrome/browser/sync/engine/net/server_connection_manager.h
@@ -58,7 +58,11 @@ struct HttpResponse {
     SYNC_SERVER_ERROR,
 
     // SYNC_AUTH_ERROR is returned when the HTTP status code indicates that an
-    // auth error has occured (i.e. a 401)
+    // auth error has occured (i.e. a 401 or sync-specific AUTH_INVALID
+    // response)
+    // TODO(tim): Caring about AUTH_INVALID is a layering violation. But
+    // this app-specific logic is being added as a stable branch hotfix so
+    // minimal changes prevail for the moment.  Fix this! Bug 35060.
     SYNC_AUTH_ERROR,
 
     // All the following connection codes are valid responses from the server.
@@ -110,7 +114,23 @@ struct ServerConnectionEvent {
   bool server_reachable;
 };
 
-class WatchServerStatus;
+class ServerConnectionManager;
+// A helper class that automatically notifies when the status changes.
+// TODO(tim): This class shouldn't be exposed outside of the implementation,
+// bug 35060.
+class ScopedServerStatusWatcher {
+ public:
+  ScopedServerStatusWatcher(ServerConnectionManager* conn_mgr,
+                            HttpResponse* response);
+  ~ScopedServerStatusWatcher();
+ private:
+  ServerConnectionManager* const conn_mgr_;
+  HttpResponse* const response_;
+  // TODO(tim): Should this be Barrier:AtomicIncrement?
+  base::subtle::AtomicWord reset_count_;
+  bool server_reachable_;
+  DISALLOW_COPY_AND_ASSIGN(ScopedServerStatusWatcher);
+};
 
 // Use this class to interact with the sync server.
 // The ServerConnectionManager currently supports POSTing protocol buffers.
@@ -191,14 +211,16 @@ class ServerConnectionManager {
   // set auth token in our headers.
   //
   // Returns true if executed successfully.
-  virtual bool PostBufferWithCachedAuth(const PostBufferParams* params);
+  virtual bool PostBufferWithCachedAuth(const PostBufferParams* params,
+                                        ScopedServerStatusWatcher* watcher);
 
   // POSTS buffer_in and reads a response into buffer_out. Add a specific auth
   // token to http headers.
   //
   // Returns true if executed successfully.
   virtual bool PostBufferWithAuth(const PostBufferParams* params,
-                                  const std::string& auth_token);
+                                  const std::string& auth_token,
+                                  ScopedServerStatusWatcher* watcher);
 
   // Checks the time on the server. Returns false if the request failed. |time|
   // is an out parameter that stores the value returned from the server.
@@ -231,12 +253,6 @@ class ServerConnectionManager {
 
   inline bool server_reachable() const { return server_reachable_; }
 
-  void ResetAuthStatus();
-
-  void ResetConnection();
-
-  void NotifyStatusChanged();
-
   const std::string client_id() const { return client_id_; }
 
   void SetDomainFromSignIn(SignIn signin_type, const std::string& signin);
@@ -296,7 +312,8 @@ class ServerConnectionManager {
   // Internal PostBuffer base function.
   virtual bool PostBufferToPath(const PostBufferParams*,
                                 const std::string& path,
-                                const std::string& auth_token);
+                                const std::string& auth_token,
+                                ScopedServerStatusWatcher* watcher);
 
   // Protects access to sync_server_, sync_server_port_ and use_ssl_:
   mutable Lock server_parameters_mutex_;
@@ -339,7 +356,10 @@ class ServerConnectionManager {
 
  private:
   friend class Post;
-  friend class WatchServerStatus;
+  friend class ScopedServerStatusWatcher;
+
+  void NotifyStatusChanged();
+  void ResetConnection();
 
   mutable Lock terminate_all_io_mutex_;
   bool terminate_all_io_;  // When set to true, terminate all connections asap.