Untrack sync entity metadata if data is gone

This is a mitigation to what are believed to be bugs in the bridges,
which may forget to appropriately report local deletions to the
processor, leading to orphan sync metadata.

We have recently instrumented a UMA metric
("Sync.ModelTypeOrphanMetadata") to assess the severity of the issue,
and the collected data suggests that the problem is not specific to a
single datatype. While we investigate each bridge individually, a
reasonable workaround is to let the processor remove the orphaned
metadata and untrack the entity.

Bugs in the bridges should still be surfaced by the aforementioned
UMA metric, although absolute numbers should tend to be smaller after
this patch, because issues will get fixed after emitting the metric
once.

Bug: 875671,871733
Change-Id: Ieb06692f46739d73ce6f04115c25e14198ade1dd
Reviewed-on: https://chromium-review.googlesource.com/1233676
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592775}

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -1042,7 +1042,8 @@ void ClientTagBasedModelTypeProcessor::ConsumeDataBatch(
     }
   }
 
-  // Report failed loading of entities to UMA.
+  // Detect failed loads that shouldn't have failed.
+  std::vector<std::string> storage_keys_to_untrack;
   for (const std::string& storage_key : storage_keys_to_load) {
     ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
     if (entity == nullptr || entity->metadata().is_deleted()) {
@@ -1050,10 +1051,33 @@ void ClientTagBasedModelTypeProcessor::ConsumeDataBatch(
       // deletion.
       continue;
     }
+    // This scenario indicates a bug in the bridge, which didn't properly
+    // propagate a local deletion to the processor, either in the form of
+    // Delete() or UntrackEntity(). As a workaround to avoid negative side
+    // effects of this inconsistent state, we treat it as if UntrackEntity()
+    // had been called.
+    storage_keys_to_untrack.push_back(storage_key);
     UMA_HISTOGRAM_ENUMERATION("Sync.ModelTypeOrphanMetadata",
                               ModelTypeToHistogramInt(type_),
                               static_cast<int>(MODEL_TYPE_COUNT));
   }
+
+  if (storage_keys_to_untrack.empty()) {
+    return;
+  }
+
+  DCHECK(model_ready_to_sync_);
+  DCHECK(IsTrackingMetadata());
+
+  std::unique_ptr<MetadataChangeList> metadata_changes =
+      bridge_->CreateMetadataChangeList();
+
+  for (const std::string& storage_key : storage_keys_to_untrack) {
+    UntrackEntityForStorageKey(storage_key);
+    metadata_changes->ClearMetadata(storage_key);
+  }
+
+  bridge_->ApplySyncChanges(std::move(metadata_changes), EntityChangeList());
 }
 
 void ClientTagBasedModelTypeProcessor::CommitLocalChanges(

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -1876,36 +1876,51 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
-       ShouldReportOrphanMetadataInGetLocalChangesWhenDataIsMissing) {
+       ShouldClearOrphanMetadataInGetLocalChangesWhenDataIsMissing) {
   InitializeToReadyState();
   bridge()->WriteItem(kKey1, kValue1);
 
   // Loose the entity in the bridge (keeping the metadata around as an orphan).
   bridge()->MimicBugToLooseItemWithoutNotifyingProcessor(kKey1);
 
+  ASSERT_FALSE(db().HasData(kKey1));
+  ASSERT_TRUE(db().HasMetadata(kKey1));
+  ASSERT_NE(nullptr, GetEntityForStorageKey(kKey1));
+
   // Reset "the browser" so that the processor looses the copy of the data.
   ResetState(/*keep_db=*/true);
 
   // Initializing the processor will trigger it to commit again. It does not
   // have a copy of the data so it will ask the bridge.
+  {
+    base::HistogramTester histogram_tester;
+    bridge()->ExpectSynchronousDataCallback();
+    InitializeToReadyState();
+
+    histogram_tester.ExpectBucketCount(
+        "Sync.ModelTypeOrphanMetadata",
+        /*bucket=*/ModelTypeToHistogramInt(GetModelType()), /*count=*/1);
+  }
+
+  // Orphan metadata should have been deleted.
+  EXPECT_EQ(1, bridge()->apply_call_count());
+  EXPECT_FALSE(db().HasMetadata(kKey1));
+  EXPECT_EQ(nullptr, GetEntityForStorageKey(kKey1));
+
   base::HistogramTester histogram_tester;
-  bridge()->ExpectSynchronousDataCallback();
-  InitializeToReadyState();
 
-  // Do it again, explicitly. The processor cannot return the entity as a
-  // LocalChange (because it cannot load its data) so it needs to ask the
-  // bridge.
+  // Do it again, explicitly. The processor does not track the entity so it
+  // shouldn't ask the bridge or return local changes.
   CommitRequestDataList commit_request;
-  bridge()->ExpectSynchronousDataCallback();
   type_processor()->GetLocalChanges(
       INT_MAX, base::BindOnce(&CaptureCommitRequest, &commit_request));
   EXPECT_EQ(0U, commit_request.size());
+  EXPECT_TRUE(bridge()->GetDataCallback().is_null());
 
-  // The processor reports the same orphan to UMA on both calls, so the total
-  // count is 2.
+  // The processor should not report orphan again in UMA.
   histogram_tester.ExpectBucketCount(
       "Sync.ModelTypeOrphanMetadata",
-      /*bucket=*/ModelTypeToHistogramInt(GetModelType()), /*count=*/2);
+      /*bucket=*/ModelTypeToHistogramInt(GetModelType()), /*count=*/0);
 }
 
 TEST_F(
@@ -1917,6 +1932,10 @@ TEST_F(
   // Loose the entity in the bridge (keeping the metadata around as an orphan).
   bridge()->MimicBugToLooseItemWithoutNotifyingProcessor(kKey1);
 
+  ASSERT_FALSE(db().HasData(kKey1));
+  ASSERT_TRUE(db().HasMetadata(kKey1));
+  ASSERT_NE(nullptr, GetEntityForStorageKey(kKey1));
+
   // Reset "the browser" so that the processor looses the copy of the data.
   ResetState(/*keep_db=*/true);
 
@@ -1934,6 +1953,13 @@ TEST_F(
   std::move(bridge()->GetDataCallback()).Run();
   histogram_tester.ExpectTotalCount("Sync.ModelTypeOrphanMetadata",
                                     /*count=*/0);
+
+  EXPECT_EQ(0, bridge()->apply_call_count());
+  EXPECT_EQ(nullptr, GetEntityForStorageKey(kKey1));
+
+  // The expectation below documents the fact that bridges are responsible for
+  // clearing the untracked metadata from their databases.
+  EXPECT_TRUE(db().HasMetadata(kKey1));
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
@@ -1944,6 +1970,10 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   // Loose the entity in the bridge (keeping the metadata around as an orphan).
   bridge()->MimicBugToLooseItemWithoutNotifyingProcessor(kKey1);
 
+  ASSERT_FALSE(db().HasData(kKey1));
+  ASSERT_TRUE(db().HasMetadata(kKey1));
+  ASSERT_NE(nullptr, GetEntityForStorageKey(kKey1));
+
   // Reset "the browser" so that the processor looses the copy of the data.
   ResetState(/*keep_db=*/true);
 
@@ -1961,6 +1991,9 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   std::move(bridge()->GetDataCallback()).Run();
   histogram_tester.ExpectTotalCount("Sync.ModelTypeOrphanMetadata",
                                     /*count=*/0);
+
+  EXPECT_EQ(0, bridge()->apply_call_count());
+  EXPECT_EQ(nullptr, GetEntityForStorageKey(kKey1));
 }
 
 TEST_F(ClientTagBasedModelTypeProcessorTest,
@@ -1968,6 +2001,10 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   InitializeToReadyState();
   bridge()->WriteItem(kKey1, kValue1);
 
+  ASSERT_TRUE(db().HasData(kKey1));
+  ASSERT_TRUE(db().HasMetadata(kKey1));
+  ASSERT_NE(nullptr, GetEntityForStorageKey(kKey1));
+
   // Reset "the browser" so that the processor looses the copy of the data.
   ResetState(/*keep_db=*/true);
 
@@ -1986,6 +2023,10 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   // The processor never reports any orphan.
   histogram_tester.ExpectTotalCount("Sync.ModelTypeOrphanMetadata",
                                     /*count=*/0);
+
+  EXPECT_TRUE(db().HasData(kKey1));
+  EXPECT_TRUE(db().HasMetadata(kKey1));
+  EXPECT_NE(nullptr, GetEntityForStorageKey(kKey1));
 }
 
 class CommitOnlyClientTagBasedModelTypeProcessorTest