Only mark sync enabled after a successful cycle.

Before a sync cycle is complete, we don't know yet if the sync token is
actually valid, because sync may not have tried it yet.

This means that without this CL, if the user was in auth error state,
and Chrome starts, GetUploadToGoogleState reports that uploading to
Google is active until sync actually tries to push or pull some data.

BUG=830570

Change-Id: Id59157db5fd2b83c4bd073cdf61e7c9da0bc730b
Reviewed-on: https://chromium-review.googlesource.com/1000780Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Florian Uunk <feuunk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550129}

chrome/browser/metrics/desktop_session_duration/desktop_profile_session_durations_service.cc

@@ -137,7 +137,8 @@ void DesktopProfileSessionDurationsService::OnStateChanged(
       sync_session_timer_ = std::make_unique<base::ElapsedTimer>();
     }
     sync_status_ = FeatureState::OFF;
-  } else if (sync->IsSyncActive()) {
+  } else if (sync->IsSyncActive() &&
+             sync->GetLastCycleSnapshot().is_initialized()) {
     if (sync_status_ == FeatureState::OFF && sync_session_timer_) {
       LogSyncDuration(sync_session_timer_->Elapsed());
       sync_session_timer_ = std::make_unique<base::ElapsedTimer>();

components/suggestions/suggestions_service_impl_unittest.cc

@@ -91,6 +91,7 @@ class MockSyncService : public syncer::FakeSyncService {
   MOCK_CONST_METHOD0(IsUsingSecondaryPassphrase, bool());
   MOCK_CONST_METHOD0(GetPreferredDataTypes, syncer::ModelTypeSet());
   MOCK_CONST_METHOD0(GetActiveDataTypes, syncer::ModelTypeSet());
+  MOCK_CONST_METHOD0(GetLastCycleSnapshot, syncer::SyncCycleSnapshot());
 };
 
 class TestSuggestionsStore : public suggestions::SuggestionsStore {
@@ -174,6 +175,14 @@ class SuggestionsServiceTest : public testing::Test {
         .Times(AnyNumber())
         .WillRepeatedly(
             Return(syncer::ModelTypeSet(syncer::HISTORY_DELETE_DIRECTIVES)));
+    EXPECT_CALL(*sync_service(), GetLastCycleSnapshot())
+        .Times(AnyNumber())
+        .WillRepeatedly(Return(syncer::SyncCycleSnapshot(
+            syncer::ModelNeutralState(), syncer::ProgressMarkerMap(), false, 5,
+            2, 7, false, 0, base::Time::Now(), base::Time::Now(),
+            std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
+            std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
+            sync_pb::SyncEnums::UNKNOWN_ORIGIN)));
     // These objects are owned by the SuggestionsService, but we keep the
     // pointers around for testing.
     test_suggestions_store_ = new TestSuggestionsStore();

components/sync/driver/sync_service_utils.cc

@@ -6,6 +6,7 @@
 
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_service.h"
+#include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 
 namespace syncer {
 
@@ -21,7 +22,14 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
       sync_service->IsUsingSecondaryPassphrase()) {
     return UploadState::NOT_ACTIVE;
   }
-  if (!sync_service->IsSyncActive() || !sync_service->ConfigurationDone()) {
+  // TODO(crbug.com/831579): We currently need to wait for GetLastCycleSnapshot
+  // to return an initialized snapshot because we don't actually know if the
+  // token is valid until sync has tried it. This is bad because sync can take
+  // arbitrarily long to try the token (especially if the user doesn't have
+  // history sync enabled). Instead, if the identity code would persist
+  // persistent auth errors, we could read those from startup.
+  if (!sync_service->IsSyncActive() || !sync_service->ConfigurationDone() ||
+      !sync_service->GetLastCycleSnapshot().is_initialized()) {
     return UploadState::INITIALIZING;
   }
   return UploadState::ACTIVE;

components/sync/driver/sync_service_utils_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/sync/driver/sync_service_utils.h"
 
+#include <vector>
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/fake_sync_service.h"
 #include "components/sync/driver/sync_service.h"
@@ -26,6 +27,7 @@ class TestSyncService : public FakeSyncService {
   void SetCustomPassphraseEnabled(bool enabled) {
     custom_passphrase_enabled_ = enabled;
   }
+  void SetSyncCycleComplete(bool complete) { sync_cycle_complete_ = complete; }
 
   // SyncService implementation.
   bool IsSyncAllowed() const override { return sync_allowed_; }
@@ -45,6 +47,17 @@ class TestSyncService : public FakeSyncService {
       return ModelTypeSet(syncer::PASSWORDS);
     return preferred_data_types_;
   }
+  SyncCycleSnapshot GetLastCycleSnapshot() const override {
+    if (sync_cycle_complete_) {
+      return SyncCycleSnapshot(ModelNeutralState(), ProgressMarkerMap(), false,
+                               5, 2, 7, false, 0, base::Time::Now(),
+                               base::Time::Now(),
+                               std::vector<int>(MODEL_TYPE_COUNT, 0),
+                               std::vector<int>(MODEL_TYPE_COUNT, 0),
+                               sync_pb::SyncEnums::UNKNOWN_ORIGIN);
+    }
+    return SyncCycleSnapshot();
+  }
   bool ConfigurationDone() const override { return configuration_done_; }
   bool IsUsingSecondaryPassphrase() const override {
     return custom_passphrase_enabled_;
@@ -53,6 +66,7 @@ class TestSyncService : public FakeSyncService {
  private:
   bool sync_allowed_ = false;
   bool sync_active_ = false;
+  bool sync_cycle_complete_ = false;
   bool local_sync_enabled_ = false;
   ModelTypeSet preferred_data_types_;
   bool configuration_done_ = false;
@@ -80,7 +94,8 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfSyncNotAllowed) {
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
 }
 
-TEST(SyncServiceUtilsTest, UploadToGoogleInitializingUntilConfiguredAndActive) {
+TEST(SyncServiceUtilsTest,
+     UploadToGoogleInitializingUntilConfiguredAndActiveAndSyncCycleComplete) {
   TestSyncService service;
   service.SetSyncAllowed(true);
   service.SetPreferredDataTypes(ProtocolTypes());
@@ -94,8 +109,12 @@ TEST(SyncServiceUtilsTest, UploadToGoogleInitializingUntilConfiguredAndActive) {
   EXPECT_EQ(UploadState::INITIALIZING,
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
 
-  // Only after sync is both configured and active is upload actually ACTIVE.
   service.SetSyncActive(true);
+  EXPECT_EQ(UploadState::INITIALIZING,
+            GetUploadToGoogleState(&service, syncer::BOOKMARKS));
+
+  // Only after sync is both configured and active is upload actually ACTIVE.
+  service.SetSyncCycleComplete(true);
   EXPECT_EQ(UploadState::ACTIVE,
             GetUploadToGoogleState(&service, syncer::BOOKMARKS));
 }
@@ -105,6 +124,7 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledForModelType) {
   service.SetSyncAllowed(true);
   service.SetConfigurationDone(true);
   service.SetSyncActive(true);
+  service.SetSyncCycleComplete(true);
 
   // Sync is enabled only for a specific model type.
   service.SetPreferredDataTypes(ModelTypeSet(syncer::BOOKMARKS));
@@ -127,6 +147,11 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfLocalSyncEnabled) {
   service.SetPreferredDataTypes(ProtocolTypes());
   service.SetSyncActive(true);
   service.SetConfigurationDone(true);
+  service.SetSyncCycleComplete(true);
+
+  // Sanity check: Upload is active now.
+  ASSERT_EQ(UploadState::ACTIVE,
+            GetUploadToGoogleState(&service, syncer::BOOKMARKS));
 
   // If we're in "local sync" mode, uploading should never be enabled, even if
   // configuration is done and all the data types are enabled.
@@ -142,6 +167,7 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledOnPersistentAuthError) {
   service.SetPreferredDataTypes(ProtocolTypes());
   service.SetSyncActive(true);
   service.SetConfigurationDone(true);
+  service.SetSyncCycleComplete(true);
 
   // Sanity check: Upload is active now.
   ASSERT_EQ(UploadState::ACTIVE,
@@ -180,6 +206,7 @@ TEST(SyncServiceUtilsTest, UploadToGoogleDisabledIfCustomPassphraseInUse) {
   service.SetPreferredDataTypes(ProtocolTypes());
   service.SetSyncActive(true);
   service.SetConfigurationDone(true);
+  service.SetSyncCycleComplete(true);
 
   // Sanity check: Upload is ACTIVE, even for data types that are always
   // encrypted implicitly (PASSWORDS).