[SendTabToSelf] Delete STTS entry on URL removal from history

History observers do not provide notifications on each visit removal.
Instead they provide observers updates when an enitre URLRow is removed.

Bug: 1531691
Change-Id: I92831d6b67698cfb9d8012e47cd3c17d55e1c570
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594658
Commit-Queue: Jeffrey Cohen <jeffreycohen@chromium.org>
Reviewed-by: sebsg <sebsg@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657430}

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

@@ -17,6 +17,7 @@ SendTabToSelfUrlChecker::SendTabToSelfUrlChecker(
     send_tab_to_self::SendTabToSelfSyncService* service,
     const GURL& url)
     : url_(url), service_(service) {
+  DCHECK(service);
   service->GetSendTabToSelfModel()->AddObserver(this);
 }
 
@@ -58,6 +59,8 @@ SendTabToSelfModelEqualityChecker::SendTabToSelfModelEqualityChecker(
     send_tab_to_self::SendTabToSelfSyncService* service0,
     send_tab_to_self::SendTabToSelfSyncService* service1)
     : service0_(service0), service1_(service1) {
+  DCHECK(service0);
+  DCHECK(service1);
   service0->GetSendTabToSelfModel()->AddObserver(this);
   service1->GetSendTabToSelfModel()->AddObserver(this);
 }
@@ -120,6 +123,7 @@ void SendTabToSelfModelEqualityChecker::EntriesRemovedRemotely(
 SendTabToSelfActiveChecker::SendTabToSelfActiveChecker(
     send_tab_to_self::SendTabToSelfSyncService* service)
     : service_(service) {
+  DCHECK(service);
   service->GetSendTabToSelfModel()->AddObserver(this);
 }
 
@@ -173,4 +177,48 @@ void SendTabToSelfMultiDeviceActiveChecker::OnDeviceInfoChange() {
   CheckExitCondition();
 }
 
+SendTabToSelfUrlDeletedChecker::SendTabToSelfUrlDeletedChecker(
+    send_tab_to_self::SendTabToSelfSyncService* service,
+    const GURL& url)
+    : url_(url), service_(service) {
+  DCHECK(service);
+  service->GetSendTabToSelfModel()->AddObserver(this);
+}
+
+SendTabToSelfUrlDeletedChecker::~SendTabToSelfUrlDeletedChecker() {
+  service_->GetSendTabToSelfModel()->RemoveObserver(this);
+}
+
+bool SendTabToSelfUrlDeletedChecker::IsExitConditionSatisfied() {
+  send_tab_to_self::SendTabToSelfModel* model =
+      service_->GetSendTabToSelfModel();
+  DCHECK(model);
+  // Checks each URL in the model and returns passes if URL in question is not
+  // found.
+  for (auto const& guid : model->GetAllGuids()) {
+    if (model->GetEntryByGUID(guid)->GetURL() == url_) {
+      return false;
+    }
+  }
+  return true;
+}
+
+std::string SendTabToSelfUrlDeletedChecker::GetDebugMessage() const {
+  return "Waiting for data for url '" + url_.spec() + "' to be deleted.";
+}
+
+void SendTabToSelfUrlDeletedChecker::SendTabToSelfModelLoaded() {
+  // This ensures that the URL being inspected is present when the model loads.
+  DCHECK(!IsExitConditionSatisfied());
+}
+
+void SendTabToSelfUrlDeletedChecker::EntriesAddedRemotely(
+    const std::vector<const send_tab_to_self::SendTabToSelfEntry*>&
+        new_entries) {}
+
+void SendTabToSelfUrlDeletedChecker::EntriesRemovedRemotely(
+    const std::vector<std::string>& guids_removed) {
+  CheckExitCondition();
+}
+
 }  // namespace send_tab_to_self_helper

chrome/browser/sync/test/integration/send_tab_to_self_helper.h

