[Sync] Don't start up sync when FirstSetupCompleted is false and no setup in progress

Instead of starting up the backend, we now don't even start it up if the first
setup completed flag is false, unless a setup is in progress. On auto start
platforms, first setup completed is manually set to true as part of init.

BUG=624915

Review-Url: https://codereview.chromium.org/2159453002
Cr-Commit-Position: refs/heads/master@{#405950}

components/browser_sync/browser/profile_sync_service.cc

@@ -381,7 +381,15 @@ void ProfileSyncService::Initialize() {
       base::Bind(&ProfileSyncService::OnMemoryPressure,
                  sync_enabled_weak_factory_.GetWeakPtr())));
   startup_controller_->Reset(GetRegisteredDataTypes());
-  startup_controller_->TryStart();
+
+  // Auto-start means means the first time the profile starts up, sync should
+  // start up immediately.
+  if (start_behavior_ == AUTO_START && IsSyncRequested() &&
+      !IsFirstSetupComplete()) {
+    startup_controller_->TryStartImmediately();
+  } else {
+    startup_controller_->TryStart();
+  }
 }
 
 void ProfileSyncService::StartSyncingWithServer() {
@@ -2335,7 +2343,7 @@ void ProfileSyncService::RequestStart() {
     sync_prefs_.SetSyncRequested(true);
     NotifyObservers();
   }
-  startup_controller_->TryStart();
+  startup_controller_->TryStartImmediately();
 }
 
 void ProfileSyncService::ReconfigureDatatypeManager() {

components/browser_sync/browser/profile_sync_service_startup_unittest.cc

@@ -429,14 +429,14 @@ TEST_F(ProfileSyncServiceStartupTest, SwitchManaged) {
   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 and sync should
-  // start but not become active because IsFirstSetupComplete() will be false.
-  SetUpSyncBackendHost();
+  // When switching back to unmanaged, the state should change but sync should
+  // not start automatically because IsFirstSetupComplete() will be false.
   // A new DataTypeManager should not be created.
+  Mock::VerifyAndClearExpectations(data_type_manager);
   EXPECT_CALL(*component_factory_, CreateDataTypeManager(_, _, _, _, _))
       .Times(0);
   pref_service()->ClearPref(sync_driver::prefs::kSyncManaged);
-  EXPECT_TRUE(sync_service_->IsBackendInitialized());
+  EXPECT_FALSE(sync_service_->IsBackendInitialized());
   EXPECT_FALSE(sync_service_->IsSyncActive());
 }
 

components/browser_sync/browser/profile_sync_service_unittest.cc

@@ -416,13 +416,12 @@ TEST_F(ProfileSyncServiceTest, SuccessfulInitialization) {
 }
 
 // Verify that an initialization where first setup is not complete does not
-// purge preferences and/or the directory.
+// start up the backend.
 TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   prefs()->SetManagedPref(sync_driver::prefs::kSyncManaged,
                           new base::FundamentalValue(false));
   IssueTestTokens();
   CreateService(ProfileSyncService::MANUAL_START);
-  ExpectSyncBackendHostCreation(1);
   sync_driver::SyncPrefs sync_prefs(prefs());
   base::Time now = base::Time::Now();
   sync_prefs.SetLastSyncedTime(now);

components/sync_driver/startup_controller.cc

@@ -39,7 +39,8 @@ enum DeferredInitTrigger {
 StartupController::StartupController(const sync_driver::SyncPrefs* sync_prefs,
                                      base::Callback<bool()> can_start,
                                      base::Closure start_backend)
-    : received_start_request_(false),
+    : bypass_setup_complete_(false),
+      received_start_request_(false),
       setup_in_progress_(false),
       sync_prefs_(sync_prefs),
       can_start_(can_start),
@@ -66,6 +67,7 @@ StartupController::~StartupController() {}
 
 void StartupController::Reset(const syncer::ModelTypeSet registered_types) {
   received_start_request_ = false;
+  bypass_setup_complete_ = false;
   start_up_time_ = base::Time();
   start_backend_time_ = base::Time();
   // Don't let previous timers affect us post-reset.
@@ -121,16 +123,25 @@ bool StartupController::TryStart() {
   //
   // - 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()) {
+  //   and encryption information to the UI.
+  // Do not start up the sync backend if setup has not completed and isn't
+  // in progress, unless told to otherwise.
+  if (setup_in_progress_) {
     return StartUp(STARTUP_IMMEDIATE);
+  } else if (sync_prefs_->IsFirstSetupComplete() || bypass_setup_complete_) {
+    return StartUp(received_start_request_ ? STARTUP_IMMEDIATE
+                                           : STARTUP_BACKEND_DEFERRED);
   } else {
-    return StartUp(STARTUP_BACKEND_DEFERRED);
+    return false;
   }
 }
 
+bool StartupController::TryStartImmediately() {
+  received_start_request_ = true;
+  bypass_setup_complete_ = true;
+  return TryStart();
+}
+
 void StartupController::RecordTimeDeferred() {
   DCHECK(!start_up_time_.is_null());
   base::TimeDelta time_deferred = base::Time::Now() - start_up_time_;

components/sync_driver/startup_controller.h

@@ -31,6 +31,10 @@ class StartupController {
   // the backend was started.
   bool TryStart();
 
+  // Same as TryStart() above, but bypasses deferred startup and the first setup
+  // complete check.
+  bool TryStartImmediately();
+
   // Called when a datatype (SyncableService) has a need for sync to start
   // ASAP, presumably because a local change event has occurred but we're
   // still in deferred start mode, meaning the SyncableService hasn't been
@@ -67,6 +71,10 @@ class StartupController {
   // Records time spent in deferred state with UMA histograms.
   void RecordTimeDeferred();
 
+  // If true, will bypass the FirstSetupComplete check when triggering sync
+  // startup.
+  bool bypass_setup_complete_;
+
   // True if we should start sync ASAP because either a SyncableService has
   // requested it, or we're done waiting for a sign and decided to go ahead.
   bool received_start_request_;

components/sync_driver/startup_controller_unittest.cc

@@ -86,14 +86,14 @@ class StartupControllerTest : public testing::Test {
   std::unique_ptr<StartupController> controller_;
 };
 
-// Test that sync doesn't start until all conditions are met.
-TEST_F(StartupControllerTest, Basic) {
+// Test that sync doesn't start if setup is not in progress or complete.
+TEST_F(StartupControllerTest, NoSetupComplete) {
   controller()->TryStart();
   ExpectNotStarted();
 
   SetCanStart(true);
   controller()->TryStart();
-  ExpectStarted();
+  ExpectNotStarted();
 }
 
 // Test that sync defers if first setup is complete.
@@ -189,17 +189,11 @@ TEST_F(StartupControllerTest, SetupInProgressTriggerInterruptsDeferral) {
   ExpectStarted();
 }
 
-// Test that start isn't deferred on the first start but is on restarts.
-TEST_F(StartupControllerTest, DeferralOnRestart) {
+// Test that immediate startup can be forced.
+TEST_F(StartupControllerTest, ForceImmediateStartup) {
   SetCanStart(true);
-  controller()->TryStart();
+  controller()->TryStartImmediately();
   ExpectStarted();
-
-  clear_started();
-  controller()->Reset(syncer::UserTypes());
-  ExpectNotStarted();
-  controller()->TryStart();
-  ExpectStartDeferred();
 }
 
 // Test that setup-in-progress tracking is persistent across a Reset.