Delete template URLs when individual history item is deleted.

The TemplateURL were not deleted when individual history
items were removed. This is deleted only via clear browsing.

The fix was to delete template urls for only those origins
which don't have any urls trace in the history. The deletion
is now made common for individual history deletion and clear
browsing data.

This CL also adds a browser test, where we add an URL to the history
and delete it and checks if the corresponding template url is
deleted too.

Bug: 87322
Change-Id: I0b51021c17b397d8ba172d0f882778436dd66c60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821611
Commit-Queue: Rohit Agarwal <roagarwal@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735488}

chrome/browser/browsing_data/browsing_data_history_observer_service.cc

@@ -7,14 +7,91 @@
 #include "chrome/browser/browsing_data/navigation_entry_remover.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/common/buildflags.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/search_engines/template_url_service.h"
 
 #if BUILDFLAG(ENABLE_SESSION_SERVICE)
 #include "chrome/browser/sessions/session_service_factory.h"
 #endif
 
+namespace {
+
+// This method takes in parameter |urls|, which is a map,
+// {Origin -> {Count, LastVisitTime}}, keyed by origin to the
+// count of its URLs in the history and its last visit time.
+// Here, we iterate over the map to return the set of origins
+// which doesn't have any of its urls present in the history.
+// A flat_set is returned for efficient lookups
+// in the Contains method below. The insertion
+// time is O(n.logn) as we invoke the move
+// constructor with std::vector.
+base::flat_set<GURL> GetDeletedOrigins(
+    const history::OriginCountAndLastVisitMap& urls) {
+  std::vector<GURL> deleted_origins;
+  // Iterating over the map {Origin -> {Count, LastVisitTime}}.
+  for (const auto& key_value : urls) {
+    // If the Count is greater than 0, it means few of the origin's URL(s) are
+    // still in the history, so we shouldn't mark the origin for deletion.
+    if (key_value.second.first > 0)
+      continue;
+    deleted_origins.push_back(key_value.first);
+  }
+  return base::flat_set<GURL>(std::move(deleted_origins));
+}
+
+bool Contains(const base::flat_set<GURL>& deleted_origins,
+              const GURL& template_gurl) {
+  return deleted_origins.contains(template_gurl.GetOrigin());
+}
+
+void DeleteTemplateUrlsForTimeRange(TemplateURLService* keywords_model,
+                                    base::Time delete_begin,
+                                    base::Time delete_end) {
+  if (!keywords_model->loaded()) {
+    keywords_model->RegisterOnLoadedCallback(base::AdaptCallbackForRepeating(
+        base::BindOnce(&DeleteTemplateUrlsForTimeRange, keywords_model,
+                       delete_begin, delete_end)));
+    keywords_model->Load();
+    return;
+  }
+
+  keywords_model->RemoveAutoGeneratedBetween(delete_begin, delete_end);
+}
+
+void DeleteTemplateUrlsForDeletedOrigins(TemplateURLService* keywords_model,
+                                         base::flat_set<GURL> deleted_origins) {
+  if (!keywords_model->loaded()) {
+    keywords_model->RegisterOnLoadedCallback(base::AdaptCallbackForRepeating(
+        base::BindOnce(&DeleteTemplateUrlsForDeletedOrigins, keywords_model,
+                       std::move(deleted_origins))));
+    keywords_model->Load();
+    return;
+  }
+
+  keywords_model->RemoveAutoGeneratedForUrlsBetween(
+      base::BindRepeating(&Contains, std::move(deleted_origins)),
+      base::Time::Min(), base::Time::Max());
+}
+
+void DeleteTemplateUrls(TemplateURLService* keywords_model,
+                        const history::DeletionInfo& deletion_info) {
+  if (deletion_info.time_range().IsValid()) {
+    DeleteTemplateUrlsForTimeRange(keywords_model,
+                                   deletion_info.time_range().begin(),
+                                   deletion_info.time_range().end());
+  } else {
+    auto deleted_origins =
+        GetDeletedOrigins(deletion_info.deleted_urls_origin_map());
+    DeleteTemplateUrlsForDeletedOrigins(keywords_model,
+                                        std::move(deleted_origins));
+  }
+}
+
+}  // namespace
+
 BrowsingDataHistoryObserverService::BrowsingDataHistoryObserverService(
     Profile* profile)
     : profile_(profile) {
@@ -31,6 +108,13 @@ void BrowsingDataHistoryObserverService::OnURLsDeleted(
     const history::DeletionInfo& deletion_info) {
   if (!deletion_info.is_from_expiration())
     browsing_data::RemoveNavigationEntries(profile_, deletion_info);
+
+  // Deleting Template URLs. This also handles expiration events.
+  TemplateURLService* keywords_model =
+      TemplateURLServiceFactory::GetForProfile(profile_);
+
+  if (keywords_model)
+    DeleteTemplateUrls(keywords_model, deletion_info);
 }
 
 // static

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -61,7 +61,6 @@
 #include "chrome/browser/previews/previews_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/ui/find_bar/find_bar_state.h"
 #include "chrome/browser/ui/find_bar/find_bar_state_factory.h"
@@ -97,7 +96,6 @@
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/prefs/pref_service.h"
 #include "components/previews/content/previews_ui_service.h"
-#include "components/search_engines/template_url_service.h"
 #include "components/web_cache/browser/web_cache_manager.h"
 #include "components/webrtc_logging/browser/log_cleanup.h"
 #include "components/webrtc_logging/browser/text_log_list.h"
@@ -307,7 +305,6 @@ ChromeBrowsingDataRemoverDelegate::~ChromeBrowsingDataRemoverDelegate() =
 
 void ChromeBrowsingDataRemoverDelegate::Shutdown() {
   history_task_tracker_.TryCancelAll();
-  template_url_sub_.reset();
 }
 
 content::BrowsingDataRemoverDelegate::EmbedderOriginTypeMatcher
@@ -479,24 +476,6 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
             filter_builder->BuildNetworkServiceFilter(),
             CreateTaskCompletionClosureForMojo(TracingDataType::kHostCache));
 
