[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796

Review URL: https://codereview.chromium.org/1575153004

Cr-Commit-Position: refs/heads/master@{#380685}

8 files changed, 127 insertions, 165 deletions

diff --git a/chrome/browser/sync/test/integration/sync_test.cc b/chrome/browser/sync/test/integration/sync_test.cc
index 2aaedad..259efd6 100644
--- a/chrome/browser/sync/test/integration/sync_test.cc
+++ b/chrome/browser/sync/test/integration/sync_test.cc
@@ -590,14 +590,18 @@ void SyncTest::InitializeInvalidations(int index) {
 bool SyncTest::SetupSync() {
   // Create sync profiles and clients if they haven't already been created.
   if (profiles_.empty()) {
-    if (!SetupClients())
+    if (!SetupClients()) {
       LOG(FATAL) << "SetupClients() failed.";
+      return false;
+    }
   }
 
   // Sync each of the profiles.
   for (int i = 0; i < num_clients_; ++i) {
-    if (!GetClient(i)->SetupSync())
+    if (!GetClient(i)->SetupSync()) {
       LOG(FATAL) << "SetupSync() failed.";
+      return false;
+    }
   }
 
   // Because clients may modify sync data as part of startup (for example local
@@ -608,7 +612,10 @@ bool SyncTest::SetupSync() {
   // have to find their own way of waiting for an initial state if they really
   // need such guarantees.
   if (TestUsesSelfNotifications()) {
-    AwaitQuiescence();
+    if (!AwaitQuiescence()) {
+      LOG(FATAL) << "AwaitQuiescence() failed.";
+      return false;
+    }
   }
 
   // SyncRefresher is used instead of invalidations to notify other profiles to
diff --git a/components/browser_sync/browser/profile_sync_service.cc b/components/browser_sync/browser/profile_sync_service.cc
index 67987b2..185438f 100644
--- a/components/browser_sync/browser/profile_sync_service.cc
+++ b/components/browser_sync/browser/profile_sync_service.cc
@@ -250,7 +250,7 @@ void ProfileSyncService::Initialize() {
   sync_client_->Initialize();
 
   startup_controller_.reset(new browser_sync::StartupController(
-      start_behavior_, oauth2_token_service_, &sync_prefs_, signin_.get(),
+      oauth2_token_service_, &sync_prefs_, signin_.get(),
       base::Bind(&ProfileSyncService::StartUpSlowBackendComponents,
                  startup_controller_weak_factory_.GetWeakPtr())));
   scoped_ptr<browser_sync::LocalSessionEventRouter> router(
@@ -815,6 +815,9 @@ bool ProfileSyncService::IsFirstSetupComplete() const {
 
 void ProfileSyncService::SetFirstSetupComplete() {
   sync_prefs_.SetFirstSetupComplete();
+  if (IsBackendInitialized()) {
+    ReconfigureDatatypeManager();
+  }
 }
 
 void ProfileSyncService::UpdateLastSyncedTime() {
@@ -932,20 +935,12 @@ void ProfileSyncService::PostBackendInitialization() {
     UpdateLastSyncedTime();
   }
 
-  if (startup_controller_->auto_start_enabled() && !IsFirstSetupInProgress()) {
-    // Backend is initialized but we're not in sync setup, so this must be an
-    // autostart - mark our sync setup as completed and we'll start syncing
-    // below.
+  // Auto-start means IsFirstSetupComplete gets set automatically.
+  if (start_behavior_ == browser_sync::AUTO_START && !IsFirstSetupComplete()) {
+    // This will trigger a configure if it completes setup.
     SetFirstSetupComplete();
-  }
-
-  // Check IsFirstSetupComplete() before NotifyObservers() to avoid spurious
-  // data type configuration because observer may flag setup as complete and
-  // trigger data type configuration.
-  if (IsFirstSetupComplete()) {
+  } else if (CanConfigureDataTypes()) {
     ConfigureDataTypeManager();
-  } else {
-    DCHECK(IsFirstSetupInProgress());
   }
 
   NotifyObservers();
@@ -1237,15 +1232,11 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
                                   syncer::STOP_SOURCE_LIMIT);
       }
       RequestStop(CLEAR_DATA);
-#if !defined(OS_CHROMEOS)
-      // On desktop Chrome, sign out the user after a dashboard clear.
-      // Skip sign out on ChromeOS/Android.
-      if (!startup_controller_->auto_start_enabled()) {
-        SigninManager* signin_manager =
-            static_cast<SigninManager*>(signin_->GetOriginal());
-        signin_manager->SignOut(signin_metrics::SERVER_FORCED_DISABLE,
-                                signin_metrics::SignoutDelete::IGNORE_METRIC);
-      }
+#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
+      // On desktop and iOS, sign out the user after a dashboard clear.
+      static_cast<SigninManager*>(signin_->GetOriginal())
+          ->SignOut(signin_metrics::SERVER_FORCED_DISABLE,
+                    signin_metrics::SignoutDelete::IGNORE_METRIC);
 #endif
       break;
     case syncer::STOP_SYNC_FOR_DISABLED_ACCOUNT:
@@ -1455,10 +1446,6 @@ std::string ProfileSyncService::GetBackendInitializationStateString() const {
   return startup_controller_->GetBackendInitializationStateString();
 }
 
-bool ProfileSyncService::auto_start_enabled() const {
-  return startup_controller_->auto_start_enabled();
-}
-
 bool ProfileSyncService::IsSetupInProgress() const {
   return startup_controller_->IsSetupInProgress();
 }
@@ -1480,6 +1467,10 @@ const AuthError& ProfileSyncService::GetAuthError() const {
   return last_auth_error_;
 }
 
+bool ProfileSyncService::CanConfigureDataTypes() const {
+  return IsFirstSetupComplete() && !IsSetupInProgress();
+}
+
 bool ProfileSyncService::IsFirstSetupInProgress() const {
   return !IsFirstSetupComplete() && startup_controller_->IsSetupInProgress();
 }
@@ -1738,7 +1729,7 @@ void ProfileSyncService::ConfigureDataTypeManager() {
   // start syncing data until the user is done configuring encryption options,
   // etc. ReconfigureDatatypeManager() will get called again once the UI calls
   // SetSetupInProgress(false).
-  if (startup_controller_->IsSetupInProgress())
+  if (!CanConfigureDataTypes())
     return;
 
   bool restart = false;
diff --git a/components/browser_sync/browser/profile_sync_service.h b/components/browser_sync/browser/profile_sync_service.h
index df981bc..bd48e87 100644
--- a/components/browser_sync/browser/profile_sync_service.h
+++ b/components/browser_sync/browser/profile_sync_service.h
@@ -165,14 +165,8 @@ class EncryptedData;
 //   up sync at least once on their account. SetSetupInProgress(true) should be
 //   called while the user is actively configuring their account, and then
 //   SetSetupInProgress(false) should be called when configuration is complete.
-//   When SetFirstSetupComplete() == false, but SetSetupInProgress(true) has
-//   been called, then the sync engine knows not to download any user data.
-//
-//   When initial sync is complete, the UI code should call
-//   SetFirstSetupComplete() followed by SetSetupInProgress(false) - this will
-//   tell the sync engine that setup is completed and it can begin downloading
-//   data from the sync server.
-//
+//   Once both these conditions have been met, CanConfigureDataTypes() will
+//   return true and datatype configuration can begin.
 class ProfileSyncService : public sync_driver::SyncService,
                            public sync_driver::SyncFrontend,
                            public sync_driver::SyncPrefObserver,
@@ -508,9 +502,6 @@ class ProfileSyncService : public sync_driver::SyncService,
 
   SigninManagerBase* signin() const;
 
-  // Used by tests.
-  bool auto_start_enabled() const;
-
   SyncErrorController* sync_error_controller() {
     return sync_error_controller_.get();
   }
@@ -782,6 +773,9 @@ class ProfileSyncService : public sync_driver::SyncService,
   // Restarts sync clearing directory in the process.
   void OnClearServerDataDone();
 
+  // True if setup has been completed at least once and is not in progress.
+  bool CanConfigureDataTypes() const;
+
   // This profile's SyncClient, which abstracts away non-Sync dependencies and
   // the Sync API component factory.
   scoped_ptr<sync_driver::SyncClient> sync_client_;
diff --git a/components/browser_sync/browser/profile_sync_service_startup_unittest.cc b/components/browser_sync/browser/profile_sync_service_startup_unittest.cc
index 206ce89..98f66d1 100644
--- a/components/browser_sync/browser/profile_sync_service_startup_unittest.cc
+++ b/components/browser_sync/browser/profile_sync_service_startup_unittest.cc
@@ -208,6 +208,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFirstTime) {
 
   // Simulate the UI telling sync it has finished setting up.
   sync_service_->SetSetupInProgress(false);
+  sync_service_->SetFirstSetupComplete();
   EXPECT_TRUE(sync_service_->IsSyncActive());
 }
 
@@ -411,26 +412,32 @@ TEST_F(ProfileSyncServiceStartupTest, SwitchManaged) {
   SetUpSyncBackendHost();
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
   EXPECT_CALL(*data_type_manager, Configure(_, _));
+  EXPECT_CALL(*data_type_manager, state())
+      .WillRepeatedly(Return(DataTypeManager::CONFIGURED));
   EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
   IssueTestTokens(account_id);
   sync_service_->Initialize();
+  EXPECT_TRUE(sync_service_->IsBackendInitialized());
+  EXPECT_TRUE(sync_service_->IsSyncActive());
 
   // The service should stop when switching to managed mode.
   Mock::VerifyAndClearExpectations(data_type_manager);
   EXPECT_CALL(*data_type_manager, state()).
       WillOnce(Return(DataTypeManager::CONFIGURED));
   EXPECT_CALL(*data_type_manager, Stop()).Times(1);
-  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
   pref_service()->SetBoolean(sync_driver::prefs::kSyncManaged, true);
+  EXPECT_FALSE(sync_service_->IsBackendInitialized());
+  // Note that PSS no longer references |data_type_manager| after stopping.
 
-  // When switching back to unmanaged, the state should change, but the service
-  // should not start up automatically (kSyncFirstSetupComplete will be
-  // false).
-  Mock::VerifyAndClearExpectations(data_type_manager);
+  // When switching back to unmanaged, the state should change and sync should
+  // start but not become active because IsFirstSetupComplete() will be false.
+  SetUpSyncBackendHost();
+  // A new DataTypeManager should not be created.
   EXPECT_CALL(*component_factory_, CreateDataTypeManager(_, _, _, _, _))
       .Times(0);
-  EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());
   pref_service()->ClearPref(sync_driver::prefs::kSyncManaged);
+  EXPECT_TRUE(sync_service_->IsBackendInitialized());
+  EXPECT_FALSE(sync_service_->IsSyncActive());
 }
 
 TEST_F(ProfileSyncServiceStartupTest, StartFailure) {
diff --git a/components/browser_sync/browser/profile_sync_service_unittest.cc b/components/browser_sync/browser/profile_sync_service_unittest.cc
index b348ffa..be52cbe 100644
--- a/components/browser_sync/browser/profile_sync_service_unittest.cc
+++ b/components/browser_sync/browser/profile_sync_service_unittest.cc
@@ -92,14 +92,15 @@ using testing::_;
 class TestSyncServiceObserver : public sync_driver::SyncServiceObserver {
  public:
   explicit TestSyncServiceObserver(ProfileSyncService* service)
-      : service_(service), first_setup_in_progress_(false) {}
+      : service_(service), setup_in_progress_(false) {}
   void OnStateChanged() override {
-    first_setup_in_progress_ = service_->IsFirstSetupInProgress();
+    setup_in_progress_ = service_->IsSetupInProgress();
   }
-  bool first_setup_in_progress() const { return first_setup_in_progress_; }
+  bool setup_in_progress() const { return setup_in_progress_; }
+
  private:
   ProfileSyncService* service_;
-  bool first_setup_in_progress_;
+  bool setup_in_progress_;
 };
 
 // A variant of the SyncBackendHostMock that won't automatically
@@ -265,8 +266,7 @@ class ProfileSyncServiceTest : public ::testing::Test {
 
   void InitializeForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
-    sync_driver::SyncPrefs sync_prefs(
-        service_->GetSyncClient()->GetPrefService());
+    sync_driver::SyncPrefs sync_prefs(prefs());
     sync_prefs.SetFirstSyncTime(base::Time::Now());
     sync_prefs.SetFirstSetupComplete();
     sync_prefs.SetKeepEverythingSynced(true);
@@ -423,9 +423,9 @@ TEST_F(ProfileSyncServiceTest, SetupInProgress) {
   service()->AddObserver(&observer);
 
   service()->SetSetupInProgress(true);
-  EXPECT_TRUE(observer.first_setup_in_progress());
+  EXPECT_TRUE(observer.setup_in_progress());
   service()->SetSetupInProgress(false);
-  EXPECT_FALSE(observer.first_setup_in_progress());
+  EXPECT_FALSE(observer.setup_in_progress());
 
   service()->RemoveObserver(&observer);
 }
@@ -477,23 +477,21 @@ TEST_F(ProfileSyncServiceTest, AbortedByShutdown) {
 TEST_F(ProfileSyncServiceTest, EarlyRequestStop) {
   CreateService(browser_sync::AUTO_START);
   IssueTestTokens();
+  ExpectDataTypeManagerCreation(1, GetDefaultConfigureCalledCallback());
+  ExpectSyncBackendHostCreation(1);
 
   service()->RequestStop(ProfileSyncService::KEEP_DATA);
-  EXPECT_TRUE(prefs()->GetBoolean(sync_driver::prefs::kSyncSuppressStart));
+  EXPECT_FALSE(service()->IsSyncRequested());
 
-  // Because of suppression, this should fail.
-  sync_driver::SyncPrefs sync_prefs(
-      service()->GetSyncClient()->GetPrefService());
-  sync_prefs.SetFirstSyncTime(base::Time::Now());
-  service()->Initialize();
+  // Because sync is not requested, this should fail.
+  InitializeForNthSync();
+  EXPECT_FALSE(service()->IsSyncRequested());
   EXPECT_FALSE(service()->IsSyncActive());
 
-  // Request start.  This should be enough to allow init to happen.
-  ExpectDataTypeManagerCreation(1, GetDefaultConfigureCalledCallback());
-  ExpectSyncBackendHostCreation(1);
+  // Request start. This should be enough to allow init to happen.
   service()->RequestStart();
+  EXPECT_TRUE(service()->IsSyncRequested());
   EXPECT_TRUE(service()->IsSyncActive());
-  EXPECT_FALSE(prefs()->GetBoolean(sync_driver::prefs::kSyncSuppressStart));
 }
 
 // Test RequestStop() after we've initialized the backend.
diff --git a/components/sync_driver/startup_controller.cc b/components/sync_driver/startup_controller.cc
index 4db2bb2..a8ae941 100644
--- a/components/sync_driver/startup_controller.cc
+++ b/components/sync_driver/startup_controller.cc
@@ -39,21 +39,18 @@ enum DeferredInitTrigger {
 }  // namespace
 
 StartupController::StartupController(
-    ProfileSyncServiceStartBehavior start_behavior,
     const ProfileOAuth2TokenService* token_service,
     const sync_driver::SyncPrefs* sync_prefs,
     const SigninManagerWrapper* signin,
     base::Closure start_backend)
     : received_start_request_(false),
       setup_in_progress_(false),
-      auto_start_enabled_(start_behavior == AUTO_START),
       sync_prefs_(sync_prefs),
       token_service_(token_service),
       signin_(signin),
       start_backend_(start_backend),
       fallback_timeout_(
           base::TimeDelta::FromSeconds(kDeferredInitFallbackSeconds)),
-      first_start_(true),
       weak_factory_(this) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSyncDeferredStartupTimeoutSeconds)) {
@@ -108,7 +105,6 @@ bool StartupController::StartUp(StartUpDeferredOption deferred_option) {
   if (start_backend_time_.is_null()) {
     start_backend_time_ = base::Time::Now();
     start_backend_.Run();
-    first_start_ = false;
   }
 
   return true;
@@ -143,34 +139,18 @@ bool StartupController::TryStart() {
   // PSS will show error to user asking to reauthenticate.
   UMA_HISTOGRAM_BOOLEAN("Sync.RefreshTokenAvailable", true);
 
-  // If sync setup has completed we always start the backend. If the user is in
-  // the process of setting up now, we should start the backend to download
-  // account control state / encryption information). If autostart is enabled,
-  // but we haven't completed sync setup, we try to start sync anyway, since
-  // it's possible we crashed/shutdown after logging in but before the backend
-  // finished initializing the last time.
+  // For performance reasons, defer the heavy lifting for sync init unless:
   //
-  // However, the only time we actually need to start sync _immediately_ is if
-  // we haven't completed sync setup and the user is in the process of setting
-  // up - either they just signed in (for the first time) on an auto-start
-  // platform or they explicitly kicked off sync setup, and e.g we need to
-  // fetch account details like encryption state to populate UI. Otherwise,
-  // for performance reasons and maximizing parallelism at chrome startup, we
-  // defer the heavy lifting for sync init until things have calmed down.
-  if (sync_prefs_->IsFirstSetupComplete()) {
-    // For first time, defer start if data type hasn't requested sync to avoid
-    // stressing browser start.
-    if (!received_start_request_ && first_start_)
-      return StartUp(STARTUP_BACKEND_DEFERRED);
-    else
-      return StartUp(STARTUP_IMMEDIATE);
-  } else if (setup_in_progress_ || auto_start_enabled_) {
-    // We haven't completed sync setup. Start immediately if the user explicitly
-    // kicked this off or we're supposed to automatically start syncing.
+  // - a datatype has requested an immediate start of sync, or
+  // - sync needs to start up the backend immediately to provide control state
+  //   and encryption information to the UI, or
+  // - this is the first time sync is ever starting up.
+  if (received_start_request_ || setup_in_progress_ ||
+      !sync_prefs_->IsFirstSetupComplete()) {
     return StartUp(STARTUP_IMMEDIATE);
+  } else {
+    return StartUp(STARTUP_BACKEND_DEFERRED);
   }
-
-  return false;
 }
 
 void StartupController::RecordTimeDeferred() {
diff --git a/components/sync_driver/startup_controller.h b/components/sync_driver/startup_controller.h
index c64275048..6d6993a 100644
--- a/components/sync_driver/startup_controller.h
+++ b/components/sync_driver/startup_controller.h
@@ -35,8 +35,7 @@ enum ProfileSyncServiceStartBehavior {
 // to as "the backend").
 class StartupController {
  public:
-  StartupController(ProfileSyncServiceStartBehavior start_behavior,
-                    const ProfileOAuth2TokenService* token_service,
+  StartupController(const ProfileOAuth2TokenService* token_service,
                     const sync_driver::SyncPrefs* sync_prefs,
                     const SigninManagerWrapper* signin,
                     base::Closure start_backend);
@@ -64,7 +63,6 @@ class StartupController {
 
   void set_setup_in_progress(bool in_progress);
   bool IsSetupInProgress() const { return setup_in_progress_; }
-  bool auto_start_enabled() const { return auto_start_enabled_; }
   base::Time start_backend_time() const { return start_backend_time_; }
   std::string GetBackendInitializationStateString() const;
 
@@ -98,12 +96,6 @@ class StartupController {
   // due to explicit requests to do so via set_setup_in_progress.
   bool setup_in_progress_;
 
-  // If true, we want to automatically start sync signin whenever we have
-  // credentials (user doesn't need to go through the startup flow). This is
-  // typically enabled on platforms (like ChromeOS) that have their own
-  // distinct signin flow.
-  const bool auto_start_enabled_;
-
   const sync_driver::SyncPrefs* sync_prefs_;
 
   const ProfileOAuth2TokenService* token_service_;
@@ -122,9 +114,6 @@ class StartupController {
   // Used to compute preferred_types from SyncPrefs as-needed.
   syncer::ModelTypeSet registered_types_;
 
-  // True before calling |start_backend_| for the first time. False after that.
-  bool first_start_;
-
   base::WeakPtrFactory<StartupController> weak_factory_;
 };
 
diff --git a/components/sync_driver/startup_controller_unittest.cc b/components/sync_driver/startup_controller_unittest.cc
index e11678d..6fada80 100644
--- a/components/sync_driver/startup_controller_unittest.cc
+++ b/components/sync_driver/startup_controller_unittest.cc
@@ -55,7 +55,7 @@ class StartupControllerTest : public testing::Test {
     token_service_.reset(new FakeProfileOAuth2TokenService());
     signin_.reset(new FakeSigninManagerWrapper());
 
-    SetUpController(AUTO_START);
+    SetUpController();
   }
 
   void TearDown() override {
@@ -67,13 +67,12 @@ class StartupControllerTest : public testing::Test {
     started_ = false;
   }
 
-  void SetUpController(ProfileSyncServiceStartBehavior start_behavior) {
+  void SetUpController() {
     started_ = false;
     base::Closure fake_start_backend = base::Bind(
         &StartupControllerTest::FakeStartBackend, base::Unretained(this));
-    controller_.reset(new StartupController(start_behavior, token_service(),
-                                            sync_prefs_.get(), signin_.get(),
-                                            fake_start_backend));
+    controller_.reset(new StartupController(token_service(), sync_prefs_.get(),
+                                            signin_.get(), fake_start_backend));
     controller_->Reset(syncer::UserTypes());
     controller_->OverrideFallbackTimeoutForTest(
         base::TimeDelta::FromSeconds(0));
@@ -81,6 +80,28 @@ class StartupControllerTest : public testing::Test {
 
   void FakeStartBackend() {
     started_ = true;
+    sync_prefs()->SetFirstSetupComplete();
+  }
+
+  void ExpectStarted() {
+    EXPECT_TRUE(started());
+    EXPECT_EQ(kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectStartDeferred() {
+    const bool deferred_start =
+        !base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kSyncDisableDeferredStartup);
+    EXPECT_EQ(!deferred_start, started());
+    EXPECT_EQ(deferred_start ? kStateStringDeferred : kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectNotStarted() {
+    EXPECT_FALSE(started());
+    EXPECT_EQ(kStateStringNotStarted,
+              controller()->GetBackendInitializationStateString());
   }
 
   bool started() const { return started_; }
@@ -105,22 +126,19 @@ class StartupControllerTest : public testing::Test {
 // Test that sync doesn't start until all conditions are met.
 TEST_F(StartupControllerTest, Basic) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   sync_prefs()->SetFirstSetupComplete();
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   signin()->set_account_id(kTestUser);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
   controller()->TryStart();
-  EXPECT_EQ(!deferred_start, started());
-  std::string state(controller()->GetBackendInitializationStateString());
-  EXPECT_TRUE(deferred_start ? state == kStateStringDeferred :
-                               state == kStateStringStarted);
+  ExpectStartDeferred();
 }
 
 // Test that sync doesn't start when not requested even if all other
@@ -131,9 +149,7 @@ TEST_F(StartupControllerTest, NotRequested) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't when managed even if all other conditons are met.
@@ -143,9 +159,7 @@ TEST_F(StartupControllerTest, Managed) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't start until all conditions are met and a
@@ -155,13 +169,10 @@ TEST_F(StartupControllerTest, DataTypeTriggered) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringDeferred,
-            controller()->GetBackendInitializationStateString());
+  ExpectStartDeferred();
+
   controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
-  EXPECT_EQ(kStateStringStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectStarted();
 
   // The fallback timer shouldn't result in another invocation of the closure
   // we passed to the StartupController.
@@ -177,9 +188,10 @@ TEST_F(StartupControllerTest, FallbackTimer) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectStartDeferred();
+
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Test that we start immediately if sessions is disabled.
@@ -194,60 +206,44 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   sync_prefs()->SetPreferredDataTypes(syncer::UserTypes(), types);
   controller()->Reset(syncer::UserTypes());
   sync_prefs()->SetFirstSetupComplete();
+
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Sanity check that the fallback timer doesn't fire before startup
 // conditions are met.
 TEST_F(StartupControllerTest, FallbackTimerWaits) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(started());
-}
-
-// Test that sync starts without the user having to explicitly ask for
-// setup when AUTO_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithAutoStart) {
-  signin()->set_account_id(kTestUser);
-  token_service()->UpdateCredentials(kTestUser, kTestToken);
-  controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectNotStarted();
 }
 
-// Test that sync starts only after user explicitly asks for setup when
-// MANUAL_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithManualStart) {
+// Test that start isn't deferred when setup is in progress.
+TEST_F(StartupControllerTest, NoDeferralWithSetupInProgress) {
+  sync_prefs()->SetFirstSetupComplete();
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  SetUpController(MANUAL_START);
-  controller()->TryStart();
-  EXPECT_FALSE(started());
   controller()->set_setup_in_progress(true);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
-TEST_F(StartupControllerTest, Reset) {
-  sync_prefs()->SetFirstSetupComplete();
+// Test that start isn't deferred on the first start but is on restarts.
+TEST_F(StartupControllerTest, DeferralOnRestart) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
-  EXPECT_EQ(!deferred_start, started());
-  controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
+  ExpectStarted();
+
   clear_started();
   controller()->Reset(syncer::UserTypes());
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   controller()->TryStart();
-  // Restart is not deferred.
-  EXPECT_TRUE(started());
+  ExpectStartDeferred();
 }
 
 // Test that setup-in-progress tracking is persistent across a Reset.