[search_engines] Protect DSE database entry from Remove() during load

In M89, we introduced a regression where we didn't adequetely protect
the Default Search Provider from removal during keyword deduplication
while the model was still being loaded (loaded_ == false).

This CL fixes that and adds a regression test.

It also adds another CHECK during Remove() which just crashes Chrome if
some other code branch tries to delete the DSE.

Deleting the DSE causes permanent damage to the user's Profile, which
can be synced to other installations of Chrome. It's really bad, so
crashing is not bad in comparison.

Bug: 1164024
Change-Id: If1e70cc2a7b4e0c07b20b4e76acb243d7630937c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623120
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@{#842697}

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -37,11 +37,13 @@
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
 using base::Time;
 using base::TimeDelta;
+using testing::NotNull;
 
 namespace {
 
@@ -933,6 +935,38 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProviderLoadedFromPrefs) {
   AssertEquals(*cloned_url, *model()->GetDefaultSearchProvider());
 }
 
+TEST_F(TemplateURLServiceTest,
+       DefaultSearchProviderShouldBeProtectedFromKeywordConflictDuringLoad) {
+  // Start with the model unloaded, with the DSE provided purely from prefs.
+  ASSERT_FALSE(model()->loaded());
+  const TemplateURL* initial_default_search_provider =
+      model()->GetDefaultSearchProvider();
+  ASSERT_THAT(initial_default_search_provider, NotNull());
+
+  // Now simulate loading from the keyword table, where the DSE is added as a
+  // a TemplateURL to the vector.
+  TemplateURL* in_vector_dse_engine = model()->Add(
+      std::make_unique<TemplateURL>(initial_default_search_provider->data()));
+  ASSERT_THAT(in_vector_dse_engine, NotNull());
+  ASSERT_EQ(in_vector_dse_engine,
+            model()->GetTemplateURLForGUID(
+                initial_default_search_provider->sync_guid()));
+
+  // Then simulate loading a conflicting user engine with the same keyword.
+  TemplateURL* user_engine = AddKeywordWithDate(
+      "user_engine",
+      base::UTF16ToUTF8(initial_default_search_provider->keyword()),
+      "http://test2", std::string(), std::string(), std::string(), false,
+      "UTF-8", base::Time::FromTimeT(20));
+  EXPECT_THAT(user_engine, NotNull());
+
+  // Now verify that the in-vector DSE entry was not removed due to the keyword
+  // conflict. It should be protected by virtue of matching the initial DSE.
+  EXPECT_EQ(in_vector_dse_engine,
+            model()->GetTemplateURLForGUID(
+                initial_default_search_provider->sync_guid()));
+}
+
 TEST_F(TemplateURLServiceTest, RepairPrepopulatedSearchEngines) {
   test_util()->VerifyLoad();
 

components/search_engines/template_url_service.cc

@@ -480,7 +480,25 @@ TemplateURL* TemplateURLService::AddWithOverrides(
 }
 
 void TemplateURLService::Remove(const TemplateURL* template_url) {
-  DCHECK_NE(template_url, default_search_provider_);
+  // CHECK that we aren't trying to Remove() the default search provider.
+  // This has happened before, and causes permanent damage to the user Profile,
+  // which can then be Synced to other installations. It's better to crash
+  // immediately, and that's why this isn't a DCHECK. https://crbug.com/1164024
+  {
+    const TemplateURL* default_provider = GetDefaultSearchProvider();
+
+    // TODO(tommycli): Once we are sure this never happens in practice, we can
+    // remove this CrashKeyString, but we should keep the CHECK.
+    static base::debug::CrashKeyString* crash_key =
+        base::debug::AllocateCrashKeyString("removed_turl_keyword",
+                                            base::debug::CrashKeySize::Size256);
+    base::debug::ScopedCrashKeyString auto_clear(
+        crash_key, base::UTF16ToUTF8(template_url->keyword()));
+
+    CHECK_NE(template_url, default_provider);
+    if (default_provider)
+      CHECK_NE(template_url->sync_guid(), default_provider->sync_guid());
+  }
 
   auto i = FindTemplateURL(&template_urls_, template_url);
   if (i == template_urls_.end())
@@ -2175,6 +2193,14 @@ 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);
@@ -2182,7 +2208,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 != default_search_provider_) {
+    if (turl != best && turl->sync_guid() != default_provider_guid) {
       Remove(turl);
     }
   }

components/search_engines/template_url_service.h

@@ -730,6 +730,11 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Once loaded, the default search provider.  This is a pointer to a
   // TemplateURL owned by |template_urls_|.
+  //
+  // TODO(tommycli): Can we combine this with initial_default_search_provider_?
+  // Essentially all direct usages of this variable need to first check that
+  // |loading_| is true, and should call GetDefaultSearchProvider() instead.
+  // Example of a regression due to this mistake: https://crbug.com/1164024.
   TemplateURL* default_search_provider_ = nullptr;
 
   // A temporary location for the DSE until Web Data has been loaded and it can