Revert "[Sync] Refactor model type processor: entity tracker lifetime"

This reverts commit 83dccf5d.

Reason for revert: this patch contains refactoring of client tag based
model type processor. To prevent too many changes in one release it is
better to reland this patch after M83 will be branched.

Original change's description:
> [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}

TBR=mastiz@chromium.org,rushans@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

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

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -210,13 +210,6 @@ 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,8 +38,6 @@ namespace syncer {
 
 namespace {
 
-const char kDefaultAuthenticatedAccountId[] = "DefaultAccountId";
-
 const char kKey1[] = "key1";
 const char kKey2[] = "key2";
 const char kKey3[] = "key3";
@@ -135,8 +133,6 @@ 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);
   }
 
@@ -276,10 +272,10 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   void OnCommitDataLoaded() { bridge()->OnCommitDataLoaded(); }
 
-  void OnSyncStarting(const std::string& authenticated_account_id =
-                          kDefaultAuthenticatedAccountId,
-                      const std::string& cache_guid = "TestCacheGuid",
-                      SyncMode sync_mode = SyncMode::kFull) {
+  void OnSyncStarting(
+      const std::string& authenticated_account_id = "SomeAccountId",
+      const std::string& cache_guid = "TestCacheGuid",
+      SyncMode sync_mode = SyncMode::kFull) {
     DataTypeActivationRequest request;
     request.error_handler = base::BindRepeating(
         &ClientTagBasedModelTypeProcessorTest::ErrorReceived,
@@ -378,9 +374,7 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   // Return the number of entities the processor has metadata for.
   size_t ProcessorEntityCount() const {
-    if (type_processor()->entity_tracker_)
-      return type_processor()->entity_tracker_->size();
-    return 0;
+    return type_processor()->entity_tracker_->size();
   }
 
   // Expect to receive an error from the processor.
@@ -460,10 +454,9 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedAccountId) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedAccountId());
-  OnSyncStarting();
+  OnSyncStarting("SomeAccountId");
   worker()->UpdateFromServer();
-  EXPECT_EQ(kDefaultAuthenticatedAccountId,
-            type_processor()->TrackedAccountId());
+  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
@@ -489,7 +482,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedCacheGuid) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedCacheGuid());
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid");
+  OnSyncStarting("SomeAccountId", "TestCacheGuid");
   worker()->UpdateFromServer();
   EXPECT_EQ("TestCacheGuid", type_processor()->TrackedCacheGuid());
 }
@@ -500,7 +493,6 @@ 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);
@@ -510,7 +502,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ("PersistedCacheGuid", type_processor()->TrackedCacheGuid());
 
   // If sync gets started, the cache guid should still be set.
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "PersistedCacheGuid");
+  OnSyncStarting("SomeAccountId", "PersistedCacheGuid");
   EXPECT_EQ("PersistedCacheGuid", type_processor()->TrackedCacheGuid());
 }
 
@@ -1881,8 +1873,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
-                 SyncMode::kTransportOnly);
+  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kTransportOnly);
 
   base::HistogramTester histogram_tester;
 
@@ -1906,8 +1897,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldReportPersistentConfigurationTime) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
-                 SyncMode::kFull);
+  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kFull);
 
   base::HistogramTester histogram_tester;
 
@@ -1976,8 +1966,7 @@ TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
 TEST_F(FullUpdateClientTagBasedModelTypeProcessorTest,
        ShouldReportEphemeralConfigurationTimeOnlyForFirstFullUpdate) {
   InitializeToMetadataLoaded(/*initial_sync_done=*/false);
-  OnSyncStarting(kDefaultAuthenticatedAccountId, "TestCacheGuid",
-                 SyncMode::kTransportOnly);
+  OnSyncStarting("SomeAccountId", "TestCacheGuid", SyncMode::kTransportOnly);
 
   UpdateResponseDataList updates1;
   updates1.push_back(worker()->GenerateUpdateData(
@@ -2541,7 +2530,6 @@ 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;
@@ -2584,7 +2572,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldNotPropagateFailedCommitAttemptToBridgeWhenNoFailedItems) {
-  InitializeToReadyState();
   auto on_commit_attempt_errors_callback = base::BindOnce(
       [](const FailedCommitResponseDataList& error_response_list) {
         ADD_FAILURE()
@@ -2618,9 +2605,8 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
        ShouldExposeNewlyTrackedAccountId) {
   ModelReadyToSync();
   ASSERT_EQ("", type_processor()->TrackedAccountId());
-  OnSyncStarting();
-  EXPECT_EQ(kDefaultAuthenticatedAccountId,
-            type_processor()->TrackedAccountId());
+  OnSyncStarting("SomeAccountId");
+  EXPECT_EQ("SomeAccountId", type_processor()->TrackedAccountId());
 }
 
 TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
@@ -2715,50 +2701,4 @@ 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,10 +12,14 @@
 
 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)
+        metadata_map,
+    const sync_pb::ModelTypeState& model_type_state)
     : model_type_state_(model_type_state) {
   DCHECK(model_type_state.initial_sync_done());
   for (auto& kv : metadata_map) {
@@ -192,6 +196,11 @@ 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,13 +23,15 @@ 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);
+          metadata_map,
+      const sync_pb::ModelTypeState& model_type_state);
 
   ~ProcessorEntityTracker();
 
@@ -89,6 +91,9 @@ 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,7 +228,6 @@ 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));