[Sync::USS] Discard persisted metadata if they contain wrong type id

Bug: 967424
Change-Id: Ic7ed7f1934357fba85ce0e187298b6508a5e8b8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614179
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664707}

components/autofill/core/browser/webdata/autofill_wallet_metadata_sync_bridge_unittest.cc

@@ -287,6 +287,8 @@ class AutofillWalletMetadataSyncBridgeTest : public testing::Test {
   void ResetBridge(bool initial_sync_done = true) {
     sync_pb::ModelTypeState model_type_state;
     model_type_state.set_initial_sync_done(initial_sync_done);
+    model_type_state.mutable_progress_marker()->set_data_type_id(
+        GetSpecificsFieldNumberFromModelType(syncer::AUTOFILL_WALLET_METADATA));
     EXPECT_TRUE(table()->UpdateModelTypeState(syncer::AUTOFILL_WALLET_METADATA,
                                               model_type_state));
     bridge_.reset(new AutofillWalletMetadataSyncBridge(

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc

@@ -244,6 +244,8 @@ class AutofillWalletSyncBridgeTest : public UssSwitchToggler,
   void ResetBridge(bool initial_sync_done) {
     ModelTypeState model_type_state;
     model_type_state.set_initial_sync_done(initial_sync_done);
+    model_type_state.mutable_progress_marker()->set_data_type_id(
+        GetSpecificsFieldNumberFromModelType(syncer::AUTOFILL_WALLET_DATA));
     EXPECT_TRUE(table()->UpdateModelTypeState(syncer::AUTOFILL_WALLET_DATA,
                                               model_type_state));
     bridge_ = std::make_unique<AutofillWalletSyncBridge>(

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -167,12 +167,17 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
   }
 
   if (!model_type_state_.has_cache_guid()) {
+    // Initialize the cache_guid for old clients that didn't persist it.
     model_type_state_.set_cache_guid(activation_request_.cache_guid);
-  } else if (model_type_state_.cache_guid() != activation_request_.cache_guid) {
-    // There is a mismatch between the cache guid stored in |model_type_state_|
-    // and the one received from sync and stored it |activation_request_|. This
-    // indicates that the stored metadata are invalid (e.g. has been
-    // manipulated) and don't belong to the current syncing client.
+  }
+  // Check for invalid persisted metadata.
+  if (model_type_state_.cache_guid() != activation_request_.cache_guid ||
+      model_type_state_.progress_marker().data_type_id() !=
+          GetSpecificsFieldNumberFromModelType(type_)) {
+    // There is a mismatch between the cache guid or the data type id stored in
+    // |model_type_state_| and the one received from sync. This indicates that
+    // the stored metadata are invalid (e.g. has been manipulated) and don't
+    // belong to the current syncing client.
     ClearMetadataAndResetState();
 
     // The model is still ready to sync (with the same |bridge_|) - replay

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -82,7 +82,8 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
             std::make_unique<ClientTagBasedModelTypeProcessor>(
                 model_type,
                 /*dump_stack=*/base::RepeatingClosure(),
-                commit_only)) {
+                commit_only)),
+        model_type_(model_type) {
     supports_incremental_updates_ = supports_incremental_updates;
   }
 
@@ -114,6 +115,8 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   void SetInitialSyncDone(bool is_done) {
     ModelTypeState model_type_state(db().model_type_state());
     model_type_state.set_initial_sync_done(is_done);
+    model_type_state.mutable_progress_marker()->set_data_type_id(
+        GetSpecificsFieldNumberFromModelType(model_type_));
     db_->set_model_type_state(model_type_state);
   }
 
@@ -168,6 +171,7 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
     data_callback_ = base::BindOnce(std::move(callback), std::move(data));
   }
 
+  const ModelType model_type_;
   bool supports_incremental_updates_;
 
   // The number of times MergeSyncData has been called.
@@ -393,6 +397,8 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
+  model_type_state.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(GetModelType()));
   metadata_batch->SetModelTypeState(model_type_state);
   type_processor()->ModelReadyToSync(std::move(metadata_batch));
 
@@ -419,6 +425,8 @@ 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.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(GetModelType()));
   metadata_batch->SetModelTypeState(model_type_state);
   type_processor()->ModelReadyToSync(std::move(metadata_batch));
 
@@ -2242,6 +2250,38 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ(0U, db()->metadata_count());
 }
 
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldDeleteMetadataWhenDataTypeIdMismatch) {
+  // Commit item.
+  InitializeToReadyState();
+  WriteItemAndAck(kKey1, kValue1);
+  // Reset the processor to simulate a restart.
+  ResetState(/*keep_db=*/true);
+
+  // A new processor loads the metadata after changing the data type id.
+  bridge()->SetInitialSyncDone(true);
+
+  std::unique_ptr<MetadataBatch> metadata_batch = db()->CreateMetadataBatch();
+  sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
+  // This processor is supposed to process Preferences. Mark the model type
+  // state to be for sessions to simulate a data type id mismatch.
+  model_type_state.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(SESSIONS));
+  metadata_batch->SetModelTypeState(model_type_state);
+
+  type_processor()->ModelReadyToSync(std::move(metadata_batch));
+  ASSERT_TRUE(type_processor()->IsModelReadyToSyncForTest());
+
+  OnSyncStarting();
+
+  // Model should still be ready to sync.
+  ASSERT_TRUE(type_processor()->IsModelReadyToSyncForTest());
+  // OnSyncStarting() should have completed.
+  EXPECT_NE(nullptr, worker());
+  // Upon a mismatch, metadata should have been cleared.
+  EXPECT_EQ(0U, db()->metadata_count());
+}
+
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldClearOrphanMetadataInGetLocalChangesWhenDataIsMissing) {
   InitializeToReadyState();
@@ -2446,6 +2486,8 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
+  model_type_state.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(GetModelType()));
   metadata_batch->SetModelTypeState(model_type_state);
   type_processor()->ModelReadyToSync(std::move(metadata_batch));
 
@@ -2472,6 +2514,8 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
   sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
   model_type_state.set_initial_sync_done(true);
   model_type_state.set_authenticated_account_id("PersistedAccountId");
+  model_type_state.mutable_progress_marker()->set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(GetModelType()));
   metadata_batch->SetModelTypeState(model_type_state);
   type_processor()->ModelReadyToSync(std::move(metadata_batch));
 

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -225,6 +225,8 @@ class SessionSyncBridgeTest : public ::testing::Test {
 
     sync_pb::ModelTypeState state;
     state.set_initial_sync_done(true);
+    state.mutable_progress_marker()->set_data_type_id(
+        GetSpecificsFieldNumberFromModelType(syncer::SESSIONS));
     syncer::UpdateResponseDataList initial_updates;
     for (const SessionSpecifics& specifics : remote_data) {
       initial_updates.push_back(SpecificsToUpdateResponse(specifics));