[search_engines] Stop OpenSearch from deleting Default Search Engine

Currently, when Chrome adds a new engine from the OpenSearch
description file, it can delete the Default Search Engine if it is
is safe_for_autoreplace() and not prepopulated.

This is an edge case, but there are crash reports of this triggering
our CHECK. From what I can tell, this issue has existed for a long time,
but was never caught because we had a DCHECK instead of a CHECK.

This CL does two things:

 1. Prevents that happening by no longer returning an |existing_engine|
    from the CanAddAutogeneratedKeyword() method.

    Since Add() can now automatically do ranking and deduplication,
    there's no need for the caller to manually Remove() engines like
    before.

 2. Makes OpenSearch outrank form-search autogenerated engines
    in TemplateURL::IsBetterThanEngineWithConflictingKeyword().

    We always treated it this way, but this formalizes it in our
    sorting function, which helps us build our total ordering of
    TemplateURLs.

In summary: This CL makes the auto-generated engine adding logic
reuse our existing deduplication logic, rather than relying on its own
custom logic, which didn't respect the default search provider.

This is meant to be merged into M89.

Bug: 1166372
Change-Id: I410a0f8870f65cd16001f5ebd120ccbf04aadd93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2644936Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846299}

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -346,8 +346,8 @@ TEST_F(TemplateURLServiceTest, AddUpdateRemove) {
   data.last_visited = Time::FromTimeT(100);
   data.sync_guid = "00000000-0000-0000-0000-000000000001";
   TemplateURL* t_url = model()->Add(std::make_unique<TemplateURL>(data));
-  ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"),
-                                                  GURL(), NULL));
+  ASSERT_TRUE(
+      model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"), GURL()));
   VerifyObserverCount(1);
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
@@ -365,8 +365,8 @@ TEST_F(TemplateURLServiceTest, AddUpdateRemove) {
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"));
   ASSERT_TRUE(loaded_url != NULL);
   AssertEquals(*cloned_url, *loaded_url);
-  ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"),
-                                                  GURL(), NULL));
+  ASSERT_TRUE(
+      model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"), GURL()));
 
   // We expect the last_modified time to be updated to the present time on an
   // explicit reset.
@@ -382,10 +382,9 @@ TEST_F(TemplateURLServiceTest, AddUpdateRemove) {
   ASSERT_EQ(ASCIIToUTF16("b"), loaded_url->keyword());
   ASSERT_EQ("c", loaded_url->url());
   ASSERT_FALSE(loaded_url->safe_for_autoreplace());
-  ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"),
-                                                  GURL(), NULL));
-  ASSERT_FALSE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("b"), GURL(),
-                                                   NULL));
+  ASSERT_TRUE(
+      model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("keyword"), GURL()));
+  ASSERT_FALSE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("b"), GURL()));
   cloned_url.reset(new TemplateURL(loaded_url->data()));
   base::RunLoop().RunUntilIdle();
   test_util()->ResetModel(true);
@@ -853,15 +852,14 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProvider) {
 
 TEST_F(TemplateURLServiceTest, CantReplaceWithSameKeyword) {
   test_util()->ChangeModelToLoadState();
-  ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("foo"), GURL(),
-                                                  NULL));
+  ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("foo"), GURL()));
   TemplateURL* t_url =
       AddKeywordWithDate("name1", "foo", "http://foo1", "http://sugg1",
                          std::string(), "http://icon1", true, "UTF-8;UTF-16");
 
   // Can still replace, newly added template url is marked safe to replace.
   ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("foo"),
-                                         GURL("http://foo2"), NULL));
+                                                  GURL("http://foo2")));
 
   // ResetTemplateURL marks the TemplateURL as unsafe to replace, so it should
   // no longer be replaceable.
@@ -869,20 +867,20 @@ TEST_F(TemplateURLServiceTest, CantReplaceWithSameKeyword) {
                             t_url->url());
 
   ASSERT_FALSE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("foo"),
