Remove StopSyncInPausedState feature toggle

The feature has been launched to 100% Stable for over a month [1], so
this CL removes the feature toggle. Besides this, some instances of
base::RepeatingClosure() in profile_sync_service_unittest.cc are
replaced with base::DoNothing().

[1]https://groups.google.com/a/google.com/g/cr-rel-notify/c/_DOaHr_-x10/m/CA_P823xAQAJ
https://groups.google.com/a/google.com/g/cr-rel-notify/c/mOOb_eXMgy4/m/aRpz0POPAAAJ
https://groups.google.com/a/google.com/g/cr-rel-notify/c/bJ323M0Nrr4/m/jVRaVJWIBAAJ
https://groups.google.com/a/google.com/g/cr-rel-notify/c/ad3g3moxG4k/m/n-KJcTuTAAAJ

Bug: 1140447
Change-Id: Ifc72d6a35402d72bb5935a3dbc2d838fb5855bcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2489894Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#834639}

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -25,7 +25,6 @@
 #include "chrome/common/channel_info.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
-#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_internals_util.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/engine_impl/net/url_translator.h"
@@ -259,10 +258,8 @@ void ProfileSyncServiceHarness::EnterSyncPausedStateForPrimaryAccount() {
 void ProfileSyncServiceHarness::ExitSyncPausedStateForPrimaryAccount() {
   signin::SetRefreshTokenForPrimaryAccount(
       IdentityManagerFactory::GetForProfile(profile_));
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // The engine was off in the sync-paused state, so wait for it to start.
-    AwaitSyncSetupCompletion();
-  }
+  // The engine was off in the sync-paused state, so wait for it to start.
+  AwaitSyncSetupCompletion();
 }
 
 bool ProfileSyncServiceHarness::SetupSync() {

chrome/browser/sync/test/integration/sync_auth_test.cc

@@ -19,7 +19,6 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
 #include "components/sync/driver/profile_sync_service.h"
-#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_token_status.h"
 #include "content/public/test/browser_test.h"
 #include "google_apis/gaia/google_service_auth_error.h"
@@ -331,17 +330,10 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, SyncPausedState) {
   GetClient(0)->EnterSyncPausedStateForPrimaryAccount();
   ASSERT_TRUE(GetSyncService(0)->GetAuthError().IsPersistentError());
 
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // Sync should have shut itself down.
-    EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-              syncer::SyncService::TransportState::PAUSED);
-    EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
-  } else {
-    ASSERT_TRUE(AttemptToTriggerAuthError());
-
-    // Pausing sync may issue a reconfiguration, so wait until it finishes.
-    SyncTransportActiveChecker(GetSyncService(0)).Wait();
-  }
+  // Sync should have shut itself down.
+  EXPECT_EQ(GetSyncService(0)->GetTransportState(),
+            syncer::SyncService::TransportState::PAUSED);
+  EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
 
   // The active data types should now be empty.
   EXPECT_TRUE(GetSyncService(0)->GetActiveDataTypes().Empty());
@@ -353,13 +345,8 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, SyncPausedState) {
   NoAuthErrorChecker(GetSyncService(0)).Wait();
   ASSERT_FALSE(GetSyncService(0)->GetAuthError().IsPersistentError());
 
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // Once the auth error is gone, wait for Sync to start up again.
-    GetClient(0)->AwaitSyncSetupCompletion();
-  } else {
-    // Resuming sync could issue a reconfiguration, so wait until it finishes.
-    SyncTransportActiveChecker(GetSyncService(0)).Wait();
-  }
+  // Once the auth error is gone, wait for Sync to start up again.
+  GetClient(0)->AwaitSyncSetupCompletion();
 
   // Now the active data types should be back.
   EXPECT_TRUE(GetSyncService(0)->IsSyncFeatureActive());
@@ -399,17 +386,10 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, ShouldTrackDeletionsInSyncPausedState) {
   GetClient(0)->EnterSyncPausedStateForPrimaryAccount();
   ASSERT_TRUE(GetSyncService(0)->GetAuthError().IsPersistentError());
 
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // Sync should have shut itself down.
-    EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-              syncer::SyncService::TransportState::PAUSED);
-    EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
-  } else {
-    ASSERT_TRUE(AttemptToTriggerAuthError());
-
-    // Pausing sync may issue a reconfiguration, so wait until it finishes.
-    SyncTransportActiveChecker(GetSyncService(0)).Wait();
-  }
+  // Sync should have shut itself down.
+  EXPECT_EQ(GetSyncService(0)->GetTransportState(),
+            syncer::SyncService::TransportState::PAUSED);
+  EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
 
   ASSERT_FALSE(GetSyncService(0)->GetActiveDataTypes().Has(syncer::BOOKMARKS));
   ASSERT_FALSE(
@@ -429,13 +409,8 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, ShouldTrackDeletionsInSyncPausedState) {
   // access token again, so wait for that to happen.
   NoAuthErrorChecker(GetSyncService(0)).Wait();
   ASSERT_FALSE(GetSyncService(0)->GetAuthError().IsPersistentError());
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // Once the auth error is gone, wait for Sync to start up again.
-    GetClient(0)->AwaitSyncSetupCompletion();
-  } else {
-    // Resuming sync could issue a reconfiguration, so wait until it finishes.
-    SyncTransportActiveChecker(GetSyncService(0)).Wait();
-  }
+  // Once the auth error is gone, wait for Sync to start up again.
+  GetClient(0)->AwaitSyncSetupCompletion();
 
   // Resuming sync could issue a reconfiguration, so wait until it finishes.
   SyncTransportActiveChecker(GetSyncService(0)).Wait();

chrome/browser/ui/views/profiles/profile_menu_view_browsertest.cc

@@ -54,7 +54,6 @@
 #include "components/signin/public/base/signin_pref_names.h"
 #include "components/signin/public/identity_manager/consent_level.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
-#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
 #include "components/sync/test/fake_server/fake_server_network_resources.h"
@@ -718,10 +717,8 @@ PROFILE_MENU_CLICK_TEST(kActionableItems_SyncPaused,
   sync_harness()->EnterSyncPausedStateForPrimaryAccount();
   // Check that the setup was successful.
   ASSERT_TRUE(identity_manager()->HasPrimaryAccount());
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    ASSERT_EQ(syncer::SyncService::TransportState::PAUSED,
-              sync_service()->GetTransportState());
-  }
+  ASSERT_EQ(syncer::SyncService::TransportState::PAUSED,
+            sync_service()->GetTransportState());
 
   RunTest();
 }

components/sync/driver/profile_sync_service.cc