@@ -26,6 +26,8 @@ class SendTabToSelfUrlChecker
     : public StatusChangeChecker,
       public send_tab_to_self::SendTabToSelfModelObserver {
  public:
+  // The caller must ensure that |service| is not null and will outlive this
+  // object.
   SendTabToSelfUrlChecker(send_tab_to_self::SendTabToSelfSyncService* service,
                           const GURL& url);
   ~SendTabToSelfUrlChecker() override;
@@ -55,6 +57,8 @@ class SendTabToSelfModelEqualityChecker
     : public StatusChangeChecker,
       public send_tab_to_self::SendTabToSelfModelObserver {
  public:
+  // The caller must ensure that |service0| and |service1| are not null and
+  // will outlive this object.
   SendTabToSelfModelEqualityChecker(
       send_tab_to_self::SendTabToSelfSyncService* service0,
       send_tab_to_self::SendTabToSelfSyncService* service1);
@@ -84,6 +88,8 @@ class SendTabToSelfActiveChecker
     : public StatusChangeChecker,
       public send_tab_to_self::SendTabToSelfModelObserver {
  public:
+  // The caller must ensure that |service| is not null and will outlive this
+  // object.
   explicit SendTabToSelfActiveChecker(
       send_tab_to_self::SendTabToSelfSyncService* service);
   ~SendTabToSelfActiveChecker() override;
@@ -126,6 +132,36 @@ class SendTabToSelfMultiDeviceActiveChecker
   DISALLOW_COPY_AND_ASSIGN(SendTabToSelfMultiDeviceActiveChecker);
 };
 
+class SendTabToSelfUrlDeletedChecker
+    : public StatusChangeChecker,
+      public send_tab_to_self::SendTabToSelfModelObserver {
+ public:
+  // The caller must ensure that |service| is not null and will outlive this
+  // object.
+  SendTabToSelfUrlDeletedChecker(
+      send_tab_to_self::SendTabToSelfSyncService* service,
+      const GURL& url);
+  ~SendTabToSelfUrlDeletedChecker() override;
+
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied() override;
+  std::string GetDebugMessage() const override;
+
+  // SendTabToSelfModelObserver implementation.
+  void SendTabToSelfModelLoaded() override;
+  void EntriesAddedRemotely(
+      const std::vector<const send_tab_to_self::SendTabToSelfEntry*>&
+          new_entries) override;
+  void EntriesRemovedRemotely(
+      const std::vector<std::string>& guids_removed) override;
+
+ private:
+  const GURL url_;
+  send_tab_to_self::SendTabToSelfSyncService* const service_;
+
+  DISALLOW_COPY_AND_ASSIGN(SendTabToSelfUrlDeletedChecker);
+};
+
 }  // namespace send_tab_to_self_helper
 
 #endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_SEND_TAB_TO_SELF_HELPER_H_

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

@@ -3,11 +3,14 @@
 // found in the LICENSE file.
 
 #include "base/time/time.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/send_tab_to_self/send_tab_to_self_util.h"
 #include "chrome/browser/sync/send_tab_to_self_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/send_tab_to_self_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/ui/browser.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/keyed_service/core/service_access_type.h"
 #include "components/send_tab_to_self/send_tab_to_self_model.h"
 #include "components/send_tab_to_self/send_tab_to_self_sync_service.h"
 #include "components/sync/driver/sync_driver_switches.h"
@@ -84,4 +87,46 @@ IN_PROC_BROWSER_TEST_F(SingleClientSendTabToSelfSyncTest, ShouldOfferFeature) {
       GetBrowser(0)->tab_strip_model()->GetActiveWebContents()));
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientSendTabToSelfSyncTest,
+                       DeleteSharedEntryWithHistory) {
+  const std::string kUrl("https://www.example.com");
+  const std::string kGuid("kGuid");
+  const base::Time kNavigationTime(base::Time::Now());
+
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::SendTabToSelfSpecifics* send_tab_to_self =
+      specifics.mutable_send_tab_to_self();
+  send_tab_to_self->set_url(kUrl);
+  send_tab_to_self->set_guid(kGuid);
+  send_tab_to_self->set_navigation_time_usec(
+      kNavigationTime.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  send_tab_to_self->set_shared_time_usec(
+      base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
+
+  fake_server_->InjectEntity(
+      syncer::PersistentUniqueClientEntity::CreateFromSpecificsForTesting(
+          "non_unique_name", kGuid, specifics,
+          /*creation_time=*/base::Time::Now().ToTimeT(),
+          /*last_modified_time=*/base::Time::Now().ToTimeT()));
+
+  ASSERT_TRUE(SetupSync());
+
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfile(GetProfile(0),
+                                           ServiceAccessType::EXPLICIT_ACCESS);
+  history_service->AddPage(GURL(kUrl), kNavigationTime, history::SOURCE_SYNCED);
+
+  ASSERT_TRUE(send_tab_to_self_helper::SendTabToSelfUrlChecker(
+                  SendTabToSelfSyncServiceFactory::GetForProfile(GetProfile(0)),
+                  GURL(kUrl))
+                  .Wait());
+
+  history_service->DeleteURL(GURL(kUrl));
+
+  EXPECT_TRUE(send_tab_to_self_helper::SendTabToSelfUrlDeletedChecker(
+                  SendTabToSelfSyncServiceFactory::GetForProfile(GetProfile(0)),
+                  GURL(kUrl))
+                  .Wait());
+}
+
 }  // namespace

components/send_tab_to_self/send_tab_to_self_bridge.cc

@@ -343,18 +343,10 @@ void SendTabToSelfBridge::DeleteEntry(const std::string& guid) {
     return;
   }
 
-  DCHECK(change_processor()->IsTrackingMetadata());
-
   std::unique_ptr<ModelTypeStore::WriteBatch> batch =
       store_->CreateWriteBatch();
-  change_processor()->Delete(guid, batch->GetMetadataChangeList());
 
-  if (mru_entry_ && mru_entry_->GetGUID() == guid) {
-    mru_entry_ = nullptr;
-  }
-
-  entries_.erase(guid);
-  batch->DeleteData(guid);
+  DeleteEntryWithBatch(guid, batch.get());
 
   Commit(std::move(batch));
 }
