[Sync] Refactor model type processor: entity tracker lifetime

Make entity tracker's lifetime same as initial sync done. The tracker
won't exist if we still have no data from the server.

In this patch also authentication_account_id is not set explicitly for
metadata coming from ModelReadyToSync. The behavior for this field
becomes similar to cache_guid. If authentication_account_id differs from
activation request then the metadata will be reset. Generally it should
not affect anything.

In the next patch unittests will be added.

Bug: 947044
Change-Id: I34a75006d1f31888f8ca485a94538b6dd98923c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2038733
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750920}

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -210,6 +210,13 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void MergeDataWithMetadataForDebugging(AllNodesCallback callback,
                                          std::unique_ptr<DataBatch> batch);
 
+  // Initialize the cache_guid for old clients that didn't persist it.
+  void InitializeCacheGuidForOldClients();
+
+  // Checks for valid cache GUID and data type id. Resets state if metadata is
+  // invalid.
+  void CheckForInvalidPersistedMetadata();
+
   /////////////////////
   // Processor state //
   /////////////////////

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -38,6 +38,8 @@ namespace syncer {
 
 namespace {
 
+const char kDefaultAuthenticatedAccountId[] = "DefaultAccountId";
+
 const char kKey1[] = "key1";
 const char kKey2[] = "key2";
 const char kKey3[] = "key3";
@@ -133,6 +135,8 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
     model_type_state.set_initial_sync_done(is_done);
     model_type_state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(model_type_));
+    model_type_state.set_authenticated_account_id(
+        kDefaultAuthenticatedAccountId);
     db_->set_model_type_state(model_type_state);
   }
 
@@ -272,10 +276,10 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   void OnCommitDataLoaded() { bridge()->OnCommitDataLoaded(); }
 
-  void OnSyncStarting(
-      const std::string& authenticated_account_id = "SomeAccountId",
-      const std::string& cache_guid = "TestCacheGuid",
-      SyncMode sync_mode = SyncMode::kFull) {
+  void OnSyncStarting(const std::string& authenticated_account_id =
+                          kDefaultAuthenticatedAccountId,
+                      const std::string& cache_guid = "TestCacheGuid",
+                      SyncMode sync_mode = SyncMode::kFull) {
     DataTypeActivationRequest request;
     request.error_handler = base::BindRepeating(
         &ClientTagBasedModelTypeProcessorTest::ErrorReceived,
@@ -374,7 +378,9 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   // Return the number of entities the processor has metadata for.
   size_t ProcessorEntityCount() const {
-    return type_processor()->entity_tracker_->size();
+    if (type_processor()->entity_tracker_)
+      return type_processor()->entity_tracker_->size();
+    return 0;
   }
 
   // Expect to receive an error from the processor.
@@ -454,9 +460,10 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedAccountId) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedAccountId());
-  OnSyncStarting("SomeAccountId");
+  OnSyncStarting();
   worker()->UpdateFromServer();
-  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
+  EXPECT_EQ(kDefaultAuthenticatedAccountId,
+            type_processor()->TrackedAccountId());
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
@@ -482,7 +489,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedCacheGuid) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedCacheGuid());
-  OnSyncStarting("SomeAccountId", "TestCacheGuid");
+  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid");
   worker()->UpdateFromServer();
   EXPECT_EQ("TestCacheGuid", type_processor()->TrackedCacheGuid());
 }
@@ -493,6 +500,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
   model_type_state.set_cache_guid("PersistedCacheGuid");
+  model_type_state.set_authenticated_account_id(kDefaultAuthenticatedAccountId);
   model_type_state.mutable_progress_marker()->set_data_type_id(
       GetSpecificsFieldNumberFromModelType(GetModelType()));
   metadata_batch->SetModelTypeState(model_type_state);
@@ -502,7 +510,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ("PersistedCacheGuid", type_processor()->TrackedCacheGuid());
 
   // If sync gets started, the cache guid should still be set.
-  OnSyncStarting("SomeAccountId", "PersistedCacheGuid");
+  OnSyncStarting(kDefaultAuthenticatedAccountId, "PersistedCacheGuid");
   EXPECT_EQ("PersistedCacheGuid", type_processor()->TrackedCacheGuid());
 }
 
@@ -1873,7 +1881,8 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kTransportOnly);
+  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
+                 SyncMode::kTransportOnly);
 
   base::HistogramTester histogram_tester;
 
@@ -1897,7 +1906,8 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportPersistentConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kFull);
+  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
+                 SyncMode::kFull);
 
   base::HistogramTester histogram_tester;
 
@@ -1966,7 +1976,8 @@ TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
 TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTimeOnlyForFirstFullUpdate) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kTransportOnly);
+  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
+                 SyncMode::kTransportOnly);
 
   UpdateResponseDataList updates1;
   updates1.push_back(worker()->GenerateUpdateData(
@@ -2530,6 +2541,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldPropagateFailedCommitItemsToBridgeWhenCommitCompleted) {
+  InitializeToReadyState();
   FailedCommitResponseData response_data;
   response_data.client_tag_hash = GetHash("dummy tag");
   response_data.response_type = sync_pb::CommitResponse::TRANSIENT_ERROR;
@@ -2572,6 +2584,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldNotPropagateFailedCommitAttemptToBridgeWhenNoFailedItems) {
+  InitializeToReadyState();
   auto on_commit_attempt_errors_callback = base::BindOnce(
       [](const FailedCommitResponseDataList& error_response_list) {
         ADD_FAILURE()
@@ -2605,8 +2618,9 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedAccountId) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedAccountId());
-  OnSyncStarting("SomeAccountId");
-  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
+  OnSyncStarting();
+  EXPECT_EQ(kDefaultAuthenticatedAccountId,
+            type_processor()->TrackedAccountId());
 }
 
 TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
@@ -2701,4 +2715,50 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ(0U, db()->metadata_count());
 }
 
+TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldResetOnInvalidCacheGuid) {
+  ResetStateWriteItem(kKey1, kValue1);
+  InitializeToMetadataLoaded();
+  OnSyncStarting();
+  OnCommitDataLoaded();
+  ASSERT_EQ(1U, ProcessorEntityCount());
+
+  ResetStateWriteItem(kKey1, kValue1);
+  sync_pb::ModelTypeState model_type_state = db()->model_type_state();
+  model_type_state.set_cache_guid("OtherCacheGuid");
+  db()->set_model_type_state(model_type_state);
+
+  ModelReadyToSync();
+  OnSyncStarting();
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
+TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldResetOnInvalidDataTypeId) {
+  ResetStateWriteItem(kKey1, kValue1);
+  ASSERT_EQ(0U, ProcessorEntityCount());
+
+  // Initialize change processor, expect to load data from the bridge.
+  InitializeToMetadataLoaded();
+  OnSyncStarting();
+  OnCommitDataLoaded();
+  ASSERT_EQ(1U, ProcessorEntityCount());
+
+  ResetStateWriteItem(kKey1, kValue1);
+
+  base::HistogramTester histogram_tester;
+  OnSyncStarting();
+  // Set different data type id.
+  sync_pb::ModelTypeState model_type_state = db()->model_type_state();
+
+  ASSERT_NE(model_type_state.progress_marker().data_type_id(),
+            GetSpecificsFieldNumberFromModelType(AUTOFILL));
+  model_type_state.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(AUTOFILL));
+  db()->set_model_type_state(model_type_state);
+
+  ModelReadyToSync();
+  EXPECT_EQ(0U, ProcessorEntityCount());
+  histogram_tester.ExpectUniqueSample("Sync.PersistedModelTypeIdMismatch",
+                                      ModelTypeForHistograms::kPreferences, 1);
+}
+
 }  // namespace syncer

components/sync/model_impl/processor_entity_tracker.cc

@@ -12,14 +12,10 @@
 
 namespace syncer {
 
-ProcessorEntityTracker::ProcessorEntityTracker(ModelType type) {
-  InitializeMetadata(type);
-}
-
 ProcessorEntityTracker::ProcessorEntityTracker(
+    const sync_pb::ModelTypeState& model_type_state,
     std::map<std::string, std::unique_ptr<sync_pb::EntityMetadata>>
-        metadata_map,
-    const sync_pb::ModelTypeState& model_type_state)
+        metadata_map)
     : model_type_state_(model_type_state) {
   DCHECK(model_type_state.initial_sync_done());
   for (auto& kv : metadata_map) {
@@ -196,11 +192,6 @@ size_t ProcessorEntityTracker::size() const {
   return entities_.size();
 }
 
-void ProcessorEntityTracker::InitializeMetadata(ModelType type) {
-  model_type_state_.mutable_progress_marker()->set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(type));
-}
-
 std::vector<const ProcessorEntity*>
 ProcessorEntityTracker::IncrementSequenceNumberForAllExcept(
     const std::unordered_set<std::string>& already_updated_storage_keys) {

components/sync/model_impl/processor_entity_tracker.h

@@ -23,15 +23,13 @@ class ProcessorEntity;
 // This component tracks entities for ClientTagBasedModelTypeProcessor.
 class ProcessorEntityTracker {
  public:
-  explicit ProcessorEntityTracker(ModelType type);
-
   // Creates tracker and fills entities data from batch metadata map. This
   // constructor must be used only if initial_sync_done returns true in
   // |model_type_state|.
   ProcessorEntityTracker(
+      const sync_pb::ModelTypeState& model_type_state,
       std::map<std::string, std::unique_ptr<sync_pb::EntityMetadata>>
-          metadata_map,
-      const sync_pb::ModelTypeState& model_type_state);
+          metadata_map);
 
   ~ProcessorEntityTracker();
 
@@ -91,9 +89,6 @@ class ProcessorEntityTracker {
     model_type_state_ = model_type_state;
   }
 
-  // Sets data type id to model type state. Used for first time syncing.
-  void InitializeMetadata(ModelType type);
-
   // Returns number of entities, including tombstones.
   size_t size() const;
 

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -228,6 +228,7 @@ class SessionSyncBridgeTest : public ::testing::Test {
     state.set_initial_sync_done(true);
     state.mutable_progress_marker()->set_data_type_id(
         GetSpecificsFieldNumberFromModelType(syncer::SESSIONS));
+    state.set_authenticated_account_id("SomeAccountId");
     syncer::UpdateResponseDataList initial_updates;
     for (const SessionSpecifics& specifics : remote_data) {
       initial_updates.push_back(SpecificsToUpdateResponse(specifics));