[search_engines] Protect DSE from removal when the user clears History

Some users have their DSE set to an autogenerated TemplateURL created
as the user browses (via OpenSearch).

Those engines are automatically deleted when the corresponding History
entry is deleted.

This code protects those DSEs from removal. This was part of the
CanReplace() clause in M88 and before, so we regressed this in M89.

This CL fixes the regression and adds a test to prevent future
regression.

BUG=1166372

Change-Id: I448d053b7dcfc38343f2d82e5c86df8c5e168d66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628045
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843400}

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -581,13 +581,18 @@ TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) {
   AddKeywordWithDate("name4", "key4", "http://foo4", std::string(),
                      std::string(), std::string(), true, std::string(),
                      now + one_day, Time(), Time());
-  // Try the other three states.
-  AddKeywordWithDate("name5", "key5", "http://foo5", "http://suggest5",
-                     std::string(), "http://icon5", false, "UTF-8;UTF-16", now,
-                     Time(), Time());
-  AddKeywordWithDate("name6", "key6", "http://foo6", "http://suggest6",
-                     std::string(), "http://icon6", false, "UTF-8;UTF-16",
-                     month_ago, Time(), Time());
+  // Add a non-replaceable engine, to verify we don't never remove those.
+  AddKeywordWithDate("user_engine_name", "user_engine_key", "http://foo5",
+                     "http://suggest5", std::string(), "http://icon5", false,
+                     "UTF-8;UTF-16", now, Time(), Time());
+  // Also add a replaceable engine that's marked as the Default Search Engine.
+  // We also need to verify we never remove those. https://crbug.com/1166372
+  TemplateURL* replaceable_dse = AddKeywordWithDate(
+      "replaceable_dse_name", "replaceable_dse_key", "http://foo6",
+      "http://suggest6", std::string(), "http://icon6", true, "UTF-8;UTF-16",
+      month_ago, Time(), Time());
+  ASSERT_THAT(replaceable_dse, NotNull());
+  model()->SetUserSelectedDefaultSearchProvider(replaceable_dse);
 
   // We just added a few items, validate them.
   EXPECT_EQ(6U, model()->GetTemplateURLs().size());
@@ -608,13 +613,15 @@ TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) {
   EXPECT_EQ(0U,
             model()->GetTemplateURLs()[0]->date_created().ToInternalValue());
 
-  EXPECT_EQ(ASCIIToUTF16("key5"), model()->GetTemplateURLs()[1]->keyword());
+  EXPECT_EQ(ASCIIToUTF16("user_engine_key"),
+            model()->GetTemplateURLs()[1]->keyword());
   EXPECT_FALSE(model()->GetTemplateURLs()[1]->safe_for_autoreplace());
   EXPECT_EQ(now.ToInternalValue(),
             model()->GetTemplateURLs()[1]->date_created().ToInternalValue());
 
-  EXPECT_EQ(ASCIIToUTF16("key6"), model()->GetTemplateURLs()[2]->keyword());
-  EXPECT_FALSE(model()->GetTemplateURLs()[2]->safe_for_autoreplace());
+  EXPECT_EQ(ASCIIToUTF16("replaceable_dse_key"),
+            model()->GetTemplateURLs()[2]->keyword());
+  EXPECT_TRUE(model()->GetTemplateURLs()[2]->safe_for_autoreplace());
   EXPECT_EQ(month_ago.ToInternalValue(),
             model()->GetTemplateURLs()[2]->date_created().ToInternalValue());
 

components/search_engines/template_url_service.cc

@@ -571,6 +571,7 @@ void TemplateURLService::RemoveAutoGeneratedForUrlsBetween(
     if (turl->date_created() >= created_after &&
         (created_before.is_null() || turl->date_created() < created_before) &&
         turl->safe_for_autoreplace() && turl->prepopulate_id() == 0 &&
+        !MatchesDefaultSearchProvider(turl) &&
         (url_filter.is_null() ||
          url_filter.Run(turl->GenerateSearchURL(search_terms_data())))) {
       Remove(turl);
@@ -2201,14 +2202,6 @@ bool TemplateURLService::RemoveDuplicateReplaceableEnginesOf(
     best = candidate;
   }
 
-  // Use a GUID comparison rather than a pointer comparison, because while the
-  // model is being loaded, the DSE may be sourced from prefs, and we still want
-  // to protect the corresponding database entry from deletion.
-  // https://crbug.com/1164024
-  const TemplateURL* default_provider = GetDefaultSearchProvider();
-  std::string default_provider_guid =
-      default_provider ? default_provider->sync_guid() : "";
-
   // Remove all the replaceable TemplateURLs that are not the best.
   for (TemplateURL* turl : replaceable_turls) {
     DCHECK_NE(turl, candidate);
@@ -2216,7 +2209,7 @@ bool TemplateURLService::RemoveDuplicateReplaceableEnginesOf(
     // Never actually remove the DSE during this phase. This handling defers
     // deleting the DSE until it's no longer set as the DSE, analagous to how
     // we handle ACTION_DELETE of the DSE in ProcessSyncChanges().
-    if (turl != best && turl->sync_guid() != default_provider_guid) {
+    if (turl != best && !MatchesDefaultSearchProvider(turl)) {
       Remove(turl);
     }
   }
@@ -2250,3 +2243,12 @@ bool TemplateURLService::RemoveDuplicateReplaceableEnginesOf(
   return candidate != best && candidate->safe_for_autoreplace() &&
          candidate->prepopulate_id() == 0;
 }
+
+bool TemplateURLService::MatchesDefaultSearchProvider(TemplateURL* turl) const {
+  DCHECK(turl);
+  const TemplateURL* default_provider = GetDefaultSearchProvider();
+  if (!default_provider)
+    return false;
+
+  return turl->sync_guid() == default_provider->sync_guid();
+}

components/search_engines/template_url_service.h

@@ -679,6 +679,12 @@ class TemplateURLService : public WebDataServiceConsumer,
   // correct handling to its caller.
   bool RemoveDuplicateReplaceableEnginesOf(TemplateURL* candidate);
 
+  // Returns true if |turl| matches the default search provider. This method
+  // does both a GUID comparison, because while the model is being loaded, the
+  // DSE may be sourced from prefs, and we still want to consider the
+  // corresponding database entry a match. https://crbug.com/1164024
+  bool MatchesDefaultSearchProvider(TemplateURL* turl) const;
+
   // ---------- Browser state related members ---------------------------------
   PrefService* prefs_ = nullptr;