@@ -486,12 +486,7 @@ bool ProfileSyncService::IsEngineAllowedToRun() const {
   auto disable_reasons = GetDisableReasons();
   disable_reasons.RemoveAll(SyncService::DisableReasonSet(
       DISABLE_REASON_USER_CHOICE, DISABLE_REASON_PLATFORM_OVERRIDE));
-  return disable_reasons.Empty() && !IsInPausedState();
-}
-
-bool ProfileSyncService::IsInPausedState() const {
-  return auth_manager_->IsSyncPaused() &&
-         base::FeatureList::IsEnabled(switches::kStopSyncInPausedState);
+  return disable_reasons.Empty() && !auth_manager_->IsSyncPaused();
 }
 
 void ProfileSyncService::OnProtocolEvent(const ProtocolEvent& event) {
@@ -788,8 +783,8 @@ SyncService::TransportState ProfileSyncService::GetTransportState() const {
   if (!IsEngineAllowedToRun()) {
     // We generally shouldn't have an engine while in a disabled state, but it
     // can happen if this method gets called during ShutdownImpl().
-    return IsInPausedState() ? TransportState::PAUSED
-                             : TransportState::DISABLED;
+    return auth_manager_->IsSyncPaused() ? TransportState::PAUSED
+                                         : TransportState::DISABLED;
   }
 
   if (!engine_ || !engine_->IsInitialized()) {

components/sync/driver/profile_sync_service.h

@@ -285,10 +285,6 @@ class ProfileSyncService : public SyncService,
 
   bool IsEngineAllowedToRun() const;
 
-  // Same as GetTransportState() returning PAUSED. In this state, the engine
-  // shouldn't run.
-  bool IsInPausedState() const;
-
   // Reconfigures the data type manager with the latest enabled types.
   // Note: Does not initialize the engine if it is not already initialized.
   // If a Sync setup is currently in progress (i.e. a settings UI is open), then

components/sync/driver/profile_sync_service_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
@@ -329,19 +330,6 @@ class ProfileSyncServiceTest : public ::testing::Test {
   SyncClientMock* sync_client_;  // Owned by |service_|.
 };
 
-class ProfileSyncServiceTestWithStopSyncInPausedState
-    : public ProfileSyncServiceTest {
- public:
-  ProfileSyncServiceTestWithStopSyncInPausedState() {
-    override_features_.InitAndEnableFeature(switches::kStopSyncInPausedState);
-  }
-
-  ~ProfileSyncServiceTestWithStopSyncInPausedState() override = default;
-
- private:
-  base::test::ScopedFeatureList override_features_;
-};
-
 class ProfileSyncServiceTestWithSyncInvalidationsServiceCreated
     : public ProfileSyncServiceTest {
  public:
@@ -807,7 +795,7 @@ TEST_F(ProfileSyncServiceTest, RevokeAccessTokenFromTokenService) {
   EXPECT_CALL(*component_factory(), CreateSyncEngine)
       .WillOnce(
           Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, base::RepeatingClosure()))));
+              &init_account_id, base::DoNothing()))));
   InitializeForNthSync();
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
@@ -840,9 +828,6 @@ TEST_F(ProfileSyncServiceTest, RevokeAccessTokenFromTokenService) {
 // Checks that CREDENTIALS_REJECTED_BY_CLIENT resets the access token and stops
 // Sync. Regression test for https://crbug.com/824791.
 TEST_F(ProfileSyncServiceTest, CredentialsRejectedByClient_StopSync) {
-  base::test::ScopedFeatureList feature;
-  feature.InitAndEnableFeature(switches::kStopSyncInPausedState);
-
   CoreAccountId init_account_id;
 
   SignIn();
@@ -850,7 +835,7 @@ TEST_F(ProfileSyncServiceTest, CredentialsRejectedByClient_StopSync) {
   EXPECT_CALL(*component_factory(), CreateSyncEngine)
       .WillOnce(
           Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, base::RepeatingClosure()))));
+              &init_account_id, base::DoNothing()))));
   InitializeForNthSync();
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
@@ -898,70 +883,6 @@ TEST_F(ProfileSyncServiceTest, CredentialsRejectedByClient_StopSync) {
   service()->RemoveObserver(&observer);
 }
 
-TEST_F(ProfileSyncServiceTest, CredentialsRejectedByClient_DoNotStopSync) {
-  base::test::ScopedFeatureList feature;
-  feature.InitAndDisableFeature(switches::kStopSyncInPausedState);
-
-  CoreAccountId init_account_id;
-
-  bool invalidate_credentials_called = false;
-  base::RepeatingClosure invalidate_credentials_callback =
-      base::BindRepeating([](bool* called) { *called = true; },
-                          base::Unretained(&invalidate_credentials_called));
-
-  SignIn();
-  CreateService(ProfileSyncService::MANUAL_START);
-  EXPECT_CALL(*component_factory(), CreateSyncEngine)
-      .WillOnce(
-          Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, invalidate_credentials_callback))));
-  InitializeForNthSync();
-  ASSERT_EQ(SyncService::TransportState::ACTIVE,
-            service()->GetTransportState());
-
-  TestSyncServiceObserver observer;
-  service()->AddObserver(&observer);
-
-  const CoreAccountId primary_account_id =
-      identity_manager()->GetPrimaryAccountId();
-
-  // Make sure the expected account_id was passed to the SyncEngine.
-  ASSERT_EQ(primary_account_id, init_account_id);
-
-  // At this point, the real SyncEngine would try to connect to the server, fail
-  // (because it has no access token), and eventually call
-  // OnConnectionStatusChange(CONNECTION_AUTH_ERROR). Since our fake SyncEngine
-  // doesn't do any of this, call that explicitly here.
-  service()->OnConnectionStatusChange(CONNECTION_AUTH_ERROR);
-
-  base::RunLoop().RunUntilIdle();
-  ASSERT_FALSE(service()->GetAccessTokenForTest().empty());
-  ASSERT_EQ(GoogleServiceAuthError::AuthErrorNone(), service()->GetAuthError());
-  ASSERT_EQ(GoogleServiceAuthError::AuthErrorNone(), observer.auth_error());
-
-  // Simulate the credentials getting locally rejected by the client by setting
-  // the refresh token to a special invalid value.
-  identity_test_env()->SetInvalidRefreshTokenForPrimaryAccount();
-  const GoogleServiceAuthError rejected_by_client =
-      GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
-          GoogleServiceAuthError::InvalidGaiaCredentialsReason::
-              CREDENTIALS_REJECTED_BY_CLIENT);
-  ASSERT_EQ(rejected_by_client,
-            identity_test_env()
-                ->identity_manager()
-                ->GetErrorStateOfRefreshTokenForAccount(primary_account_id));
-  EXPECT_TRUE(service()->GetAccessTokenForTest().empty());
-  EXPECT_TRUE(invalidate_credentials_called);
-
-  // The observer should have been notified of the auth error state.
-  EXPECT_EQ(rejected_by_client, observer.auth_error());
-  // The Sync engine should still be running.
-  EXPECT_EQ(SyncService::TransportState::ACTIVE,
-            service()->GetTransportState());
-
-  service()->RemoveObserver(&observer);
-}
-
 // CrOS does not support signout.
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
 TEST_F(ProfileSyncServiceTest, SignOutRevokeAccessToken) {
@@ -972,7 +893,7 @@ TEST_F(ProfileSyncServiceTest, SignOutRevokeAccessToken) {
   EXPECT_CALL(*component_factory(), CreateSyncEngine)
       .WillOnce(
           Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, base::RepeatingClosure()))));
+              &init_account_id, base::DoNothing()))));
   InitializeForNthSync();
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
@@ -1079,7 +1000,7 @@ TEST_F(ProfileSyncServiceTest, CredentialErrorReturned) {
   EXPECT_CALL(*component_factory(), CreateSyncEngine)
       .WillOnce(
           Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, base::RepeatingClosure()))));
+              &init_account_id, base::DoNothing()))));
   InitializeForNthSync();
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
@@ -1140,7 +1061,7 @@ TEST_F(ProfileSyncServiceTest, CredentialErrorClearsOnNewToken) {
   EXPECT_CALL(*component_factory(), CreateSyncEngine)
       .WillOnce(
           Return(ByMove(std::make_unique<FakeSyncEngineCollectCredentials>(
-              &init_account_id, base::RepeatingClosure()))));
+              &init_account_id, base::DoNothing()))));
   InitializeForNthSync();
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
@@ -1391,8 +1312,7 @@ TEST_F(ProfileSyncServiceTest, GenerateCacheGUID) {
 
 // Regression test for crbug.com/1043642, can be removed once
 // ProfileSyncService usages after shutdown are addressed.
-TEST_F(ProfileSyncServiceTestWithStopSyncInPausedState,
-       ShouldProvideDisableReasonsAfterShutdown) {
+TEST_F(ProfileSyncServiceTest, ShouldProvideDisableReasonsAfterShutdown) {
   SignIn();
   CreateService(ProfileSyncService::MANUAL_START);
   InitializeForFirstSync();

components/sync/driver/sync_driver_switches.cc

@@ -40,10 +40,6 @@ const char kSyncShortInitialRetryOverride[] =
 // sure that it's what you want.
 const char kSyncShortNudgeDelayForTest[] = "sync-short-nudge-delay-for-test";
 
-// If enabled, the sync engine will be shut down in the "paused" state.
-const base::Feature kStopSyncInPausedState{"StopSyncInPausedState",
-                                           base::FEATURE_ENABLED_BY_DEFAULT};
-
 // Allows custom passphrase users to receive Wallet data for secondary accounts
 // while in transport-only mode.
 const base::Feature kSyncAllowWalletDataInTransportModeWithCustomPassphrase{

components/sync/driver/sync_driver_switches.h

@@ -25,7 +25,6 @@ extern const char kSyncIncludeSpecificsInProtocolLog[];
 extern const char kSyncShortInitialRetryOverride[];
 extern const char kSyncShortNudgeDelayForTest[];
 
-extern const base::Feature kStopSyncInPausedState;
 extern const base::Feature
     kSyncAllowWalletDataInTransportModeWithCustomPassphrase;
 extern const base::Feature kSyncAutofillWalletOfferData;