-                                          GURL("http://foo2"), NULL));
+                                                   GURL("http://foo2")));
 }
 
 TEST_F(TemplateURLServiceTest, CantReplaceWithSameHosts) {
   test_util()->ChangeModelToLoadState();
   ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("foo"),
-                                         GURL("http://foo.com"), NULL));
+                                                  GURL("http://foo.com")));
   TemplateURL* t_url =
       AddKeywordWithDate("name1", "foo", "http://foo.com", "http://sugg1",
                          std::string(), "http://icon1", true, "UTF-8;UTF-16");
 
   // Can still replace, newly added template url is marked safe to replace.
   ASSERT_TRUE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("bar"),
-                                         GURL("http://foo.com"), NULL));
+                                                  GURL("http://foo.com")));
 
   // ResetTemplateURL marks the TemplateURL as unsafe to replace, so it should
   // no longer be replaceable.
@@ -890,7 +888,7 @@ TEST_F(TemplateURLServiceTest, CantReplaceWithSameHosts) {
                             t_url->url());
 
   ASSERT_FALSE(model()->CanAddAutogeneratedKeyword(ASCIIToUTF16("bar"),
-                                          GURL("http://foo.com"), NULL));
+                                                   GURL("http://foo.com")));
 }
 
 TEST_F(TemplateURLServiceTest, HasDefaultSearchProvider) {

chrome/browser/ui/search_engines/search_engine_tab_helper.cc

@@ -199,17 +199,10 @@ void SearchEngineTabHelper::GenerateKeywordIfNecessary(
     return;
   }
 
-  const TemplateURL* current_url;
   GURL url = handle->GetSearchableFormURL();
-  if (!url_service->CanAddAutogeneratedKeyword(keyword, url, &current_url))
+  if (!url_service->CanAddAutogeneratedKeyword(keyword, url))
     return;
 
-  if (current_url && current_url->originating_url().is_valid()) {
-    // The existing keyword was generated from an OpenSearch description
-    // document, don't regenerate.
-    return;
-  }
-
   TemplateURLData data;
   data.SetShortName(keyword);
   data.SetKeyword(keyword);
@@ -231,8 +224,9 @@ void SearchEngineTabHelper::GenerateKeywordIfNecessary(
   data.safe_for_autoreplace = true;
   data.input_encodings.push_back(handle->GetSearchableFormEncoding());
 
-  // This Add() call may displace the previously auto-generated TemplateURL,
-  // and that's expected. But it will never displace the default search engine.
+  // This Add() call may displace the previously auto-generated TemplateURL.
+  // But it will never displace the Default Search Engine, nor will it displace
+  // any OpenSearch document derived engines, which outrank this one.
   url_service->Add(std::make_unique<TemplateURL>(data));
 }
 

components/search_engines/template_url.cc

@@ -1367,6 +1367,9 @@ bool TemplateURL::IsBetterThanEngineWithConflictingKeyword(
         engine->created_from_play_api(),
         // Favor prepopulated engines over other auto-generated engines.
         engine->prepopulate_id() > 0,
+        // Favor engines derived from OpenSearch descriptions over
+        // autogenerated engines heuristically generated from searchable forms.
+        engine->originating_url().is_valid(),
         // More recently modified engines or created engines win.
         engine->last_modified(), engine->date_created(),
         // TODO(tommycli): This should be a tie-breaker than provides a total

components/search_engines/template_url_fetcher.cc

@@ -195,16 +195,12 @@ void TemplateURLFetcher::RequestDelegate::AddSearchProvider() {
   DCHECK(model);
   DCHECK(model->loaded());
 
-  const TemplateURL* existing_url = nullptr;
-  if (!model->CanAddAutogeneratedKeyword(keyword_, GURL(template_url_->url()),
-                                         &existing_url)) {
+  if (!model->CanAddAutogeneratedKeyword(keyword_,
+                                         GURL(template_url_->url()))) {
     fetcher_->RequestCompleted(this);  // WARNING: Deletes us!
     return;
   }
 
-  if (existing_url)
-    model->Remove(existing_url);
-
   // The short name is what is shown to the user. We preserve original names
   // since it is better when generated keyword in many cases.
   TemplateURLData data(template_url_->data());
@@ -216,6 +212,7 @@ void TemplateURLFetcher::RequestDelegate::AddSearchProvider() {
     data.favicon_url = favicon_url_;
 
   // Mark the keyword as replaceable so it can be removed if necessary.
+  // Add() will automatically remove conflicting keyword replaceable engines.
   data.safe_for_autoreplace = true;
   model->Add(std::make_unique<TemplateURL>(data));
 

components/search_engines/template_url_service.cc

@@ -341,14 +341,11 @@ base::android::ScopedJavaLocalRef<jobject> TemplateURLService::GetJavaObject() {
 
 bool TemplateURLService::CanAddAutogeneratedKeyword(
     const base::string16& keyword,
-    const GURL& url,
-    const TemplateURL** template_url_to_replace) {
+    const GURL& url) {
   DCHECK(!keyword.empty());  // This should only be called for non-empty
                              // keywords. If we need to support empty kewords
                              // the code needs to change slightly.
   const TemplateURL* existing_url = GetTemplateURLForKeyword(keyword);
-  if (template_url_to_replace)
-    *template_url_to_replace = existing_url;
   if (existing_url) {
     // TODO(tommycli): Currently, this code goes one step beyond
     // safe_for_autoreplace() and also forbids automatically modifying

components/search_engines/template_url_service.h

@@ -138,15 +138,12 @@ class TemplateURLService : public WebDataServiceConsumer,
 #endif
 
   // Returns true if there is no TemplateURL that conflicts with the
-  // keyword/url pair, or there is one but it can be replaced. If there is an
-  // existing keyword that can be replaced and template_url_to_replace is
-  // non-NULL, template_url_to_replace is set to the keyword to replace.
+  // keyword/url pair, or there is one but it can be replaced.
   //
   // |url| is the URL of the search query.  This is used to prevent auto-adding
   // a keyword for hosts already associated with a manually-edited keyword.
   bool CanAddAutogeneratedKeyword(const base::string16& keyword,
-                                  const GURL& url,
-                                  const TemplateURL** template_url_to_replace);
+                                  const GURL& url);
 
   // Returns whether the engine is a "pre-existing" engine, either from the
   // prepopulate list or created by policy.

ios/chrome/browser/search_engines/search_engine_tab_helper.mm

@@ -249,21 +249,10 @@ void SearchEngineTabHelper::AddTemplateURLBySearchableURL(
     return;
   }
 
-  const TemplateURL* existing_url;
-  if (!url_service->CanAddAutogeneratedKeyword(keyword, searchable_url,
-                                               &existing_url)) {
+  if (!url_service->CanAddAutogeneratedKeyword(keyword, searchable_url)) {
     return;
   }
 
-  if (existing_url) {
-    if (existing_url->originating_url().is_valid()) {
-      // The existing keyword was generated from an OpenSearch description
-      // document, don't regenerate.
-      return;
-    }
-    url_service->Remove(existing_url);
-  }
-
   TemplateURLData data;
   data.SetShortName(keyword);
   data.SetKeyword(keyword);
@@ -282,6 +271,10 @@ void SearchEngineTabHelper::AddTemplateURLBySearchableURL(
     data.favicon_url = TemplateURL::GenerateFaviconURL(previous_item->GetURL());
   }
   data.safe_for_autoreplace = true;
+
+  // This Add() call may displace the previously auto-generated TemplateURL.
+  // But it will never displace the Default Search Engine, nor will it displace
+  // any OpenSearch document derived engines, which outrank this one.
   url_service->Add(std::make_unique<TemplateURL>(data));
 }