Add timeout to clearing server data

Background: the sync nudge infrastructure is highly tuned for sync scenarios -- a fire-and-forget nudge is sent tothe sync thread which kicks off sync.  If the syncer is disconnected, throttled, paused, etc, then the nudge is dropped as a sync cannot happen.

Problem: one-shot commands such as clearing server data can also be dropped without any acknowledgment, which means that the UI is never updated.  There were a few alternatives for fixing this: 1) threading the clear through, 2) adding a timeout, or 3) fixing the nudge infrastructure to support more reliable messaging semantics.  #3 was the ideal choice, however not doable for M8.  Until the syncer is redesigned, #2 is our stop-gap solution.

On success, sync is disabled and the dialog dialog is closed.  We do not care if a timeout event occurred thereafter, and theres no reason to show an error message to the user at that point.  A similar situation exists if you are looking at the dialog and someone clears from another browser: the dialog closes itself without warning and without regard to any local requests to clear.

BUG=57360
TEST=

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

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

3 files changed, 105 insertions, 4 deletions

diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 276ad22..41069c8 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -56,6 +56,8 @@ const char* ProfileSyncService::kSyncServerUrl =
 const char* ProfileSyncService::kDevServerUrl =
     "https://clients4.google.com/chrome-sync/dev";
 
+static const int kSyncClearDataTimeoutInSeconds = 60;  // 1 minute.
+
 ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
                                        Profile* profile,
                                        const std::string& cros_user)
@@ -445,6 +447,9 @@ void ProfileSyncService::Shutdown(bool sync_disabled) {
 
 void ProfileSyncService::ClearServerData() {
   clear_server_data_state_ = CLEAR_CLEARING;
+  clear_server_data_timer_.Start(
+      base::TimeDelta::FromSeconds(kSyncClearDataTimeoutInSeconds), this,
+      &ProfileSyncService::OnClearServerDataTimeout);
   backend_->RequestClearServerData();
 }
 
@@ -606,14 +611,36 @@ void ProfileSyncService::OnStopSyncingPermanently() {
   DisableForUser();
 }
 
+void ProfileSyncService::OnClearServerDataTimeout() {
+  if (clear_server_data_state_ != CLEAR_SUCCEEDED &&
+      clear_server_data_state_ != CLEAR_FAILED) {
+    clear_server_data_state_ = CLEAR_FAILED;
+    FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
+  }
+}
+
 void ProfileSyncService::OnClearServerDataFailed() {
-  clear_server_data_state_ = CLEAR_FAILED;
-  FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
+  clear_server_data_timer_.Stop();
+
+  // Only once clear has succeeded there is no longer a need to transition to
+  // a failed state as sync is disabled locally.  Also, no need to fire off
+  // the observers if the state didn't change (i.e. it was FAILED before).
+  if (clear_server_data_state_ != CLEAR_SUCCEEDED &&
+      clear_server_data_state_ != CLEAR_FAILED) {
+    clear_server_data_state_ = CLEAR_FAILED;
+    FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
+  }
 }
 
 void ProfileSyncService::OnClearServerDataSucceeded() {
-  clear_server_data_state_ = CLEAR_SUCCEEDED;
-  FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
+  clear_server_data_timer_.Stop();
+
+  // Even if the timout fired, we still transition to the succeeded state as
+  // we want UI to update itself and no longer allow the user to press "clear"
+  if (clear_server_data_state_ != CLEAR_SUCCEEDED) {
+    clear_server_data_state_ = CLEAR_SUCCEEDED;
+    FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
+  }
 }
 
 void ProfileSyncService::ShowLoginDialog(gfx::NativeWindow parent_window) {
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index e56aadc..1e9dc4d 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -14,6 +14,7 @@
 #include "base/scoped_ptr.h"
 #include "base/string16.h"
 #include "base/time.h"
+#include "base/timer.h"
 #include "chrome/browser/prefs/pref_member.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
@@ -179,6 +180,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   virtual void OnAuthError();
   virtual void OnStopSyncingPermanently();
   virtual void OnClearServerDataFailed();
+  virtual void OnClearServerDataTimeout();
   virtual void OnClearServerDataSucceeded();
 
   // Called when a user enters credentials through UI.
@@ -498,6 +500,13 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // Keep track of where we are in a server clear operation
   ClearServerDataState clear_server_data_state_;
 
+  // Timeout for the clear data command.  This timeout is a temporary hack
+  // and is necessary becaue the nudge sync framework can drop nudges for
+  // a wide variety of sync-related conditions (throttling, connections issues,
+  // syncer paused, etc.).  It can only be removed correctly when the framework
+  // is reworked to allow one-shot commands like clearing server data.
+  base::OneShotTimer<ProfileSyncService> clear_server_data_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };
 
diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc
index 42fa0e2..460a3fa 100644
--- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc
@@ -170,6 +170,71 @@ TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(SwitchManaged)) {
   profile_.GetPrefs()->ClearPref(prefs::kSyncManaged);
 }
 
+TEST_F(ProfileSyncServiceStartupTest, ClearServerData) {
+  DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
+  EXPECT_CALL(*data_type_manager, Configure(_)).Times(1);
+  EXPECT_CALL(observer_, OnStateChanged()).Times(3);
+
+  profile_.GetTokenService()->IssueAuthTokenForTest(
+      GaiaConstants::kSyncService, "sync_token");
+  service_->Initialize();
+  Mock::VerifyAndClearExpectations(data_type_manager);
+
+  // Success can overwrite failure, failure cannot overwrite success.  We want
+  // this behavior because once clear has succeeded, sync gets disabled, and
+  // we don't care about any subsequent failures (e.g. timeouts)
+  service_->ResetClearServerDataState();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_NOT_STARTED ==
+      service_->GetClearServerDataState());
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(1);
+  service_->OnClearServerDataFailed();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_FAILED ==
+      service_->GetClearServerDataState());
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(1);
+  service_->OnClearServerDataSucceeded();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED ==
+      service_->GetClearServerDataState());
+
+  service_->OnClearServerDataFailed();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED ==
+      service_->GetClearServerDataState());
+
+  // Now test the timeout states
+  service_->ResetClearServerDataState();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_NOT_STARTED ==
+      service_->GetClearServerDataState());
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(1);
+  service_->OnClearServerDataTimeout();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_FAILED ==
+      service_->GetClearServerDataState());
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(1);
+  service_->OnClearServerDataSucceeded();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED ==
+      service_->GetClearServerDataState());
+
+  service_->OnClearServerDataFailed();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_SUCCEEDED ==
+      service_->GetClearServerDataState());
+
+  // Test the pending state, doesn't matter what state
+  // the back end syncmgr returns
+  EXPECT_CALL(*data_type_manager, state()).
+    WillOnce(Return(DataTypeManager::STOPPED));
+  service_->ResetClearServerDataState();
+  service_->ClearServerData();
+  EXPECT_TRUE(ProfileSyncService::CLEAR_CLEARING ==
+      service_->GetClearServerDataState());
+
+  // Stop the timer and reset the state
+  EXPECT_CALL(observer_, OnStateChanged()).Times(1);
+  service_->OnClearServerDataSucceeded();
+  service_->ResetClearServerDataState();
+}
+
 TEST_F(ProfileSyncServiceStartupTest, SKIP_MACOSX(StartFailure)) {
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
   DataTypeManager::ConfigureResult result =