Revert "[Typed URLs] Untrack entity from the processor when an URL gets expired"

This reverts commit 6b0d759f.

Reason for revert: Crashing on Canary https://crbug.com/847747.

Original change's description:
> [Typed URLs] Untrack entity from the processor when an URL gets expired
> 
> This CL improves the bug fix for an issue with TYPED_URL. Before this
> CL, one could hit DCHECK if typing an url that got expired in the
> current running instance of Chrome. The DCHECK was due to the sync
> entity still being tracked by ClientTagBasedModelTypeProcessor.
> 
> Bug: 827111
> Change-Id: If040ed1657a5ec3b1bdada79c994f14ec63ce893
> Reviewed-on: https://chromium-review.googlesource.com/1076088
> Commit-Queue: Jan Krcal <jkrcal@chromium.org>
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#562525}

TBR=jkrcal@chromium.org,mastiz@chromium.org

Change-Id: I34842135760215814aa30f88e1e5ac337a808cb0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 827111
Reviewed-on: https://chromium-review.googlesource.com/1078667Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562784}

chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc

@@ -228,14 +228,6 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, AddThenExpireThenAddAgain) {
   urls = GetTypedUrlsFromClient(1);
   ASSERT_EQ(2U, urls.size());
   EXPECT_TRUE(CheckSyncHasURLMetadata(1, url));
-
-  // The first client can add the URL again (regression test for
-  // https://crbug.com/827111).
-  AddUrlToHistoryWithTransition(0, url, ui::PAGE_TRANSITION_TYPED,
-                                history::SOURCE_BROWSED);
-  urls = GetTypedUrlsFromClient(0);
-  EXPECT_EQ(2U, urls.size());
-  EXPECT_TRUE(CheckSyncHasURLMetadata(0, url));
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, E2E_ENABLED(AddThenDelete)) {

components/history/core/browser/typed_url_sync_bridge.cc

@@ -416,7 +416,11 @@ void TypedURLSyncBridge::OnURLsDeleted(HistoryBackend* history_backend,
       std::string storage_key = GetStorageKeyFromURLRow(row);
       sync_metadata_database_->ClearSyncMetadata(syncer::TYPED_URLS,
                                                  storage_key);
-      change_processor()->UntrackEntityForStorageKey(storage_key);
+      // TODO(jkrcal): Untrack the entity from the processor, too (by
+      // introducing UntrackEntityForStorageKey() function into the change
+      // processor). Extend the integration tests to cover the crash in
+      // https://crbug.com/827111. Also add unit-test coverage for expired
+      // deletions (needs some refactoring in the tests).
     }
     return;
   }

components/history/core/browser/typed_url_sync_bridge_unittest.cc

@@ -1034,11 +1034,10 @@ TEST_F(TypedURLSyncBridgeTest, ExpireLocalTypedUrl) {
                             EntityChange::ACTION_ADD);
   }
 
-  // Check all the metadata is here, no need to untrack anything so far.
+  // Check all the metadata is here.
   MetadataBatch metadata_batch;
   metadata_store()->GetAllSyncMetadata(&metadata_batch);
   ASSERT_EQ(5u, metadata_batch.TakeAllMetadata().size());
-  ASSERT_EQ(0U, processor().untrack_for_storage_key_set().size());
 
   // Simulate expiration - delete some urls from the backend and create deleted
   // row vector.