-    // As part of history deletion we also delete the auto-generated keywords.
-    TemplateURLService* keywords_model =
-        TemplateURLServiceFactory::GetForProfile(profile_);
-
-    if (keywords_model && !keywords_model->loaded()) {
-      // TODO(msramek): Store filters from the currently executed task on the
-      // object to avoid having to copy them to callback methods.
-      template_url_sub_ = keywords_model->RegisterOnLoadedCallback(
-          base::AdaptCallbackForRepeating(base::BindOnce(
-              &ChromeBrowsingDataRemoverDelegate::OnKeywordsLoaded,
-              weak_ptr_factory_.GetWeakPtr(), nullable_filter,
-              CreateTaskCompletionClosure(TracingDataType::kKeywordsModel))));
-      keywords_model->Load();
-    } else if (keywords_model) {
-      keywords_model->RemoveAutoGeneratedForUrlsBetween(
-          nullable_filter, delete_begin_, delete_end_);
-    }
-
     // The PrerenderManager keeps history of prerendered pages, so clear that.
     // It also may have a prerendered page. If so, the page could be
     // considered to have a small amount of historical information, so delete
@@ -1284,18 +1263,6 @@ void ChromeBrowsingDataRemoverDelegate::
   domain_reliability_clearer_ = std::move(clearer);
 }
 
-void ChromeBrowsingDataRemoverDelegate::OnKeywordsLoaded(
-    base::RepeatingCallback<bool(const GURL&)> url_filter,
-    base::OnceClosure done) {
-  // Deletes the entries from the model.
-  TemplateURLService* model =
-      TemplateURLServiceFactory::GetForProfile(profile_);
-  model->RemoveAutoGeneratedForUrlsBetween(url_filter, delete_begin_,
-                                           delete_end_);
-  template_url_sub_.reset();
-  std::move(done).Run();
-}
-
 bool ChromeBrowsingDataRemoverDelegate::IsForAllTime() const {
   return delete_begin_ == base::Time() && delete_end_ == base::Time::Max();
 }

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

