Avoid DCHECK failure if received unexpected client tag hash

We shouldn't DCHECK-fail for conditions that depend on external
data, which in this case involves a reply from the sync server.
Instead, let's just ignore those updates.

Bug: None
Change-Id: I4e73d5bc68685399b74eb026af6cab344e530b3e
Reviewed-on: https://chromium-review.googlesource.com/1248628Reviewed-by: Florian Uunk <feuunk@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594803}

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -366,8 +366,17 @@ void ClientTagBasedModelTypeProcessor::Put(
 
   ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
   if (entity == nullptr) {
-    // The bridge is creating a new entity.
-    data->client_tag_hash = GetClientTagHash(storage_key, *data);
+    // The bridge is creating a new entity. The bridge may or may not populate
+    // |data->client_tag_hash|, so let's ask for the client tag if needed.
+    if (data->client_tag_hash.empty()) {
+      data->client_tag_hash = GetClientTagHash(storage_key, *data);
+    } else {
+      // If the Put() call already included the client tag, let's verify that
+      // it's consistent with the bridge's regular GetClientTag() function.
+      DCHECK_EQ(data->client_tag_hash,
+                GenerateSyncableHash(type_, bridge_->GetClientTag(*data)));
+    }
+
     if (data->creation_time.is_null())
       data->creation_time = base::Time::Now();
     if (data->modification_time.is_null())
@@ -667,6 +676,14 @@ ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::ProcessUpdate(
     return nullptr;
   }
 
+  // Filter out unexpected client tag hashes.
+  if (!data.is_deleted() &&
+      client_tag_hash !=
+          GenerateSyncableHash(type_, bridge_->GetClientTag(data))) {
+    DLOG(WARNING) << "Received unexpected client tag hash: " << client_tag_hash;
+    return nullptr;
+  }
+
   ProcessorEntityTracker* entity = GetEntityForTagHash(client_tag_hash);
 
   // Handle corner cases first.
@@ -1104,6 +1121,7 @@ ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::GetEntityForTagHash(
 ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::CreateEntity(
     const std::string& storage_key,
     const EntityData& data) {
+  DCHECK(!data.client_tag_hash.empty());
   DCHECK(entities_.find(data.client_tag_hash) == entities_.end());
   DCHECK(!bridge_->SupportsGetStorageKey() || !storage_key.empty());
   DCHECK(storage_key.empty() || storage_key_to_tag_hash_.find(storage_key) ==
@@ -1120,7 +1138,6 @@ ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::CreateEntity(
 
 ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::CreateEntity(
     const EntityData& data) {
-  // Verify the tag hash matches, may be relaxed in the future.
   DCHECK_EQ(data.client_tag_hash,
             GenerateSyncableHash(type_, bridge_->GetClientTag(data)));
   std::string storage_key;

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -756,7 +756,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   entity_data->specifics.mutable_preference()->set_value(kValue1);
 
   entity_data->non_unique_name = kKey1;
-  entity_data->client_tag_hash = kHash3;
+  entity_data->client_tag_hash = kHash1;
   entity_data->id = kId1;
   bridge()->WriteItem(kKey1, std::move(entity_data));
 
@@ -909,6 +909,15 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_EQ(0U, ProcessorEntityCount());
 }
 
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldIgnoreRemoteUpdatesWithUnexpectedClientTagHash) {
+  InitializeToReadyState();
+  worker()->UpdateFromServer(kHash2, GenerateSpecifics(kKey1, kValue1));
+  EXPECT_EQ(0U, db().data_count());
+  EXPECT_EQ(0U, db().metadata_count());
+  EXPECT_EQ(0U, ProcessorEntityCount());
+}
+
 // Test that an error applying changes from a server update is
 // propagated to the error handler.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldReportErrorApplyingUpdate) {