From 1e5b217d538e3532adc4fd8e4613f619bcfb1385 Mon Sep 17 00:00:00 2001
From: Amelie Schneider <[email protected]>
Date: Tue, 22 Sep 2026 03:19:27 -0700
Subject: [PATCH] [Search Engines] Move TemplateURL ID lookup to TemplateURLService

KeywordEditorController maintained a local cache of TemplateURL pointers
mapped by their ID. This cache could become stale if the controller's
associated observer (SearchEnginesHandler) stopped observing the
TemplateURLService while the controller still existed. A subsequent
message from a WebUI renderer could then trigger a use-after-free by
accessing a freed pointer from this stale map.

To fix this, move the ID-to-TURL mapping into TemplateURLService itself.
The service, as the owner of these objects, is the source of truth and
can keep its map up-to-date as engines are added or removed.
KeywordEditorController now delegates ID lookups to the service.

This eliminates the redundant and potentially dangerous cache in the
controller.

Bug: 513226023
Change-Id: If6a6895ef83e7f956bf9442458c1cbc690d34a36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8425826
Commit-Queue: Amelie Schneider <[email protected]>
Reviewed-by: Nicolas Dossou-Gbété <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1702465}
---
diff --git a/chrome/browser/ui/search_engines/keyword_editor_controller.cc b/chrome/browser/ui/search_engines/keyword_editor_controller.cc
index c360b2972..9ec99c1 100644
--- a/chrome/browser/ui/search_engines/keyword_editor_controller.cc
+++ b/chrome/browser/ui/search_engines/keyword_editor_controller.cc
@@ -51,8 +51,8 @@
 }  // namespace
 
 KeywordEditorController::KeywordEditorController(Profile* profile)
-    : url_model_(TemplateURLServiceFactory::GetForProfile(profile)) {
-  url_model_->Load();
+    : template_url_service_(TemplateURLServiceFactory::GetForProfile(profile)) {
+  template_url_service_->Load();
 
   if (!base::FeatureList::IsEnabled(switches::kSearchSettingsUpdate)) {
     bool ai_mode_enabled = OmniboxFieldTrial::IsAimStarterPackEnabled(
@@ -62,7 +62,7 @@
         profile->GetPrefs()->GetInteger(
             optimization_guide::prefs::kGeminiSettings) == 0;
     table_model_ = std::make_unique<TemplateURLTableModel>(
-        url_model_,
+        template_url_service_,
         internal::GetDisabledStarterPackIds(ai_mode_enabled, gemini_enabled));
   }
 }
@@ -83,7 +83,7 @@
   data.SetURL(fixed_up_url);
   data.is_active = TemplateURLData::ActiveStatus::kTrue;
   TemplateURL* template_url =
-      url_model_->Add(std::make_unique<TemplateURL>(data));
+      template_url_service_->Add(std::make_unique<TemplateURL>(data));
   return template_url->id();
 }
 
@@ -94,34 +94,36 @@
     const std::string& fixed_up_url) {
   CHECK(!fixed_up_url.empty());
 
-  if (!ShouldUpdateTemplateURL(url_model_, template_url, title, keyword,
-                               fixed_up_url)) {
+  if (!ShouldUpdateTemplateURL(template_url_service_, template_url, title,
+                               keyword, fixed_up_url)) {
     return;
   }
 
   // The default search provider should support replacement.
-  CHECK(url_model_->GetDefaultSearchProvider() != template_url ||
-        template_url->SupportsReplacement(url_model_->search_terms_data()));
-  url_model_->ResetTemplateURL(template_url, title, keyword, fixed_up_url);
+  CHECK(template_url_service_->GetDefaultSearchProvider() != template_url ||
+        template_url->SupportsReplacement(
+            template_url_service_->search_terms_data()));
+  template_url_service_->ResetTemplateURL(template_url, title, keyword,
+                                          fixed_up_url);
 
   base::RecordAction(UserMetricsAction("KeywordEditor_ModifiedKeyword"));
 }
 
 bool KeywordEditorController::CanEdit(const TemplateURL* url) const {
   return (url->type() == TemplateURL::NORMAL) &&
-         (url != url_model_->GetDefaultSearchProvider() ||
-          !url_model_->is_default_search_managed()) &&
+         (url != template_url_service_->GetDefaultSearchProvider() ||
+          !template_url_service_->is_default_search_managed()) &&
          (!url->CreatedByNonDefaultSearchProviderPolicy() ||
           (url->CanPolicyBeOverridden() && !url->featured_by_policy()));
 }
 
 bool KeywordEditorController::CanMakeDefault(const TemplateURL* url) const {
-  return url_model_->CanMakeDefault(url);
+  return template_url_service_->CanMakeDefault(url);
 }
 
 bool KeywordEditorController::CanRemove(const TemplateURL* url) const {
   return (url->type() == TemplateURL::NORMAL) &&
-         (url != url_model_->GetDefaultSearchProvider()) &&
+         (url != template_url_service_->GetDefaultSearchProvider()) &&
          (url->starter_pack_id() ==
           template_url_starter_pack_data::StarterPackId::kNone) &&
          (!url->CreatedByNonDefaultSearchProviderPolicy() ||
@@ -135,7 +137,7 @@
 
 bool KeywordEditorController::CanDeactivate(const TemplateURL* url) const {
   return url->is_active() == TemplateURLData::ActiveStatus::kTrue &&
-         url != url_model_->GetDefaultSearchProvider() &&
+         url != template_url_service_->GetDefaultSearchProvider() &&
          !IsPrepopulatedEngine(url) &&
          (!url->CreatedByNonDefaultSearchProviderPolicy() ||
           url->CanPolicyBeOverridden());
@@ -158,24 +160,25 @@
     return;
   }
 
-  url_model_->Remove(template_url);
+  template_url_service_->Remove(template_url);
   base::RecordAction(UserMetricsAction("KeywordEditor_RemoveKeyword"));
 }
 
 const TemplateURL* KeywordEditorController::GetDefaultSearchProvider() {
-  return url_model_->GetDefaultSearchProvider();
+  return template_url_service_->GetDefaultSearchProvider();
 }
 
 void KeywordEditorController::MakeDefaultTemplateURL(
     TemplateURLID id,
     search_engines::ChoiceMadeLocation choice_location) {
   TemplateURL* template_url = GetTemplateURL(id);
-  if (!template_url || template_url == url_model_->GetDefaultSearchProvider()) {
+  if (!template_url ||
+      template_url == template_url_service_->GetDefaultSearchProvider()) {
     return;
   }
 
-  url_model_->SetUserSelectedDefaultSearchProvider(template_url,
-                                                   choice_location);
+  template_url_service_->SetUserSelectedDefaultSearchProvider(template_url,
+                                                              choice_location);
 }
 
 void KeywordEditorController::SetIsActiveTemplateURL(TemplateURLID id,
@@ -185,16 +188,15 @@
     return;
   }
 
-  url_model_->SetIsActiveTemplateURL(template_url, is_active);
+  template_url_service_->SetIsActiveTemplateURL(template_url, is_active);
 }
 
 bool KeywordEditorController::loaded() const {
-  return url_model_->loaded();
+  return template_url_service_->loaded();
 }
 
 TemplateURL* KeywordEditorController::GetTemplateURL(TemplateURLID id) {
-  auto it = id_to_turl_.find(id);
-  return it == id_to_turl_.end() ? nullptr : it->second;
+  return template_url_service_->GetTemplateURLForId(id);
 }
 
 TemplateURL* KeywordEditorController::GetTemplateURLForIndex(int index) {
@@ -206,12 +208,4 @@
   if (!base::FeatureList::IsEnabled(switches::kSearchSettingsUpdate)) {
     table_model_->Reload();
   }
-
-  TemplateURLService::TemplateURLVector urls = url_model_->GetTemplateURLs();
-  id_to_turl_.clear();
-  id_to_turl_.reserve(urls.size());
-
-  for (const auto& url : url_model_->GetTemplateURLs()) {
-    id_to_turl_[url->id()] = url;
-  }
 }
diff --git a/chrome/browser/ui/search_engines/keyword_editor_controller.h b/chrome/browser/ui/search_engines/keyword_editor_controller.h
index 1b422a94..b227ca57 100644
--- a/chrome/browser/ui/search_engines/keyword_editor_controller.h
+++ b/chrome/browser/ui/search_engines/keyword_editor_controller.h
@@ -86,7 +86,7 @@
   // deactivates it if false.
   void SetIsActiveTemplateURL(TemplateURLID id, bool is_active);
 
-  // Return true if the |url_model_| data is loaded.
+  // Return true if the |template_url_service_| data is loaded.
   bool loaded() const;
 
   // Return the TemplateURL corresponding to the `id`. Returns
@@ -101,10 +101,7 @@
   void Refresh();
 
  private:
-  raw_ptr<TemplateURLService> url_model_;
-
-  // Mapping from IDs to the TemplateURL.
-  absl::flat_hash_map<TemplateURLID, TemplateURL*> id_to_turl_;
+  raw_ptr<TemplateURLService> template_url_service_;
 
   // Model for the TableView.
   std::unique_ptr<TemplateURLTableModel> table_model_;
diff --git a/components/search_engines/template_url_service.cc b/components/search_engines/template_url_service.cc
index afb1bb3c..9d2350d 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -547,6 +547,16 @@
         host, &search_terms_data));
   }
 
+  // Returns the best `TemplateURL` found with a URL with the specified `id`,
+  // or nullptr if no such URL can be found.
+  const TemplateURL* GetTemplateURLForId(TemplateURLID id) const {
+    return GetTemplateURLForSelector(base::BindRepeating(
+        [](TemplateURLID id, const TemplateURL& turl) {
+          return turl.id() == id;
+        },
+        id));
+  }
+
  private:
   // Returns a pointer to a `TemplateURL` `t` such that `selector(t) == true`.
   // Prioritizes DSP.
@@ -822,6 +832,21 @@
                        host, search_terms_data());
 }
 
+TemplateURL* TemplateURLService::GetTemplateURLForId(TemplateURLID id) {
+  return const_cast<TemplateURL*>(
+      static_cast<const TemplateURLService*>(this)->GetTemplateURLForId(id));
+}
+
+const TemplateURL* TemplateURLService::GetTemplateURLForId(
+    TemplateURLID id) const {
+  auto elem(id_to_turl_.find(id));
+  if (elem != id_to_turl_.end()) {
+    return elem->second;
+  }
+
+  return loaded_ ? nullptr : pre_loading_providers_->GetTemplateURLForId(id);
+}
+
 TemplateURL* TemplateURLService::Add(
     std::unique_ptr<TemplateURL> template_url) {
   DCHECK(template_url);
@@ -2752,6 +2777,12 @@
     }
   }
 
+  // Remove from `id_to_turl_`.
+  auto it = id_to_turl_.find(template_url->id());
+  if (it != id_to_turl_.end() && it->second == template_url) {
+    id_to_turl_.erase(it);
+  }
+
   if (template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION) {
     return;
   }
@@ -2778,6 +2809,7 @@
 void TemplateURLService::AddToMaps(TemplateURL* template_url) {
   const std::u16string& keyword = template_url->keyword();
   keyword_to_turl_.insert(std::make_pair(keyword, template_url));
+  id_to_turl_.insert(std::make_pair(template_url->id(), template_url));
 
   if (template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION) {
     return;
diff --git a/components/search_engines/template_url_service.h b/components/search_engines/template_url_service.h
index e423785..e0287f45 100644
--- a/components/search_engines/template_url_service.h
+++ b/components/search_engines/template_url_service.h
@@ -277,6 +277,11 @@
   TemplateURL* GetTemplateURLForHost(const std::string& host);
   const TemplateURL* GetTemplateURLForHost(const std::string& host) const;
 
+  // Looks up `id` and returns the best `TemplateURL` for it. Returns
+  // `nullptr` if the id was not found.
+  TemplateURL* GetTemplateURLForId(TemplateURLID id);
+  const TemplateURL* GetTemplateURLForId(TemplateURLID id) const;
+
   // Returns the `TemplateURL` corresponding to `starter_pack_id`, if any.
   TemplateURL* FindStarterPackTemplateURL(
       template_url_starter_pack_data::StarterPackId starter_pack_id);
@@ -748,6 +753,9 @@
   using GUIDToTURL =
       std::map<std::string, raw_ptr<TemplateURL, CtnExperimental>>;
 
+  using IdToTURL =
+      std::map<TemplateURLID, raw_ptr<TemplateURL, CtnExperimental>>;
+
   // A mapping from keywords to the corresponding TemplateURLs.
   // This is a multimap, so the system can
   // efficiently tolerate multiple engines with the same keyword, like from
@@ -806,14 +814,14 @@
   void ApplyInitializersForTesting(
       base::span<const TemplateURLService::Initializer> initializers);
 
-  // Removes |template_url| from various internal maps
-  // (|keyword_to_turl_|, |guid_to_turl_|, |provider_map_|).
+  // Removes `template_url` from various internal maps
+  // (`keyword_to_turl_`, `guid_to_turl_`, `id_to_turl_`, `provider_map_`).
   void RemoveFromMaps(const TemplateURL* template_url);
 
-  // Adds |template_url| to various internal maps
-  // (|keyword_to_turl_|, |guid_to_turl_|, |provider_map_|) if
+  // Adds `template_url` to various internal maps
+  // (`keyword_to_turl_`, `guid_to_turl_`, `id_to_turl_`, `provider_map_`) if
   // appropriate.  (It might not be appropriate if, for instance,
-  // |template_url|'s keyword conflicts with the keyword of a custom search
+  // `template_url`'s keyword conflicts with the keyword of a custom search
   // engine already existing in the maps that is not allowed to be replaced.)
   void AddToMaps(TemplateURL* template_url);
 
@@ -1018,6 +1026,9 @@
   // Mapping from Sync GUIDs to the TemplateURL.
   GUIDToTURL guid_to_turl_;
 
+  // Mapping from IDs to the TemplateURL.
+  IdToTURL id_to_turl_;
+
   // Mapping from keyword to TemplateURLs created by the policy.
   base::flat_map<std::u16string, raw_ptr<TemplateURL, CtnExperimental>>
       enterprise_search_keyword_to_turl_;