@@ -378,11 +370,22 @@ void SendTabToSelfBridge::OnURLsDeleted(
     history::HistoryService* history_service,
     const history::DeletionInfo& deletion_info) {
   // We only care about actual user (or sync) deletions.
+
+  if (!change_processor()->IsTrackingMetadata()) {
+    return;  // Sync processor not yet ready, don't sync.
+  }
+
   if (deletion_info.is_from_expiration())
     return;
 
   if (!deletion_info.IsAllHistory()) {
-    // TODO(crbug.com/938102) remove the specific entries that were deleted.
+    std::vector<GURL> urls;
+
+    for (const history::URLRow& row : deletion_info.deleted_rows()) {
+      urls.push_back(row.url());
+    }
+
+    DeleteEntries(urls);
     return;
   }
 
@@ -608,4 +611,49 @@ void SendTabToSelfBridge::SetTargetDeviceNameToCacheInfoMap() {
   }
 }
 
+void SendTabToSelfBridge::DeleteEntryWithBatch(
+    const std::string& guid,
+    ModelTypeStore::WriteBatch* batch) {
+  // Assure that an entry with that guid exists.
+  DCHECK(GetEntryByGUID(guid) != nullptr);
+  DCHECK(change_processor()->IsTrackingMetadata());
+
+  change_processor()->Delete(guid, batch->GetMetadataChangeList());
+
+  if (mru_entry_ && mru_entry_->GetGUID() == guid) {
+    mru_entry_ = nullptr;
+  }
+
+  entries_.erase(guid);
+  batch->DeleteData(guid);
+}
+
+void SendTabToSelfBridge::DeleteEntries(const std::vector<GURL>& urls) {
+  std::unique_ptr<ModelTypeStore::WriteBatch> batch =
+      store_->CreateWriteBatch();
+
+  std::vector<std::string> removed_guids;
+
+  for (const GURL url : urls) {
+    auto entry = entries_.begin();
+    while (entry != entries_.end()) {
+      bool to_delete = (url == entry->second->GetURL());
+
+      std::string guid = entry->first;
+      entry++;
+      if (to_delete) {
+        removed_guids.push_back(guid);
+        DeleteEntryWithBatch(guid, batch.get());
+      }
+    }
+  }
+  Commit(std::move(batch));
+  if (!removed_guids.empty()) {
+    // To err on the side of completeness this notifies all clients that these
+    // entries have been removed. Regardless of if these entries were removed
+    // "remotely".
+    NotifyRemoteSendTabToSelfEntryDeleted(removed_guids);
+  }
+}
+
 }  // namespace send_tab_to_self