@@ -1058,8 +1057,6 @@ TEST_F(TypedURLSyncBridgeTest, ExpireLocalTypedUrl) {
   // This does not propagate to the processor.
   EXPECT_EQ(0U, processor().put_multimap().size() - previous_put_size);
   EXPECT_EQ(0U, processor().delete_set().size());
-  // The processor is still informed to clear its in-memory maps.
-  EXPECT_EQ(3U, processor().untrack_for_storage_key_set().size());
 
   // The urls are removed from the metadata store.
   MetadataBatch smaller_metadata_batch;

components/sync/model/fake_model_type_change_processor.cc

@@ -43,9 +43,6 @@ void FakeModelTypeChangeProcessor::UpdateStorageKey(
 void FakeModelTypeChangeProcessor::UntrackEntity(
     const EntityData& entity_data) {}
 
-void FakeModelTypeChangeProcessor::UntrackEntityForStorageKey(
-    const std::string& storage_key) {}
-
 void FakeModelTypeChangeProcessor::OnModelStarting(
     ModelTypeSyncBridge* bridge) {}
 

components/sync/model/fake_model_type_change_processor.h

@@ -36,7 +36,6 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
-  void UntrackEntityForStorageKey(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;

components/sync/model/mock_model_type_change_processor.cc

@@ -44,10 +44,6 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     other_->UntrackEntity(entity_data);
   }
 
-  void UntrackEntityForStorageKey(const std::string& storage_key) override {
-    other_->UntrackEntityForStorageKey(storage_key);
-  }
-
   void OnModelStarting(ModelTypeSyncBridge* bridge) override {
     other_->OnModelStarting(bridge);
   }
@@ -102,9 +98,6 @@ void MockModelTypeChangeProcessor::DelegateCallsByDefaultTo(
   ON_CALL(*this, UntrackEntity(_))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::UntrackEntity));
-  ON_CALL(*this, UntrackEntityForStorageKey(_))
-      .WillByDefault(Invoke(
-          delegate, &ModelTypeChangeProcessor::UntrackEntityForStorageKey));
   ON_CALL(*this, OnModelStarting(_))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::OnModelStarting));

components/sync/model/mock_model_type_change_processor.h

@@ -31,8 +31,6 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                     const std::string& storage_key,
                     MetadataChangeList* metadata_change_list));
   MOCK_METHOD1(UntrackEntity, void(const EntityData& entity_data));
-  MOCK_METHOD1(UntrackEntityForStorageKey,
-               void(const std::string& storage_key));
   MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   MOCK_METHOD1(ModelReadyToSync, void(std::unique_ptr<MetadataBatch> batch));
   MOCK_METHOD0(IsTrackingMetadata, bool());

components/sync/model/model_type_change_processor.h

@@ -57,12 +57,6 @@ class ModelTypeChangeProcessor {
   // change_processor()->Delete() instead.
   virtual void UntrackEntity(const EntityData& entity_data) = 0;
 
-  // Remove entity metadata and do not track the entity. Applies to datatypes
-  // that support local deletions that should not get synced up (e.g. TYPED_URL
-  // does not sync up deletions of expired URLs). If the deletion should get
-  // synced up, use change_processor()->Delete() instead.
-  virtual void UntrackEntityForStorageKey(const std::string& storage_key) = 0;
-
   // Pass the pointer to the processor so that the processor can notify the
   // bridge of various events; |bridge| must not be nullptr and must outlive
   // this object.

components/sync/model/recording_model_type_change_processor.cc

@@ -43,11 +43,6 @@ void RecordingModelTypeChangeProcessor::UntrackEntity(
   untrack_set_.insert(FakeModelTypeSyncBridge::CopyEntityData(entity_data));
 }
 
-void RecordingModelTypeChangeProcessor::UntrackEntityForStorageKey(
-    const std::string& storage_key) {
-  untrack_for_storage_key_set_.insert(storage_key);
-}
-
 void RecordingModelTypeChangeProcessor::ModelReadyToSync(
     std::unique_ptr<MetadataBatch> batch) {
   std::swap(metadata_, batch);

components/sync/model/recording_model_type_change_processor.h

@@ -32,7 +32,6 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
-  void UntrackEntityForStorageKey(const std::string& storage_key) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
 
@@ -54,10 +53,6 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
     return untrack_set_;
   }
 
-  const std::set<std::string>& untrack_for_storage_key_set() const {
-    return untrack_for_storage_key_set_;
-  }
-
   MetadataBatch* metadata() const { return metadata_.get(); }
 
   // Constructs the processor and assigns its raw pointer to the given address.
@@ -73,7 +68,6 @@ class RecordingModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
   std::multimap<std::string, std::unique_ptr<EntityData>> update_multimap_;
   std::set<std::string> delete_set_;
   std::set<std::unique_ptr<EntityData>> untrack_set_;
-  std::set<std::string> untrack_for_storage_key_set_;
   std::unique_ptr<MetadataBatch> metadata_;
   bool is_tracking_metadata_ = true;
 };

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -341,14 +341,6 @@ void ClientTagBasedModelTypeProcessor::UntrackEntity(
   entities_.erase(client_tag_hash);
 }
 
-void ClientTagBasedModelTypeProcessor::UntrackEntityForStorageKey(
-    const std::string& storage_key) {
-  ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
-  DCHECK_NE(entity, nullptr);
-  entities_.erase(entity->metadata().client_tag_hash());
-  storage_key_to_tag_hash_.erase(storage_key);
-}
-
 void ClientTagBasedModelTypeProcessor::NudgeForCommitIfNeeded() {
   // Don't bother sending anything if there's no one to send to.
   if (!IsConnected())

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -63,7 +63,6 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
-  void UntrackEntityForStorageKey(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -62,12 +62,6 @@ void CaptureCommitRequest(CommitRequestDataList* dst,
   *dst = std::move(src);
 }
 
-void CaptureStatusCounters(StatusCounters* dst,
-                           ModelType model_type,
-                           const StatusCounters& counters) {
-  *dst = counters;
-}
-
 class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
  public:
   explicit TestModelTypeSyncBridge(bool commit_only)
@@ -248,11 +242,6 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
     return;
   }
 
-  ProcessorEntityTracker* GetEntityForStorageKey(
-      const std::string& storage_key) {
-    return type_processor()->GetEntityForStorageKey(storage_key);
-  }
-
   void ResetState(bool keep_db) {
     bridge_ = keep_db
                   ? std::make_unique<TestModelTypeSyncBridge>(
@@ -1524,35 +1513,6 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, UntrackEntity) {
   EXPECT_EQ(0, bridge()->get_storage_key_call_count());
 }
 
-// Tests that UntrackEntityForStorage won't propagate storage key to
-// ProcessorEntityTracker, and no entity's metadata are added into
-// MetadataChangeList.
-TEST_F(ClientTagBasedModelTypeProcessorTest, UntrackEntityForStorageKey) {
-  InitializeToReadyState();
-  bridge()->WriteItem(kKey1, kValue1);
-  worker()->VerifyPendingCommits({{kHash1}});
-  worker()->AckOnePendingCommit();
-
-  // Check the processor tracks the entity.
-  StatusCounters status_counters;
-  type_processor()->GetStatusCountersForDebugging(
-      base::BindOnce(&CaptureStatusCounters, &status_counters));
-  ASSERT_EQ(1u, status_counters.num_entries);
-  ASSERT_NE(nullptr, GetEntityForStorageKey(kKey1));
-
-  // The bridge deletes the data locally and does not want to sync the deletion.
-  // It only untracks the entity.
-  type_processor()->UntrackEntityForStorageKey(kKey1);
-
-  // The deletion is not synced up.
-  worker()->VerifyPendingCommits({});
-  // The processor tracks no entity any more.
-  type_processor()->GetStatusCountersForDebugging(
-      base::BindOnce(&CaptureStatusCounters, &status_counters));
-  EXPECT_EQ(status_counters.num_entries, 0U);
-  EXPECT_EQ(nullptr, GetEntityForStorageKey(kKey1));
-}
-
 // Tests that ClientTagBasedModelTypeProcessor can do garbage collection by
 // version. Create 2 entries, one is version 1, another is version 3. Check if
 // sync will delete version 1 entry when server set expired version is 2.