[search_engines] Stop Extensions from duplicating TemplateURL Sync GUIDs

Extensions that use the Override Settings API to create new search
engines matching prepopulated engines currently erreneously duplicate
the Sync GUID of the prepopulated engines.

That violates an invariant of TemplateURLService - that each engine
have a unique Sync GUID.

It also caused CHECK crashes, when it seemed like we were trying to
remove an engine that matched the GUID of the DSE, yet it didn't match
the pointer value of the DSE.

This CL also relaxes the CHECK, because now we have to be tolerant of
users with a strange database state of multiple engines having the same
Sync GUID.

It seems like this bug has been around for years without causing harm
though, so it's probably mostly harmless except for our CHECK.

Bug: 1166372
Change-Id: I4c0e0729a3b3f27f29615b782532b20dcd4dc996
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2636683
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845330}

chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc

@@ -52,7 +52,18 @@ std::unique_ptr<TemplateURLData> ConvertSearchProvider(
   if (search_provider.prepopulated_id) {
     data = TemplateURLPrepopulateData::GetPrepopulatedEngine(
         prefs, *search_provider.prepopulated_id);
-    if (!data) {
+    if (data) {
+      // We need to override the prepopulate_id and Sync GUID of the generated
+      // engine; otherwise, we will collide the original and also clone the
+      // Sync GUID of the original. See https://crbug.com/1166372#c13
+      //
+      // Note that prepopulate_id must be set first, since GenerateSyncGUID()
+      // internally depends on it.
+      std::string old_sync_guid = data->sync_guid;
+      data->prepopulate_id = 0;
+      data->GenerateSyncGUID();
+      DCHECK_NE(data->sync_guid, old_sync_guid);
+    } else {
       VLOG(1) << "Settings Overrides API can't recognize prepopulated_id="
           << *search_provider.prepopulated_id;
     }
@@ -61,6 +72,10 @@ std::unique_ptr<TemplateURLData> ConvertSearchProvider(
   if (!data)
     data = std::make_unique<TemplateURLData>();
 
+  // `prepopulate_id` must be 0 to avoid collisions with prepopulated
+  // engines.
+  DCHECK_EQ(0, data->prepopulate_id);
+
   if (search_provider.name)
     data->SetShortName(base::UTF8ToUTF16(*search_provider.name));
   if (search_provider.keyword)
@@ -93,7 +108,6 @@ std::unique_ptr<TemplateURLData> ConvertSearchProvider(
   }
   data->date_created = base::Time();
   data->last_modified = base::Time();
-  data->prepopulate_id = 0;
   if (search_provider.alternate_urls) {
     data->alternate_urls.clear();
     for (size_t i = 0; i < search_provider.alternate_urls->size(); ++i) {

components/search_engines/template_url_service.cc

@@ -496,8 +496,18 @@ void TemplateURLService::Remove(const TemplateURL* template_url) {
         crash_key, base::UTF16ToUTF8(template_url->keyword()));
 
     CHECK_NE(template_url, default_provider);
-    if (default_provider)
+
+    // For Extensions that use Override Settings API, there was a bug that
+    // caused extension engines to duplicate the Sync GUID of prepopulated
+    // engines. Since users still have those duplicated GUIDs in the wild,
+    // we skip the check for extensions. https://crbug.com/1166372#c13
+    if (default_provider &&
+        default_provider->type() !=
+            TemplateURL::Type::NORMAL_CONTROLLED_BY_EXTENSION &&
+        template_url->type() !=
+            TemplateURL::Type::NORMAL_CONTROLLED_BY_EXTENSION) {
       CHECK_NE(template_url->sync_guid(), default_provider->sync_guid());
+    }
   }
 
   auto i = FindTemplateURL(&template_urls_, template_url);