components/send_tab_to_self/send_tab_to_self_bridge.h

@@ -103,7 +103,7 @@ class SendTabToSelfBridge : public syncer::ModelTypeSyncBridge,
       const std::vector<const SendTabToSelfEntry*>& new_entries);
 
   // Notify all observers when the entries with |guids| have been removed from
-  // the model via sync.
+  // the model via sync or via history deletion.
   void NotifyRemoteSendTabToSelfEntryDeleted(
       const std::vector<std::string>& guids);
 
@@ -136,6 +136,14 @@ class SendTabToSelfBridge : public syncer::ModelTypeSyncBridge,
   // Sets the target device name to cache guid map.
   void SetTargetDeviceNameToCacheInfoMap();
 
+  // Remove entry with |guid| from entries, but doesn't call Commit on provided
+  // |batch|. This allows multiple for deletions without duplicate batch calls.
+  void DeleteEntryWithBatch(const std::string& guid,
+                            syncer::ModelTypeStore::WriteBatch* batch);
+
+  // Delete all of the entries that match the URLs provided.
+  void DeleteEntries(const std::vector<GURL>& urls);
+
   // |entries_| is keyed by GUIDs.
   SendTabToSelfEntries entries_;
 

components/send_tab_to_self/send_tab_to_self_bridge_unittest.cc

@@ -333,6 +333,46 @@ TEST_F(SendTabToSelfBridgeTest, ApplySyncChangesOneDeletion) {
   EXPECT_EQ(0ul, bridge()->GetAllGuids().size());
 }
 
+// Tests that the send tab to self entry is correctly removed.
+TEST_F(SendTabToSelfBridgeTest, LocalHistoryDeletion) {
+  InitializeBridge();
+  SendTabToSelfEntry entry1("guid1", GURL("http://www.example.com/"), "title",
+                            AdvanceAndGetTime(), AdvanceAndGetTime(), "device",
+                            kLocalDeviceCacheGuid);
+
+  SendTabToSelfEntry entry2("guid2", GURL("http://www.example2.com/"), "title2",
+                            AdvanceAndGetTime(), AdvanceAndGetTime(), "device2",
+                            kLocalDeviceCacheGuid);
+
+  SendTabToSelfEntry entry3("guid3", GURL("http://www.example3.com/"), "title3",
+                            AdvanceAndGetTime(), AdvanceAndGetTime(), "device3",
+                            kLocalDeviceCacheGuid);
+
+  syncer::EntityChangeList add_changes;
+
+  add_changes.push_back(
+      syncer::EntityChange::CreateAdd("guid1", MakeEntityData(entry1)));
+  add_changes.push_back(
+      syncer::EntityChange::CreateAdd("guid2", MakeEntityData(entry2)));
+  add_changes.push_back(
+      syncer::EntityChange::CreateAdd("guid3", MakeEntityData(entry3)));
+
+  bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                             std::move(add_changes));
+
+  ASSERT_EQ(3ul, bridge()->GetAllGuids().size());
+
+  history::URLRows urls_to_remove;
+  urls_to_remove.push_back(history::URLRow(GURL("http://www.example.com/")));
+  urls_to_remove.push_back(history::URLRow(GURL("http://www.example2.com/")));
+
+  EXPECT_CALL(*mock_observer(), EntriesRemovedRemotely(SizeIs(2)));
+
+  bridge()->OnURLsDeleted(nullptr, history::DeletionInfo::ForUrls(
+                                       urls_to_remove, std::set<GURL>()));
+  EXPECT_EQ(1ul, bridge()->GetAllGuids().size());
+}
+
 TEST_F(SendTabToSelfBridgeTest, ApplySyncChangesEmpty) {
   InitializeBridge();
   EXPECT_CALL(*mock_observer(), EntriesAddedRemotely(_)).Times(0);