SyncDisableObserver: Don't use ProfileSyncService::GetSyncTokenStatus

GetSyncTokenStatus is otherwise only used for debug UI, and I'd like to
clearly mark it as such. It's also *almost* identical to other state
exposed from ProfileSyncService.

Behavior difference:
Before: When the access token expires, the ConnectionStatus in the
SyncTokenStatus temporarily changes to CONNECTION_AUTH_ERROR until a
new access token is acquired, and so UKM is temporarily disabled.
After: UKM remains enabled while a new access token is fetched, and
only gets disabled if that fails.

Bug: 839834, 953272
Change-Id: I68a6ada78a654290e2267de10d6430cbf2ae7195
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1065741
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653514}

components/ukm/BUILD.gn

@@ -56,6 +56,7 @@ static_library("observers") {
     "//base",
     "//components/history/core/browser",
     "//components/sync",
+    "//google_apis",
   ]
 
   public_deps = [

components/ukm/observers/DEPS

 include_rules = [
   "+components/history/core/browser",
   "+components/sync",
+  "+google_apis",
 ]

components/ukm/observers/sync_disable_observer.cc

@@ -10,11 +10,10 @@
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
-#include "components/sync/driver/sync_token_status.h"
 #include "components/sync/driver/sync_user_settings.h"
-#include "components/sync/engine/connection_status.h"
 #include "components/unified_consent/feature.h"
 #include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
+#include "google_apis/gaia/google_service_auth_error.h"
 
 using unified_consent::UrlKeyedDataCollectionConsentHelper;
 
@@ -80,7 +79,6 @@ bool SyncDisableObserver::SyncState::AllowsUkmWithExtension() const {
 SyncDisableObserver::SyncState SyncDisableObserver::GetSyncState(
     syncer::SyncService* sync_service,
     UrlKeyedDataCollectionConsentHelper* consent_helper) {
-  syncer::SyncTokenStatus status = sync_service->GetSyncTokenStatus();
   SyncState state;
 
   // For the following two settings, we want them to match the state of a user
@@ -107,11 +105,19 @@ SyncDisableObserver::SyncState SyncDisableObserver::GetSyncState(
 
   state.initialized = sync_service->IsEngineInitialized();
 
-  // We use CONNECTION_OK here as an auth error can be used in the sync
-  // paused state. Therefore we need to be more direct and check CONNECTION_OK
-  // as opposed to using something like IsSyncFeatureActive().
-  state.connected = !base::FeatureList::IsEnabled(kUkmCheckAuthErrorFeature) ||
-                    status.connection_status == syncer::CONNECTION_OK;
+  // Reasoning for the individual checks:
+  // - IsSyncFeatureActive() makes sure Sync is enabled and initialized.
+  // - HasCompletedSyncCycle() makes sure Sync has actually talked to the
+  //   server. Without this, it's possible that there is some auth issue that we
+  //   just haven't detected yet.
+  // - Finally, GetAuthError() makes sure Sync is not in an auth error state. In
+  //   particular, this includes the "Sync paused" state (which is implemented
+  //   as an auth error).
+  state.connected =
+      !base::FeatureList::IsEnabled(kUkmCheckAuthErrorFeature) ||
+      (sync_service->IsSyncFeatureActive() &&
+       sync_service->HasCompletedSyncCycle() &&
+       sync_service->GetAuthError() == GoogleServiceAuthError::AuthErrorNone());
 
   state.passphrase_protected =
       state.initialized &&

components/ukm/observers/sync_disable_observer_unittest.cc

@@ -7,7 +7,7 @@
 #include "base/observer_list.h"
 #include "components/sync/driver/sync_token_status.h"
 #include "components/sync/driver/test_sync_service.h"
-#include "components/sync/engine/connection_status.h"
+#include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/unified_consent/feature.h"
 #include "components/unified_consent/pref_names.h"
@@ -24,21 +24,36 @@ namespace {
 
 class MockSyncService : public syncer::TestSyncService {
  public:
-  MockSyncService() { SetTransportState(TransportState::INITIALIZING); }
+  MockSyncService() {
+    SetTransportState(TransportState::INITIALIZING);
+    SetLastCycleSnapshot(syncer::SyncCycleSnapshot());
+  }
   ~MockSyncService() override { Shutdown(); }
 
-  void SetStatus(bool has_passphrase, bool history_enabled) {
-    SetTransportState(TransportState::ACTIVE);
+  void SetStatus(bool has_passphrase, bool history_enabled, bool active) {
+    SetTransportState(active ? TransportState::ACTIVE
+                             : TransportState::INITIALIZING);
     SetIsUsingSecondaryPassphrase(has_passphrase);
     SetPreferredDataTypes(
         history_enabled
             ? syncer::ModelTypeSet(syncer::HISTORY_DELETE_DIRECTIVES)
             : syncer::ModelTypeSet());
+
+    // It doesn't matter what exactly we set here, it's only relevant that the
+    // SyncCycleSnapshot is initialized at all.
+    SetLastCycleSnapshot(syncer::SyncCycleSnapshot(
+        syncer::ModelNeutralState(), syncer::ProgressMarkerMap(), false, 0, 0,
+        0, true, 0, base::Time::Now(), base::Time::Now(),
+        std::vector<int>(syncer::ModelType::NUM_ENTRIES, 0),
+        std::vector<int>(syncer::ModelType::NUM_ENTRIES, 0),
+        sync_pb::SyncEnums::UNKNOWN_ORIGIN, base::TimeDelta::FromMinutes(1),
+        false));
+
     NotifyObserversOfStateChanged();
   }
 
-  void SetConnectionStatus(syncer::ConnectionStatus status) {
-    connection_status_ = status;
+  void SetAuthError(GoogleServiceAuthError::State error_state) {
+    syncer::TestSyncService::SetAuthError(GoogleServiceAuthError(error_state));
     NotifyObserversOfStateChanged();
   }
 
@@ -48,12 +63,6 @@ class MockSyncService : public syncer::TestSyncService {
     }
   }
 
-  void NotifyObserversOfStateChanged() {
-    for (auto& observer : observers_) {
-      observer.OnStateChanged(this);
-    }
-  }
-
  private:
   // syncer::TestSyncService:
   void AddObserver(syncer::SyncServiceObserver* observer) override {
@@ -62,13 +71,12 @@ class MockSyncService : public syncer::TestSyncService {
   void RemoveObserver(syncer::SyncServiceObserver* observer) override {
     observers_.RemoveObserver(observer);
   }
-  syncer::SyncTokenStatus GetSyncTokenStatus() const override {
-    syncer::SyncTokenStatus status;
-    status.connection_status = connection_status_;
-    return status;
-  }
 
-  syncer::ConnectionStatus connection_status_ = syncer::CONNECTION_OK;
+  void NotifyObserversOfStateChanged() {
+    for (auto& observer : observers_) {
+      observer.OnStateChanged(this);
+    }
+  }
 
   // The list of observers of the SyncService state.
   base::ObserverList<syncer::SyncServiceObserver>::Unchecked observers_;
@@ -133,12 +141,24 @@ TEST_F(SyncDisableObserverTest, NoProfiles) {
   EXPECT_FALSE(observer.ResetPurged());
 }
 
+TEST_F(SyncDisableObserverTest, NotActive) {
+  MockSyncService sync;
+  sync.SetStatus(false, true, false);
+  sync_preferences::TestingPrefServiceSyncable prefs;
+  RegisterUrlKeyedAnonymizedDataCollectionPref(prefs);
+  TestSyncDisableObserver observer;
+  observer.ObserveServiceForSyncDisables(&sync, &prefs);
+  EXPECT_FALSE(observer.SyncStateAllowsUkm());
+  EXPECT_FALSE(observer.ResetNotified());
+  EXPECT_FALSE(observer.ResetPurged());
+}
+
 TEST_F(SyncDisableObserverTest, OneEnabled_UnifiedConsentDisabled) {
   ScopedUnifiedConsent scoped_unified_consent(
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync;
-  sync.SetStatus(false, true);
+  sync.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync, nullptr);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
   EXPECT_TRUE(observer.ResetNotified());
@@ -155,7 +175,7 @@ TEST_F(SyncDisableObserverTest, OneEnabled_UnifiedConsentEnabled) {
   for (bool has_passphrase : {true, false}) {
     for (bool history_enabled : {true, false}) {
       TestSyncDisableObserver observer;
-      sync.SetStatus(has_passphrase, history_enabled);
+      sync.SetStatus(has_passphrase, history_enabled, /*active=*/true);
       observer.ObserveServiceForSyncDisables(&sync, &prefs);
       EXPECT_TRUE(observer.SyncStateAllowsUkm());
       EXPECT_TRUE(observer.ResetNotified());
@@ -168,7 +188,7 @@ TEST_F(SyncDisableObserverTest, Passphrased_UnifiedConsentDisabled) {
   ScopedUnifiedConsent scoped_unified_consent(
       UnifiedConsentFeatureState::kDisabled);
   MockSyncService sync;
-  sync.SetStatus(true, true);
+  sync.SetStatus(true, true, true);
   TestSyncDisableObserver observer;
   observer.ObserveServiceForSyncDisables(&sync, nullptr);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
@@ -181,7 +201,7 @@ TEST_F(SyncDisableObserverTest, HistoryDisabled_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync;
-  sync.SetStatus(false, false);
+  sync.SetStatus(false, false, true);
   observer.ObserveServiceForSyncDisables(&sync, nullptr);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
   EXPECT_FALSE(observer.ResetNotified());
@@ -193,12 +213,12 @@ TEST_F(SyncDisableObserverTest, AuthError_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync;
-  sync.SetStatus(false, true);
+  sync.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync, nullptr);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
-  sync.SetConnectionStatus(syncer::CONNECTION_AUTH_ERROR);
+  sync.SetAuthError(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
-  sync.SetConnectionStatus(syncer::CONNECTION_OK);
+  sync.SetAuthError(GoogleServiceAuthError::NONE);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
 }
 
@@ -207,10 +227,10 @@ TEST_F(SyncDisableObserverTest, MixedProfiles1_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync1;
-  sync1.SetStatus(false, false);
+  sync1.SetStatus(false, false, true);
   observer.ObserveServiceForSyncDisables(&sync1, nullptr);
   MockSyncService sync2;
-  sync2.SetStatus(false, true);
+  sync2.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync2, nullptr);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
   EXPECT_FALSE(observer.ResetNotified());
@@ -222,12 +242,12 @@ TEST_F(SyncDisableObserverTest, MixedProfiles2_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync1;
-  sync1.SetStatus(false, true);
+  sync1.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync1, nullptr);
   EXPECT_TRUE(observer.ResetNotified());
 
   MockSyncService sync2;
-  sync2.SetStatus(false, false);
+  sync2.SetStatus(false, false, true);
   observer.ObserveServiceForSyncDisables(&sync2, nullptr);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
   EXPECT_TRUE(observer.ResetNotified());
@@ -263,11 +283,11 @@ TEST_F(SyncDisableObserverTest, TwoEnabled_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync1;
-  sync1.SetStatus(false, true);
+  sync1.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync1, nullptr);
   EXPECT_TRUE(observer.ResetNotified());
   MockSyncService sync2;
-  sync2.SetStatus(false, true);
+  sync2.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync2, nullptr);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
   EXPECT_FALSE(observer.ResetNotified());
@@ -304,7 +324,7 @@ TEST_F(SyncDisableObserverTest, OneAddRemove_UnifiedConsentDisabled) {
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
   EXPECT_FALSE(observer.ResetNotified());
   EXPECT_FALSE(observer.ResetPurged());
-  sync.SetStatus(false, true);
+  sync.SetStatus(false, true, true);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
   EXPECT_TRUE(observer.ResetNotified());
   EXPECT_FALSE(observer.ResetPurged());
@@ -340,12 +360,12 @@ TEST_F(SyncDisableObserverTest, PurgeOnDisable_UnifiedConsentDisabled) {
       UnifiedConsentFeatureState::kDisabled);
   TestSyncDisableObserver observer;
   MockSyncService sync;
-  sync.SetStatus(false, true);
+  sync.SetStatus(false, true, true);
   observer.ObserveServiceForSyncDisables(&sync, nullptr);
   EXPECT_TRUE(observer.SyncStateAllowsUkm());
   EXPECT_TRUE(observer.ResetNotified());
   EXPECT_FALSE(observer.ResetPurged());
-  sync.SetStatus(false, false);
+  sync.SetStatus(false, false, true);
   EXPECT_FALSE(observer.SyncStateAllowsUkm());
   EXPECT_TRUE(observer.ResetNotified());
   EXPECT_TRUE(observer.ResetPurged());