Convert DISABLE_REASON_PAUSED into TransportState::PAUSED

crrev.com/c/1478878 started exposing the web signout state as a
DisableReason in the SyncService API. This caused a number of bugs when
kStopSyncInPausedState was enabled, the main reason being that the
presence of a DisableReason causes the class to consider
sync-the-feature as disabled.

The CL removes the DISABLE_REASON_PAUSED and adds a new transport state
PAUSED. IsEngineAllowedToStart() now checks that a) no reason to disable
sync-the-feature exists; b) sync is not in the paused state.

The feature flag is still kept as DISABLED_BY_DEFAULT, pending
experimentation on Canary/Dev.

TBR=parastoog@google.com

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

chrome/browser/sharing/sharing_utils.cc

@@ -66,7 +66,9 @@ bool IsSyncDisabledForSharing(syncer::SyncService* sync_service) {
     return false;
 
   if (sync_service->GetTransportState() ==
-      syncer::SyncService::TransportState::DISABLED) {
+          syncer::SyncService::TransportState::DISABLED ||
+      sync_service->GetTransportState() ==
+          syncer::SyncService::TransportState::PAUSED) {
     return true;
   }
 

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

@@ -336,9 +336,8 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, SyncPausedState) {
   if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
     // Sync should have shut itself down.
     EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-              syncer::SyncService::TransportState::DISABLED);
-    EXPECT_TRUE(GetSyncService(0)->HasDisableReason(
-        syncer::SyncService::DISABLE_REASON_PAUSED));
+              syncer::SyncService::TransportState::PAUSED);
+    EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
   } else {
     ASSERT_TRUE(AttemptToTriggerAuthError());
 
@@ -405,9 +404,8 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, ShouldTrackDeletionsInSyncPausedState) {
   if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
     // Sync should have shut itself down.
     EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-              syncer::SyncService::TransportState::DISABLED);
-    EXPECT_TRUE(GetSyncService(0)->HasDisableReason(
-        syncer::SyncService::DISABLE_REASON_PAUSED));
+              syncer::SyncService::TransportState::PAUSED);
+    EXPECT_FALSE(GetSyncService(0)->IsEngineInitialized());
   } else {
     ASSERT_TRUE(AttemptToTriggerAuthError());
 

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

@@ -5,6 +5,7 @@
 #include <stddef.h>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
@@ -50,6 +51,7 @@
 #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"
@@ -701,8 +703,10 @@ PROFILE_MENU_CLICK_TEST(kActionableItems_SyncPaused,
   sync_harness()->EnterSyncPausedStateForPrimaryAccount();
   // Check that the setup was successful.
   ASSERT_TRUE(identity_manager()->HasPrimaryAccount());
-  ASSERT_FALSE(sync_service()->HasDisableReason(
-      syncer::SyncService::DISABLE_REASON_PAUSED));
+  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
+    ASSERT_EQ(syncer::SyncService::TransportState::PAUSED,
+              sync_service()->GetTransportState());
+  }
 
   RunTest();
 }

components/autofill/core/browser/personal_data_manager.cc

@@ -591,8 +591,8 @@ AutofillSyncSigninState PersonalDataManager::GetSyncSigninState() const {
     return AutofillSyncSigninState::kSignedInAndSyncFeatureEnabled;
   }
 
-  if (sync_service_->GetDisableReasons() ==
-      syncer::SyncService::DISABLE_REASON_PAUSED) {
+  if (sync_service_->GetTransportState() ==
+      syncer::SyncService::TransportState::PAUSED) {
     return AutofillSyncSigninState::kSyncPaused;
   }
 

components/sync/driver/about_sync_util.cc

@@ -202,8 +202,6 @@ std::string GetDisableReasonsString(
     reason_strings.push_back("User choice");
   if (disable_reasons.Has(SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR))
     reason_strings.push_back("Unrecoverable error");
-  if (disable_reasons.Has(SyncService::DISABLE_REASON_PAUSED))
-    reason_strings.push_back("Paused");
   return base::JoinString(reason_strings, ", ");
 }
 
@@ -211,6 +209,8 @@ std::string GetTransportStateString(syncer::SyncService::TransportState state) {
   switch (state) {
     case syncer::SyncService::TransportState::DISABLED:
       return "Disabled";
+    case syncer::SyncService::TransportState::PAUSED:
+      return "Paused";
     case syncer::SyncService::TransportState::START_DEFERRED:
       return "Start deferred";
     case syncer::SyncService::TransportState::INITIALIZING:

components/sync/driver/profile_sync_service.cc

@@ -275,7 +275,7 @@ ProfileSyncService::ProfileSyncService(InitParams init_params)
   startup_controller_ = std::make_unique<StartupController>(
       base::BindRepeating(&ProfileSyncService::GetPreferredDataTypes,
                           base::Unretained(this)),
-      base::BindRepeating(&ProfileSyncService::IsEngineAllowedToStart,
+      base::BindRepeating(&ProfileSyncService::IsEngineAllowedToRun,
                           base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::StartUpSlowEngineComponents,
                           base::Unretained(this)));
@@ -412,7 +412,7 @@ void ProfileSyncService::CredentialsChanged() {
   // If the engine isn't allowed to start anymore due to the credentials change,
   // then shut down. This happens when the user signs out on the web, i.e. we're
   // in the "Sync paused" state.
-  if (!IsEngineAllowedToStart()) {
+  if (!IsEngineAllowedToRun()) {
     // TODO(crbug/1031162): Remove once traffic investigation is closed.
     EmitUmaMetricWithEmitTimeMinutes(
         "Sync.PeakAnalysis.StopAfterCredentialsChanged");
@@ -436,14 +436,19 @@ void ProfileSyncService::CredentialsChanged() {
   NotifyObservers();
 }
 
-bool ProfileSyncService::IsEngineAllowedToStart() const {
+bool ProfileSyncService::IsEngineAllowedToRun() const {
   // USER_CHOICE (i.e. the Sync feature toggle) and PLATFORM_OVERRIDE (i.e.
   // Android's "MasterSync" toggle) do not prevent starting up the Sync
   // transport.
   auto disable_reasons = GetDisableReasons();
   disable_reasons.RemoveAll(SyncService::DisableReasonSet(
       DISABLE_REASON_USER_CHOICE, DISABLE_REASON_PLATFORM_OVERRIDE));
-  return disable_reasons.Empty();
+  return disable_reasons.Empty() && !IsInPausedState();
+}
+
+bool ProfileSyncService::IsInPausedState() const {
+  return auth_manager_->IsSyncPaused() &&
+         base::FeatureList::IsEnabled(switches::kStopSyncInPausedState);
 }
 
 void ProfileSyncService::OnProtocolEvent(const ProtocolEvent& event) {
@@ -509,7 +514,7 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(ModelType type) {
 }
 
 void ProfileSyncService::StartUpSlowEngineComponents() {
-  DCHECK(IsEngineAllowedToStart());
+  DCHECK(IsEngineAllowedToRun());
 
   const CoreAccountInfo authenticated_account_info =
       GetAuthenticatedAccountInfo();
@@ -756,25 +761,17 @@ SyncService::DisableReasonSet ProfileSyncService::GetDisableReasons() const {
   if (unrecoverable_error_reason_ != ERROR_REASON_UNSET) {
     result.Put(DISABLE_REASON_UNRECOVERABLE_ERROR);
   }
-  if (base::FeatureList::IsEnabled(switches::kStopSyncInPausedState)) {
-    // Some crashes on Chrome OS (crbug.com/1043642) suggest that
-    // ProfileSyncService gets called after its shutdown. It's not clear why
-    // this actually happens. To avoid crashes check that |auth_manager_| isn't
-    // null.
-    if (auth_manager_ && auth_manager_->IsSyncPaused()) {
-      result.Put(DISABLE_REASON_PAUSED);
-    }
-  }
   return result;
 }
 
 SyncService::TransportState ProfileSyncService::GetTransportState() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!IsEngineAllowedToStart()) {
+  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 TransportState::DISABLED;
+    return IsInPausedState() ? TransportState::PAUSED
+                             : TransportState::DISABLED;
   }
 
   if (!engine_ || !engine_->IsInitialized()) {
@@ -900,7 +897,7 @@ void ProfileSyncService::OnEngineInitialized(
   // TODO(treib): Based on some crash reports, it seems like the user could have
   // signed out already at this point, so many of the steps below, including
   // datatype reconfiguration, should not be triggered.
-  DCHECK(IsEngineAllowedToStart());
+  DCHECK(IsEngineAllowedToRun());
 
   // The very first time the backend initializes is effectively the first time
   // we can say we successfully "synced".  LastSyncedTime will only be null in

components/sync/driver/profile_sync_service.h

@@ -275,7 +275,11 @@ class ProfileSyncService : public SyncService,
   // Callbacks for SyncUserSettingsImpl.
   void SyncAllowedByPlatformChanged(bool allowed);
 
-  bool IsEngineAllowedToStart() const;
+  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.

components/sync/driver/profile_sync_service_unittest.cc

@@ -798,9 +798,9 @@ TEST_F(ProfileSyncServiceTest, CredentialsRejectedByClient_StopSync) {
   // The observer should have been notified of the auth error state.
   EXPECT_EQ(rejected_by_client, observer.auth_error());
   // The Sync engine should have been shut down.
-  EXPECT_EQ(SyncService::TransportState::DISABLED,
+  EXPECT_FALSE(service()->IsEngineInitialized());
+  EXPECT_EQ(SyncService::TransportState::PAUSED,
             service()->GetTransportState());
-  EXPECT_TRUE(service()->HasDisableReason(SyncService::DISABLE_REASON_PAUSED));
 
   service()->RemoveObserver(&observer);
 }
@@ -1517,8 +1517,9 @@ TEST_F(ProfileSyncServiceTest,
   identity_test_env()->SetInvalidRefreshTokenForPrimaryAccount();
   ASSERT_EQ(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS,
             service()->GetAuthError().state());
-  ASSERT_EQ(SyncService::DisableReasonSet(SyncService::DISABLE_REASON_PAUSED),
-            service()->GetDisableReasons());
+  ASSERT_EQ(SyncService::TransportState::PAUSED,
+            service()->GetTransportState());
+  ASSERT_TRUE(service()->GetDisableReasons().Empty());
 
   // Verify that sync service does not provide demographics when sync is paused.
   UserDemographicsResult user_demographics_result =

components/sync/driver/sync_service.cc

@@ -40,6 +40,7 @@ bool SyncService::CanSyncFeatureStart() const {
 bool SyncService::IsEngineInitialized() const {
   switch (GetTransportState()) {
     case TransportState::DISABLED:
+    case TransportState::PAUSED:
     case TransportState::START_DEFERRED:
     case TransportState::INITIALIZING:
       return false;
@@ -58,6 +59,7 @@ bool SyncService::IsSyncFeatureActive() const {
   }
   switch (GetTransportState()) {
     case TransportState::DISABLED:
+    case TransportState::PAUSED:
     case TransportState::START_DEFERRED:
     case TransportState::INITIALIZING:
     case TransportState::PENDING_DESIRED_CONFIGURATION:

components/sync/driver/sync_service.h

@@ -138,11 +138,7 @@ class SyncService : public KeyedService {
     // again until either the browser is restarted, or the user fully signs out
     // and back in again.
     DISABLE_REASON_UNRECOVERABLE_ERROR,
-    // Sync is paused because the user signed out on the web. This is different
-    // from NOT_SIGNED_IN: In this case, there *is* still a primary account, but
-    // it doesn't have valid credentials.
-    DISABLE_REASON_PAUSED,
-    DISABLE_REASON_LAST = DISABLE_REASON_PAUSED,
+    DISABLE_REASON_LAST = DISABLE_REASON_UNRECOVERABLE_ERROR,
   };
 
   using DisableReasonSet =
@@ -155,6 +151,9 @@ class SyncService : public KeyedService {
     // Sync is inactive, e.g. due to enterprise policy, or simply because there
     // is no authenticated user.
     DISABLED,
+    // Sync is paused, e.g. because the user signed out on the web, and the
+    // engine is inactive.
+    PAUSED,
     // Sync's startup was deferred, so that it doesn't slow down browser
     // startup. Once the deferral time (usually 10s) expires, or something
     // requests immediate startup, Sync will actually start.

components/sync/driver/sync_service_utils.cc

@@ -42,6 +42,7 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
 
   switch (sync_service->GetTransportState()) {
     case SyncService::TransportState::DISABLED:
+    case SyncService::TransportState::PAUSED:
       return UploadState::NOT_ACTIVE;
 
     case SyncService::TransportState::START_DEFERRED: