Sync: Reconfigure whenever a SyncSetupInProgressHandle is released

Before: Reconfigure Sync only when the *last* handle is released.
After: Reconfigure Sync whenever a handle is released, whether there
are any others or not.

On desktop, this fixes the case where a chrome://settings/syncSetup
tab is left open and forgotten somewhere, which previously prevented
Sync from ever applying new settings.

On Android/iOS, only one Sync setup UI can be visible at a time, so
this doesn't change any behavior. However, we now need to be careful
not to needlessly re-create the handle.

Bug: 884159
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Ib61d063ac8b6e76d05a5b72d17de7b1e031a51dd
Reviewed-on: https://chromium-review.googlesource.com/c/1280208
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601899}

chrome/browser/supervised_user/child_accounts/child_account_service.cc

@@ -239,8 +239,10 @@ bool ChildAccountService::SetActive(bool active) {
   // The logic to do this lives in the SupervisedUserSyncDataTypeController.
   browser_sync::ProfileSyncService* sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (sync_service->IsFirstSetupComplete())
-    sync_service->ReconfigureDatatypeManager();
+  if (sync_service->IsFirstSetupComplete()) {
+    sync_service->ReconfigureDatatypeManager(
+        /*bypass_setup_in_progress_check=*/false);
+  }
 
   return true;
 }

chrome/browser/supervised_user/supervised_user_service.cc

@@ -723,7 +723,7 @@ void SupervisedUserService::OnForceSessionSyncChanged() {
   includes_sync_sessions_type_ =
       profile_->GetPrefs()->GetBoolean(prefs::kForceSessionSync);
   ProfileSyncServiceFactory::GetForProfile(profile_)
-      ->ReconfigureDatatypeManager();
+      ->ReconfigureDatatypeManager(/*bypass_setup_in_progress_check=*/false);
 }
 
 void SupervisedUserService::Shutdown() {

chrome/browser/sync/profile_sync_service_android.cc

@@ -185,7 +185,9 @@ void ProfileSyncServiceAndroid::SetSetupInProgress(
     jboolean in_progress) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (in_progress) {
+    if (!sync_blocker_) {
       sync_blocker_ = sync_service_->GetSetupInProgressHandle();
+    }
   } else {
     sync_blocker_.reset();
   }

components/browser_sync/profile_sync_service.cc

@@ -317,7 +317,7 @@ void ProfileSyncService::StartSyncingWithServer() {
   if (base::FeatureList::IsEnabled(
           switches::kSyncClearDataOnPassphraseEncryption) &&
       sync_prefs_.GetPassphraseEncryptionTransitionInProgress()) {
-    DCHECK(CanConfigureDataTypes());
+    DCHECK(CanConfigureDataTypes(/*bypass_setup_in_progress_check=*/false));
     // We are restarting catchup configuration after browser restart.
     UMA_HISTOGRAM_ENUMERATION("Sync.ClearServerDataEvents",
                               syncer::CLEAR_SERVER_DATA_RETRIED,
@@ -795,7 +795,7 @@ syncer::SyncService::TransportState ProfileSyncService::GetTransportState()
   // configure the data types. Also if a later (non-initial) setup happens to be
   // in progress, we won't configure them right now.
   if (data_type_manager_->state() == DataTypeManager::STOPPED) {
-    DCHECK(!CanConfigureDataTypes());
+    DCHECK(!CanConfigureDataTypes(/*bypass_setup_in_progress_check=*/false));
     return TransportState::PENDING_DESIRED_CONFIGURATION;
   }
 
@@ -807,7 +807,8 @@ syncer::SyncService::TransportState ProfileSyncService::GetTransportState()
   // Note that if a setup is started after the data types have been configured,
   // then they'll stay configured even though CanConfigureDataTypes will be
   // false.
-  DCHECK(CanConfigureDataTypes() || IsSetupInProgress());
+  DCHECK(CanConfigureDataTypes(/*bypass_setup_in_progress_check=*/false) ||
+         IsSetupInProgress());
 
   if (data_type_manager_->state() != DataTypeManager::CONFIGURED) {
     return TransportState::CONFIGURING;
@@ -825,7 +826,7 @@ void ProfileSyncService::SetFirstSetupComplete() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   sync_prefs_.SetFirstSetupComplete();
   if (engine_initialized_) {
-    ReconfigureDatatypeManager();
+    ReconfigureDatatypeManager(/*bypass_setup_in_progress_check=*/false);
   }
 }
 
@@ -994,7 +995,7 @@ void ProfileSyncService::OnEngineInitialized(
   if (start_behavior_ == AUTO_START && !IsFirstSetupComplete()) {
     // This will trigger a configure if it completes setup.
     SetFirstSetupComplete();
-  } else if (CanConfigureDataTypes()) {
+  } else if (CanConfigureDataTypes(/*bypass_setup_in_progress_check=*/false)) {
     // Datatype downloads on restart are generally due to newly supported
     // datatypes (although it's also possible we're picking up where a failed
     // previous configuration left off).
@@ -1295,14 +1296,15 @@ const GoogleServiceAuthError& ProfileSyncService::GetAuthError() const {
   return auth_manager_->GetLastAuthError();
 }
 
-bool ProfileSyncService::CanConfigureDataTypes() const {
+bool ProfileSyncService::CanConfigureDataTypes(
+    bool bypass_setup_in_progress_check) const {
   // TODO(crbug.com/856179): Arguably, IsSetupInProgress() shouldn't prevent
   // configuring data types in transport mode, but at least for now, it's
   // easier to keep it like this. Changing this will likely require changes to
   // the setup UI flow.
   return data_type_manager_ &&
          (IsFirstSetupComplete() || IsStandaloneTransportEnabled()) &&
-         !IsSetupInProgress();
+         (bypass_setup_in_progress_check || !IsSetupInProgress());
 }
 
 std::unique_ptr<syncer::SyncSetupInProgressHandle>
@@ -1376,7 +1378,7 @@ void ProfileSyncService::OnUserChoseDatatypes(
                                     user_events_separate_pref_group_);
 
   // Now reconfigure the DTM.
-  ReconfigureDatatypeManager();
+  ReconfigureDatatypeManager(/*bypass_setup_in_progress_check=*/false);
 }
 
 syncer::ModelTypeSet ProfileSyncService::GetActiveDataTypes() const {
@@ -1482,9 +1484,6 @@ void ProfileSyncService::SetSyncAllowedByPlatform(bool allowed) {
 
 void ProfileSyncService::ConfigureDataTypeManager(
     syncer::ConfigureReason reason) {
-  DCHECK(CanConfigureDataTypes() ||
-         reason == syncer::CONFIGURE_REASON_MIGRATION);
-
   syncer::ConfigureContext configure_context;
   configure_context.authenticated_account_id =
       GetAuthenticatedAccountInfo().account_id;
@@ -2020,13 +2019,14 @@ void ProfileSyncService::RequestStart() {
   }
   // If Sync-the-transport was already running, just reconfigure.
   if (IsStandaloneTransportEnabled() && engine_initialized_) {
-    ReconfigureDatatypeManager();
+    ReconfigureDatatypeManager(/*bypass_setup_in_progress_check=*/false);
   } else {
     startup_controller_->TryStart(/*force_immediate=*/true);
   }
 }
 
-void ProfileSyncService::ReconfigureDatatypeManager() {
+void ProfileSyncService::ReconfigureDatatypeManager(
+    bool bypass_setup_in_progress_check) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // If we haven't initialized yet, don't configure the DTM as it could cause
   // association to start before a Directory has even been created.
@@ -2037,7 +2037,7 @@ void ProfileSyncService::ReconfigureDatatypeManager() {
     // start syncing data until the user is done configuring encryption options,
     // etc. ReconfigureDatatypeManager() will get called again once the last
     // SyncSetupInProgressHandle is released.
-    if (CanConfigureDataTypes()) {
+    if (CanConfigureDataTypes(bypass_setup_in_progress_check)) {
       ConfigureDataTypeManager(syncer::CONFIGURE_REASON_RECONFIGURATION);
     } else {
       DVLOG(0) << "ConfigureDataTypeManager not invoked because datatypes "
@@ -2226,19 +2226,21 @@ base::Location ProfileSyncService::unrecoverable_error_location() const {
 void ProfileSyncService::OnSetupInProgressHandleDestroyed() {
   DCHECK_GT(outstanding_setup_in_progress_handles_, 0);
 
-  // Don't re-start Sync until all outstanding handles are destroyed.
-  if (--outstanding_setup_in_progress_handles_ != 0)
-    return;
+  --outstanding_setup_in_progress_handles_;
 
-  if (engine_initialized_)
-    ReconfigureDatatypeManager();
+  if (engine_initialized_) {
+    // The user closed a setup UI, and will expect their changes to actually
+    // take effect now. So we reconfigure here even if another setup UI happens
+    // to be open right now.
+    ReconfigureDatatypeManager(/*bypass_setup_in_progress_check=*/true);
+  }
 
   NotifyObservers();
 }
 
 void ProfileSyncService::ReconfigureDueToPassphrase(
     syncer::ConfigureReason reason) {
-  if (CanConfigureDataTypes()) {
+  if (CanConfigureDataTypes(/*bypass_setup_in_progress_check=*/false)) {
     DCHECK(data_type_manager_->IsNigoriEnabled());
     ConfigureDataTypeManager(reason);
   }

components/browser_sync/profile_sync_service.h

@@ -357,11 +357,10 @@ class ProfileSyncService : public syncer::SyncService,
 
   // Reconfigures the data type manager with the latest enabled types.
   // Note: Does not initialize the engine if it is not already initialized.
-  // This function needs to be called only after sync has been initialized
-  // (i.e., only for reconfigurations). The reason we don't initialize the
-  // engine is because if we had encountered an unrecoverable error we don't
-  // want to startup once more.
-  void ReconfigureDatatypeManager();
+  // If a Sync setup is currently in progress (i.e. a settings UI is open), then
+  // the reconfiguration will only happen if |bypass_setup_in_progress_check| is
+  // set to true.
+  void ReconfigureDatatypeManager(bool bypass_setup_in_progress_check);
 
   syncer::PassphraseRequiredReason passphrase_required_reason_for_test() const {
     return crypto_.passphrase_required_reason();
@@ -586,7 +585,7 @@ class ProfileSyncService : public syncer::SyncService,
   void OnClearServerDataDone();
 
   // True if setup has been completed at least once and is not in progress.
-  bool CanConfigureDataTypes() const;
+  bool CanConfigureDataTypes(bool bypass_setup_in_progress_check) const;
 
   // Called when a SetupInProgressHandle issued by this instance is destroyed.
   virtual void OnSetupInProgressHandleDestroyed();

components/browser_sync/profile_sync_service_unittest.cc

@@ -1514,7 +1514,8 @@ TEST_F(ProfileSyncServiceTest, ConfigureDataTypeManagerReason) {
   service()->OnConfigureDone(configure_result);
 
   // Reconfiguration.
-  service()->ReconfigureDatatypeManager();
+  service()->ReconfigureDatatypeManager(
+      /*bypass_setup_in_progress_check=*/false);
   EXPECT_EQ(syncer::CONFIGURE_REASON_RECONFIGURATION, configure_reason);
   service()->OnConfigureDone(configure_result);
   ShutdownAndDeleteService();
@@ -1532,7 +1533,8 @@ TEST_F(ProfileSyncServiceTest, ConfigureDataTypeManagerReason) {
   service()->OnConfigureDone(configure_result);
 
   // Reconfiguration.
-  service()->ReconfigureDatatypeManager();
+  service()->ReconfigureDatatypeManager(
+      /*bypass_setup_in_progress_check=*/false);
   EXPECT_EQ(syncer::CONFIGURE_REASON_RECONFIGURATION, configure_reason);
   service()->OnConfigureDone(configure_result);
   ShutdownAndDeleteService();

ios/chrome/browser/sync/sync_setup_service.cc

@@ -64,6 +64,7 @@ bool SyncSetupService::IsDataTypePreferred(syncer::ModelType datatype) const {
 
 void SyncSetupService::SetDataTypeEnabled(syncer::ModelType datatype,
                                           bool enabled) {
+  if (!sync_blocker_)
     sync_blocker_ = sync_service_->GetSetupInProgressHandle();
   syncer::ModelTypeSet types = GetPreferredDataTypes();
   if (enabled)
@@ -106,6 +107,7 @@ bool SyncSetupService::IsSyncingAllDataTypes() const {
 }
 
 void SyncSetupService::SetSyncingAllDataTypes(bool sync_all) {
+  if (!sync_blocker_)
     sync_blocker_ = sync_service_->GetSetupInProgressHandle();
   if (sync_all && !IsSyncEnabled())
     SetSyncEnabled(true);
@@ -178,6 +180,7 @@ void SyncSetupService::PrepareForFirstSyncSetup() {
   // |PrepareForFirstSyncSetup| should always be called while the user is signed
   // out. At that time, sync setup is not completed.
   DCHECK(!sync_service_->IsFirstSetupComplete());
+  if (!sync_blocker_)
     sync_blocker_ = sync_service_->GetSetupInProgressHandle();
 }
 
@@ -199,6 +202,7 @@ bool SyncSetupService::HasUncommittedChanges() {
 
 void SyncSetupService::SetSyncEnabledWithoutChangingDatatypes(
     bool sync_enabled) {
+  if (!sync_blocker_)
     sync_blocker_ = sync_service_->GetSetupInProgressHandle();
   if (sync_enabled) {
     sync_service_->RequestStart();