[Sync] Fix auth error handling while the backend is initializing.

This changes it so that we no longer throw an unrecoverable error if we get an
auth error while bringing up the backend (which can happen if we fail to
download control types). We now let just let the backend continuing retrying
(in backoff mode), until the user resolves the auth error, at which point
we'll perform our pending sync cycle, finishing backend initialization.

BUG=163491


Review URL: https://chromiumcodereview.appspot.com/11428125

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

chrome/browser/sync/glue/sync_backend_host.cc

@@ -460,6 +460,7 @@ void SyncBackendHost::Initialize(
 }
 
 void SyncBackendHost::UpdateCredentials(const SyncCredentials& credentials) {
+  DCHECK(sync_thread_.IsRunning());
   sync_thread_.message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoUpdateCredentials, core_.get(),
                  credentials));
@@ -487,6 +488,7 @@ void SyncBackendHost::StartSyncingWithServer() {
 
 void SyncBackendHost::SetEncryptionPassphrase(const std::string& passphrase,
                                               bool is_explicit) {
+  DCHECK(sync_thread_.IsRunning());
   if (!IsNigoriEnabled()) {
     NOTREACHED() << "SetEncryptionPassphrase must never be called when nigori"
                     " is disabled.";

chrome/browser/sync/glue/sync_backend_host.h

@@ -181,7 +181,7 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
           report_unrecoverable_error_function);
 
   // Called on |frontend_loop| to update SyncCredentials.
-  void UpdateCredentials(const syncer::SyncCredentials& credentials);
+  virtual void UpdateCredentials(const syncer::SyncCredentials& credentials);
 
   // Registers the underlying frontend for the given IDs to the underlying
   // notifier.  This lasts until StopSyncingForShutdown() is called.

chrome/browser/sync/profile_sync_service.cc

@@ -278,7 +278,6 @@ void ProfileSyncService::TryStart() {
 
   // All systems Go for launch.
   StartUp();
-
 }
 
 void ProfileSyncService::StartSyncingWithServer() {
@@ -422,23 +421,11 @@ void ProfileSyncService::OnSyncConfigureDone(
 }
 
 void ProfileSyncService::OnSyncConfigureRetry() {
-  // In platforms with auto start we would just wait for the
-  // configure to finish. In other platforms we would throw
-  // an unrecoverable error. The reason we do this is so that
-  // the login dialog would show an error and the user would have
-  // to relogin.
-  // Also if backend has been initialized(the user is authenticated
-  // and nigori is downloaded) we would simply wait rather than going into
-  // unrecoverable error, even if the platform has auto start disabled.
-  // Note: In those scenarios the UI does not wait for the configuration
-  // to finish.
-  if (!auto_start_enabled_ && !backend_initialized_) {
-    OnInternalUnrecoverableError(FROM_HERE,
-                                 "Configure failed to download.",
-                                 true,
-                                 ERROR_REASON_CONFIGURATION_RETRY);
-  }
-
+  // Note: in order to handle auth failures that arise before the backend is
+  // initialized (e.g. from invalidation notifier, or downloading new control
+  // types), we have to gracefully handle configuration retries at all times.
+  // At this point an auth error badge should be shown, which once resolved
+  // will trigger a new sync cycle.
   NotifyObservers();
 }
 
@@ -1776,7 +1763,7 @@ void ProfileSyncService::Observe(int type,
       // Initialize the backend if sync is enabled. If the sync token was
       // not loaded, GetCredentials() will generate invalid credentials to
       // cause the backend to generate an auth error (crbug.com/121755).
-      if (backend_initialized_)
+      if (backend_.get())
         backend_->UpdateCredentials(GetCredentials());
       else
         TryStart();

chrome/browser/sync/profile_sync_service_startup_unittest.cc

@@ -235,6 +235,42 @@ TEST_F(ProfileSyncServiceStartupTest, StartNoCredentials) {
   EXPECT_TRUE(service_->ShouldPushChanges());
 }
 
+TEST_F(ProfileSyncServiceStartupTest, StartInvalidCredentials) {
+  DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
+  EXPECT_CALL(*data_type_manager, Configure(_, _)).Times(0);
+  TokenService* token_service = static_cast<TokenService*>(
+      TokenServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+          profile_.get(), BuildFakeTokenService));
+  token_service->LoadTokensFromDB();
+
+  // Tell the backend to stall while downloading control types (simulating an
+  // auth error).
+  service_->fail_initial_download();
+
+  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
+  service_->Initialize();
+  EXPECT_TRUE(service_->GetBackendForTest());
+  EXPECT_FALSE(service_->sync_initialized());
+  EXPECT_FALSE(service_->ShouldPushChanges());
+  Mock::VerifyAndClearExpectations(data_type_manager);
+
+  // Update the credentials, unstalling the backend.
+  EXPECT_CALL(*data_type_manager, Configure(_, _));
+  EXPECT_CALL(*data_type_manager, state()).
+      WillRepeatedly(Return(DataTypeManager::CONFIGURED));
+  EXPECT_CALL(*data_type_manager, Stop()).Times(1);
+  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
+  service_->SetSetupInProgress(true);
+  service_->signin()->StartSignIn("test_user", "", "", "");
+  token_service->IssueAuthTokenForTest(
+      GaiaConstants::kSyncService, "sync_token");
+  service_->SetSetupInProgress(false);
+  MessageLoop::current()->Run();
+
+  // Verify we successfully finish startup and configuration.
+  EXPECT_TRUE(service_->ShouldPushChanges());
+}
+
 TEST_F(ProfileSyncServiceStartupCrosTest, StartCrosNoCredentials) {
   EXPECT_CALL(*factory_mock(), CreateDataTypeManager(_, _, _, _)).Times(0);
   profile_->GetPrefs()->ClearPref(prefs::kSyncHasSetupCompleted);

chrome/browser/sync/test_profile_sync_service.cc

@@ -43,11 +43,12 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest(
     syncer::StorageOption storage_option)
     : browser_sync::SyncBackendHost(
         profile->GetDebugName(), profile, sync_prefs, invalidator_storage),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       id_factory_(id_factory),
       callback_(callback),
+      fail_initial_download_(fail_initial_download),
       set_initial_sync_ended_on_init_(set_initial_sync_ended_on_init),
       synchronous_init_(synchronous_init),
-      fail_initial_download_(fail_initial_download),
       storage_option_(storage_option) {}
 
 SyncBackendHostForProfileSyncTest::~SyncBackendHostForProfileSyncTest() {}
@@ -89,6 +90,15 @@ void SyncBackendHostForProfileSyncTest::InitCore(
   }
 }
 
+void SyncBackendHostForProfileSyncTest::UpdateCredentials(
+      const syncer::SyncCredentials& credentials) {
+  // If we had failed the initial download, complete initialization now.
+  if (!initial_download_closure_.is_null()) {
+    initial_download_closure_.Run();
+    initial_download_closure_.Reset();
+  }
+}
+
 void SyncBackendHostForProfileSyncTest::RequestConfigureSyncer(
     syncer::ConfigureReason reason,
     syncer::ModelTypeSet types_to_config,
@@ -104,7 +114,7 @@ void SyncBackendHostForProfileSyncTest::RequestConfigureSyncer(
 }
 
 void SyncBackendHostForProfileSyncTest
-        ::HandleSyncManagerInitializationOnFrontendLoop(
+    ::HandleSyncManagerInitializationOnFrontendLoop(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend,
     const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&
         debug_info_listener,
@@ -141,8 +151,20 @@ void SyncBackendHostForProfileSyncTest
     restored_types = syncer::ModelTypeSet::All();
   }
 
-  SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
-      js_backend, debug_info_listener, restored_types);
+  initial_download_closure_ = base::Bind(
+      &SyncBackendHostForProfileSyncTest::ContinueInitialization,
+      weak_ptr_factory_.GetWeakPtr(),
+      js_backend,
+      debug_info_listener,
+      restored_types);
+  if (fail_initial_download_) {
+    frontend()->OnSyncConfigureRetry();
+    if (synchronous_init_)
+      MessageLoop::current()->Quit();
+  } else {
+    initial_download_closure_.Run();
+    initial_download_closure_.Reset();
+  }
 }
 
 void SyncBackendHostForProfileSyncTest::SetInitialSyncEndedForAllTypes() {
@@ -167,6 +189,15 @@ void SyncBackendHostForProfileSyncTest::EmitOnIncomingInvalidation(
   frontend()->OnIncomingInvalidation(invalidation_map, source);
 }
 
+void SyncBackendHostForProfileSyncTest::ContinueInitialization(
+    const syncer::WeakHandle<syncer::JsBackend>& js_backend,
+    const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&
+        debug_info_listener,
+    syncer::ModelTypeSet restored_types) {
+  SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
+      js_backend, debug_info_listener, restored_types);
+}
+
 }  // namespace browser_sync
 
 syncer::TestIdFactory* TestProfileSyncService::id_factory() {

chrome/browser/sync/test_profile_sync_service.h

@@ -48,6 +48,9 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost {
 
   MOCK_METHOD1(RequestNudge, void(const tracked_objects::Location&));
 
+  virtual void UpdateCredentials(
+      const syncer::SyncCredentials& credentials) OVERRIDE;
+
   virtual void RequestConfigureSyncer(
       syncer::ConfigureReason reason,
       syncer::ModelTypeSet types_to_config,
@@ -74,12 +77,31 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost {
   virtual void InitCore(const DoInitializeOptions& options) OVERRIDE;
 
  private:
+  void ContinueInitialization(
+      const syncer::WeakHandle<syncer::JsBackend>& js_backend,
+      const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>&
+          debug_info_listener,
+      syncer::ModelTypeSet restored_types);
+
+  base::WeakPtrFactory<SyncBackendHostForProfileSyncTest> weak_ptr_factory_;
+
   syncer::TestIdFactory& id_factory_;
+
+  // Invoked at the start of HandleSyncManagerInitializationOnFrontendLoop.
+  // Allows extra initialization work to be performed before the backend comes
+  // up.
   base::Closure& callback_;
 
+  // Saved closure in case we failed the initial download but then received
+  // new credentials. Holds the results of
+  // HandleSyncManagerInitializationOnFrontendLoop, and if
+  // |fail_initial_download_| was true, finishes the initialization process
+  // once we receive new credentials.
+  base::Closure initial_download_closure_;
+  bool fail_initial_download_;
+
   bool set_initial_sync_ended_on_init_;
   bool synchronous_init_;
-  bool fail_initial_download_;
   syncer::StorageOption storage_option_;
 };
 
@@ -118,7 +140,9 @@ class TestProfileSyncService : public ProfileSyncService {
   void dont_set_initial_sync_ended_on_init();
   void set_synchronous_sync_configuration();
 
+  // Fails initial download until a new auth token is provided.
   void fail_initial_download();
+
   void set_storage_option(syncer::StorageOption option);
 
   syncer::TestIdFactory* id_factory();