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

8 files changed, 168 insertions, 78 deletions

diff --git a/chrome/browser/sync/engine/authenticator.cc b/chrome/browser/sync/engine/authenticator.cc
index 455e153..c2d79d3 100644
--- a/chrome/browser/sync/engine/authenticator.cc
+++ b/chrome/browser/sync/engine/authenticator.cc
@@ -71,7 +71,9 @@ Authenticator::AuthenticationResult Authenticator::AuthenticateToken(
 
   ServerConnectionManager::PostBufferParams params =
     { tx, &rx, &http_response };
-  if (!server_connection_manager_->PostBufferWithAuth(&params, auth_token)) {
+  ScopedServerStatusWatcher watch(server_connection_manager_, &http_response);
+  if (!server_connection_manager_->PostBufferWithAuth(&params, auth_token,
+                                                      &watch)) {
     LOG(WARNING) << "Error posting from authenticator:" << http_response;
     return SERVICE_DOWN;
   }
@@ -90,6 +92,8 @@ Authenticator::AuthenticationResult Authenticator::AuthenticateToken(
       return USER_NOT_ACTIVATED;
     case sync_pb::ClientToServerResponse::AUTH_INVALID:
     case sync_pb::ClientToServerResponse::AUTH_EXPIRED:
+      // TODO(tim): This is an egregious layering violation (bug 35060).
+      http_response.server_status = HttpResponse::SYNC_AUTH_ERROR;
       return BAD_AUTH_TOKEN;
     // should never happen (no birthday in this request).
     case sync_pb::ClientToServerResponse::NOT_MY_BIRTHDAY:
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.
diff --git a/chrome/browser/sync/engine/syncer_command.cc b/chrome/browser/sync/engine/syncer_command.cc
index 951c138..09d4842 100644
--- a/chrome/browser/sync/engine/syncer_command.cc
+++ b/chrome/browser/sync/engine/syncer_command.cc
@@ -41,14 +41,6 @@ void SyncerCommand::SendNotifications(SyncSession* session) {
       session->context()->syncer_event_channel()->NotifyListeners(quota_event);
     }
   }
-  if (session->auth_failure_occurred()) {
-    ServerConnectionEvent event;
-    event.what_happened = ServerConnectionEvent::STATUS_CHANGED;
-    event.server_reachable = true;
-    event.connection_code = HttpResponse::SYNC_AUTH_ERROR;
-    session->context()->connection_manager()->channel()->NotifyListeners(event);
-    session->clear_auth_failure_occurred();
-  }
 }
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/syncer_proto_util.cc b/chrome/browser/sync/engine/syncer_proto_util.cc
index 41678bf..d6fec77 100755
--- a/chrome/browser/sync/engine/syncer_proto_util.cc
+++ b/chrome/browser/sync/engine/syncer_proto_util.cc
@@ -118,7 +118,8 @@ bool SyncerProtoUtil::PostClientToServerMessage(ClientToServerMessage* msg,
   };
 
   ServerConnectionManager* scm = session->context()->connection_manager();
-  if (!scm->PostBufferWithCachedAuth(&params)) {
+  ScopedServerStatusWatcher server_status_watcher(scm, &http_response);
+  if (!scm->PostBufferWithCachedAuth(&params, &server_status_watcher)) {
     LOG(WARNING) << "Error posting from syncer:" << http_response;
   } else {
     rv = response->ParseFromString(rx);
@@ -143,9 +144,9 @@ bool SyncerProtoUtil::PostClientToServerMessage(ClientToServerMessage* msg,
       case ClientToServerResponse::USER_NOT_ACTIVATED:
       case ClientToServerResponse::AUTH_INVALID:
       case ClientToServerResponse::ACCESS_DENIED:
-        LOG(INFO) << "Authentication expired, re-requesting";
-        LOG(INFO) << "Not implemented in syncer yet!!!";
-        session->set_auth_failure_occurred();
+        LOG(INFO) << "SyncerProtoUtil: Authentication expired.";
+        // TODO(tim): This is an egregious layering violation (bug 35060).
+        http_response.server_status = HttpResponse::SYNC_AUTH_ERROR;
         rv = false;
         break;
       case ClientToServerResponse::NOT_MY_BIRTHDAY:
diff --git a/chrome/browser/sync/engine/syncer_thread.cc b/chrome/browser/sync/engine/syncer_thread.cc
index 92048bb..b89133d 100644
--- a/chrome/browser/sync/engine/syncer_thread.cc
+++ b/chrome/browser/sync/engine/syncer_thread.cc
@@ -33,8 +33,18 @@ using base::TimeTicks;
 
 namespace browser_sync {
 
-const int SyncerThread::kDefaultShortPollIntervalSeconds = 60;
-const int SyncerThread::kDefaultLongPollIntervalSeconds = 3600;
+// We use high values here to ensure that failure to receive poll updates from
+// the server doesn't result in rapid-fire polling from the client due to low
+// local limits.
+const int SyncerThread::kDefaultShortPollIntervalSeconds = 3600 * 8;
+const int SyncerThread::kDefaultLongPollIntervalSeconds = 3600 * 12;
+
+// TODO(tim): This is used to regulate the short poll (when notifications are
+// disabled) based on user idle time.  If it is set to a smaller value than
+// the short poll interval, it basically does nothing; for now, this is what
+// we want and allows stronger control over the poll rate from the server. We
+// should probably re-visit this code later and figure out if user idle time
+// is really something we want and make sure it works, if it is.
 const int SyncerThread::kDefaultMaxPollIntervalMs = 30 * 60 * 1000;
 
 void SyncerThread::NudgeSyncer(int milliseconds_from_now, NudgeSource source) {
@@ -450,11 +460,24 @@ void SyncerThread::HandleDirectoryManagerEvent(
   }
 }
 
+// Sets |*connected| to false if it is currently true but |code| suggests that
+// the current network configuration and/or auth state cannot be used to make
+// forward progress, and user intervention (e.g changing server URL or auth
+// credentials) is likely necessary.  If |*connected| is false, set it to true
+// if |code| suggests that we just recently made healthy contact with the
+// server.
 static inline void CheckConnected(bool* connected,
                                   HttpResponse::ServerConnectionCode code,
                                   ConditionVariable* condvar) {
   if (*connected) {
-    if (HttpResponse::CONNECTION_UNAVAILABLE == code) {
+    // Note, be careful when adding cases here because if the SyncerThread
+    // thinks there is no valid connection as determined by this method, it
+    // will drop out of *all* forward progress sync loops (it won't poll and it
+    // will queue up Talk notifications but not actually call SyncShare) until
+    // some external action causes a ServerConnectionManager to broadcast that
+    // a valid connection has been re-established.
+    if (HttpResponse::CONNECTION_UNAVAILABLE == code ||
+        HttpResponse::SYNC_AUTH_ERROR == code) {
       *connected = false;
       condvar->Broadcast();
     }
diff --git a/chrome/browser/sync/engine/syncer_thread.h b/chrome/browser/sync/engine/syncer_thread.h
index e23074c..5d06bc2 100644
--- a/chrome/browser/sync/engine/syncer_thread.h
+++ b/chrome/browser/sync/engine/syncer_thread.h
@@ -54,6 +54,7 @@ class SyncerThread : public base::RefCountedThreadSafe<SyncerThread>,
   FRIEND_TEST(SyncerThreadWithSyncerTest, Polling);
   FRIEND_TEST(SyncerThreadWithSyncerTest, Nudge);
   FRIEND_TEST(SyncerThreadWithSyncerTest, Throttling);
+  FRIEND_TEST(SyncerThreadWithSyncerTest, AuthInvalid);
   friend class SyncerThreadWithSyncerTest;
   friend class SyncerThreadFactory;
  public:
diff --git a/chrome/browser/sync/engine/syncer_thread_unittest.cc b/chrome/browser/sync/engine/syncer_thread_unittest.cc
index 54ea742..d9da657 100644
--- a/chrome/browser/sync/engine/syncer_thread_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_thread_unittest.cc
@@ -73,6 +73,20 @@ class SyncerThreadWithSyncerTest : public testing::Test,
       sync_cycle_ended_event_.Wait();
   }
 
+  void WaitForDisconnect() {
+    // Wait for the SyncerThread to detect loss of connection, up to a max of
+    // 10 seconds to timeout the test.
+    AutoLock lock(syncer_thread()->lock_);
+    TimeTicks start = TimeTicks::HighResNow();
+    TimeDelta ten_seconds = TimeDelta::FromSeconds(10);
+    while (syncer_thread()->vault_.connected_) {
+      syncer_thread()->vault_field_changed_.TimedWait(ten_seconds);
+      if (TimeTicks::HighResNow() - start > ten_seconds)
+        break;
+    }
+    EXPECT_FALSE(syncer_thread()->vault_.connected_);
+  }
+
  private:
 
   void HandleSyncerEvent(const SyncerEvent& event) {
@@ -90,8 +104,9 @@ class SyncerThreadWithSyncerTest : public testing::Test,
   DISALLOW_COPY_AND_ASSIGN(SyncerThreadWithSyncerTest);
 };
 
-class SyncShareIntercept : public MockConnectionManager::ThrottleRequestVisitor,
-                           public MockConnectionManager::MidCommitObserver {
+class SyncShareIntercept
+    : public MockConnectionManager::ResponseCodeOverrideRequestor,
+      public MockConnectionManager::MidCommitObserver {
  public:
   SyncShareIntercept() : sync_occured_(false, false),
                          allow_multiple_interceptions_(true) {}
@@ -103,9 +118,10 @@ class SyncShareIntercept : public MockConnectionManager::ThrottleRequestVisitor,
     sync_occured_.Signal();
   }
 
-  // ThrottleRequestVisitor implementation.
-  virtual void VisitAtomically() {
-    // Server has told the client to throttle.  We should not see any syncing.
+  // ResponseCodeOverrideRequestor implementation. This assumes any override
+  // requested is intended to silence the SyncerThread.
+  virtual void OnOverrideComplete() {
+    // We should not see any syncing.
     allow_multiple_interceptions_ = false;
     times_sync_occured_.clear();
   }
@@ -117,6 +133,12 @@ class SyncShareIntercept : public MockConnectionManager::ThrottleRequestVisitor,
   std::vector<TimeTicks> times_sync_occured() const {
     return times_sync_occured_;
   }
+
+  void Reset() {
+    allow_multiple_interceptions_ = true;
+    times_sync_occured_.clear();
+    sync_occured_.Reset();
+  }
  private:
   std::vector<TimeTicks> times_sync_occured_;
   base::WaitableEvent sync_occured_;
@@ -638,4 +660,46 @@ TEST_F(SyncerThreadWithSyncerTest, Throttling) {
   EXPECT_TRUE(syncer_thread()->Stop(2000));
 }
 
+TEST_F(SyncerThreadWithSyncerTest, AuthInvalid) {
+  SyncShareIntercept interceptor;
+  connection()->SetMidCommitObserver(&interceptor);
+  const TimeDelta poll_interval = TimeDelta::FromMilliseconds(1);
+
+  syncer_thread()->SetSyncerShortPollInterval(poll_interval);
+  EXPECT_TRUE(syncer_thread()->Start());
+  metadb()->Open();
+
+  // Wait for some healthy syncing.
+  interceptor.WaitForSyncShare(2, TimeDelta::FromSeconds(10));
+  EXPECT_GE(interceptor.times_sync_occured().size(), 2U);
+
+  // Atomically start returning auth invalid and set the interceptor to fail
+  // on any sync.
+  connection()->FailWithAuthInvalid(&interceptor);
+  WaitForDisconnect();
+
+  // Try to trigger a sync (the interceptor will assert if one occurs).
+  syncer_thread()->NudgeSyncer(0, SyncerThread::kUnknown);
+  syncer_thread()->NudgeSyncer(0, SyncerThread::kUnknown);
+
+  // Wait several poll intervals but don't expect any syncing besides the cycle
+  // that lost the connection.
+  interceptor.WaitForSyncShare(1, TimeDelta::FromSeconds(1));
+  EXPECT_EQ(1U, interceptor.times_sync_occured().size());
+
+  // Simulate a valid re-authentication and expect resumption of syncing.
+  interceptor.Reset();
+  ASSERT_TRUE(interceptor.times_sync_occured().empty());
+  connection()->StopFailingWithAuthInvalid(NULL);
+  ServerConnectionEvent e = {ServerConnectionEvent::STATUS_CHANGED,
+                             HttpResponse::SERVER_CONNECTION_OK,
+                             true};
+  connection()->channel()->NotifyListeners(e);
+
+  interceptor.WaitForSyncShare(1, TimeDelta::FromSeconds(10));
+  EXPECT_FALSE(interceptor.times_sync_occured().empty());
+
+  EXPECT_TRUE(syncer_thread()->Stop(2000));
+}
+
 }  // namespace browser_sync