@@ -19,7 +19,6 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/nacl/common/buildflags.h"
 #include "components/offline_pages/core/offline_page_model.h"
-#include "components/search_engines/template_url_service.h"
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/browser/browsing_data_remover_delegate.h"
 #include "extensions/buildflags/buildflags.h"
@@ -253,11 +252,6 @@ class ChromeBrowsingDataRemoverDelegate
   // Records unfinished tasks from |pending_sub_tasks_| after a delay.
   void RecordUnfinishedSubTasks();
 
-  // Callback for when TemplateURLService has finished loading. Clears the data,
-  // clears the respective waiting flag, and invokes NotifyIfDone.
-  void OnKeywordsLoaded(base::RepeatingCallback<bool(const GURL&)> url_filter,
-                        base::OnceClosure done);
-
   // A helper method that checks if time period is for "all time".
   bool IsForAllTime() const;
 
@@ -319,8 +313,6 @@ class ChromeBrowsingDataRemoverDelegate
   // Used if we need to clear history.
   base::CancelableTaskTracker history_task_tracker_;
 
-  std::unique_ptr<TemplateURLService::Subscription> template_url_sub_;
-
 #if defined(OS_ANDROID)
   // WebappRegistry makes calls across the JNI. In unit tests, the Java side is
   // not initialised, so the registry must be mocked out.

chrome/browser/history/history_browsertest.cc

@@ -17,17 +17,20 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_test_utils.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/search_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/common/pref_names.h"
 #include "components/prefs/pref_service.h"
+#include "components/search_engines/template_url_service.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -405,6 +408,57 @@ IN_PROC_BROWSER_TEST_F(HistoryBrowserTest, DownloadNoHistory) {
   ExpectEmptyHistory();
 }
 
+IN_PROC_BROWSER_TEST_F(HistoryBrowserTest, HistoryRemovalRemovesTemplateURL) {
+  constexpr char origin[] = "foo.com";
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url(embedded_test_server()->GetURL(origin, "/title3.html"));
+
+  // Creating keyword shortcut manually.
+  TemplateURLData data;
+  data.SetShortName(base::ASCIIToUTF16(origin));
+  data.SetKeyword(base::ASCIIToUTF16("keyword"));
+  data.SetURL(url.spec());
+  data.safe_for_autoreplace = true;
+
+  // Adding url to the history.
+  ui_test_utils::NavigateToURL(browser(), url);
+  WaitForHistoryBackendToRun(GetProfile());
+
+  EXPECT_TRUE(HistoryContainsURL(url));
+
+  // Adding the keyword in the template URL.
+  TemplateURLService* model =
+      TemplateURLServiceFactory::GetForProfile(browser()->profile());
+
+  // Waiting for the model to load.
+  search_test_utils::WaitForTemplateURLServiceToLoad(model);
+
+  TemplateURL* t_url = model->Add(std::make_unique<TemplateURL>(data));
+
+  EXPECT_EQ(t_url, model->GetTemplateURLForHost(origin));
+
+  auto* history_service = HistoryServiceFactory::GetForProfile(
+      browser()->profile(), ServiceAccessType::EXPLICIT_ACCESS);
+
+  history_service->DeleteURLs({url});
+
+  // The DeleteURL method runs an asynchronous task
+  // internally that deletes the data from db. The test
+  // must wait for the async delete to be finished in order to
+  // check if the delete was indeed successful. We emulate
+  // the wait by calling another method |FlushForTest|
+  // in the history service. Since, we know that that
+  // history processeses tasks synchronously, so when the
+  // callback is run for |FlushForTest| we know the deletion
+  // should have finished.
+  base::RunLoop run_loop;
+  history_service->FlushForTest(run_loop.QuitClosure());
+  run_loop.Run();
+
+  EXPECT_FALSE(model->GetTemplateURLForHost(origin));
+}
+
 namespace {
 
 // Grabs the RenderFrameHost for the frame navigating to the given URL.