Fix a bug where two loaded keywords clash after a GoogleBaseURL change.

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 1575 matching lines @@
     }
     if (best_fallback)
       keyword_to_template_map_[keyword] = best_fallback;
     else
       keyword_to_template_map_.erase(keyword);
   }
 
   if (!template_url->sync_guid().empty())
     guid_to_template_map_.erase(template_url->sync_guid());
   if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.

[Peter Kasting, 2014/05/13 19:04:23]

Move this comment above the conditional instead of inside it (2 places)

     UIThreadSearchTermsData search_terms_data(template_url->profile());
     provider_map_->Remove(template_url, search_terms_data);
   }
 }
 
 void TemplateURLService::AddToMaps(TemplateURL* template_url) {
   bool template_url_is_omnibox_api =
       template_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION;
   const base::string16& keyword = template_url->keyword();
   KeywordToTemplateMap::const_iterator i =
@@ skipping 10 matching lines @@
         existing_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION;
     DCHECK(existing_url_is_omnibox_api || template_url_is_omnibox_api);
     if (existing_url_is_omnibox_api ?
         !CanReplace(template_url) : CanReplace(existing_url))
       keyword_to_template_map_[keyword] = template_url;
   }
 
   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.
     UIThreadSearchTermsData search_terms_data(profile_);
     provider_map_->Add(template_url, search_terms_data);
   }
 }
 
 // Helper for partition() call in next function.
 bool HasValidID(TemplateURL* t_url) {
   return t_url->id() != kInvalidTemplateURLID;
 }
 
@@ skipping 75 matching lines @@
         ((*i)->keyword() == keyword))
       return *i;
   }
   return NULL;
 }
 
 bool TemplateURLService::UpdateNoNotify(
     TemplateURL* existing_turl,
     const TemplateURL& new_values,
     const SearchTermsData& old_search_terms_data) {
-  DCHECK(loaded_);
   DCHECK(existing_turl);
   if (std::find(template_urls_.begin(), template_urls_.end(), existing_turl) ==
       template_urls_.end())
     return false;
 
   base::string16 old_keyword(existing_turl->keyword());
   keyword_to_template_map_.erase(old_keyword);
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_.erase(existing_turl->sync_guid());
 
-  provider_map_->Remove(existing_turl, old_search_terms_data);
+  // |provider_map_| is only initialized after loading has completed.
+  if (loaded_)
+    provider_map_->Remove(existing_turl, old_search_terms_data);
+
   TemplateURLID previous_id = existing_turl->id();
   existing_turl->CopyFrom(new_values);
   existing_turl->data_.id = previous_id;
-  UIThreadSearchTermsData new_search_terms_data(profile_);
-  provider_map_->Add(existing_turl, new_search_terms_data);
+
+  if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.

[Peter Kasting, 2014/05/13 19:04:23]

Remove this comment, the one above is clear enough.

+    UIThreadSearchTermsData new_search_terms_data(profile_);
+    provider_map_->Add(existing_turl, new_search_terms_data);
+  }
 
   const base::string16& keyword = existing_turl->keyword();
   KeywordToTemplateMap::const_iterator i =
       keyword_to_template_map_.find(keyword);
   if (i == keyword_to_template_map_.end()) {
     keyword_to_template_map_[keyword] = existing_turl;
   } else {
     // We can theoretically reach here in two cases:
     //   * There is an existing extension keyword and sync brings in a rename of
     //     a non-extension keyword to match.  In this case we just need to pick
@@ skipping 299 matching lines @@
     DCHECK(std::find(template_urls_.begin(), template_urls_.end(),
                      template_url) == template_urls_.end());
     template_url->data_.id = ++next_id_;
   }
 
   template_url->ResetKeywordIfNecessary(false);
   // Check whether |template_url|'s keyword conflicts with any already in the
   // model.
   TemplateURL* existing_keyword_turl =
       GetTemplateURLForKeyword(template_url->keyword());
-  if (existing_keyword_turl != NULL) {
+
+  // We might be processing results from WebData now, in which case we are not
+  // in the loaded state and existing_keyword_turl might be the (not yet added)
+  // initial_default_search_provider_.
+  bool existing_keyword_already_stored =
+      existing_keyword_turl &&
+      (std::find(template_urls_.begin(),
+                 template_urls_.end(),
+                 existing_keyword_turl) != template_urls_.end());
+
+  // We only do the following if there is a conflict already processed into
+  // template_urls_.
+  if (existing_keyword_already_stored) {

[Peter Kasting, 2014/05/13 19:04:23]

Nit: How about this, which seems a bit clearer to me:

  template_url->ResetKeywordIfNecessary(false);

  // Check whether |template_url|'s keyword conflicts with any already in the
  // model.  Note that we can reach here during the loading phase while
  // processing the template URLs from the web data service.  In this case,
  // GetTemplateURLForKeyword() will look not only at what's already in the
  // model, but at the |initial_default_search_provider_|.  Since this engine
  // will presumably also be present in the web data, we need to double-check
  // that any "pre-existing" entries we find are actually coming from
  // |template_urls_|, lest we detect a "conflict" between the
  // |initial_default_search_provider_| and the web data version of itself.
  TemplateURL* existing_keyword_turl =
      GetTemplateURLForKeyword(template_url->keyword());
  if (existing_keyword_turl &&
      (std::find(template_urls_.begin(), template_urls_.end(),
                 existing_keyword_turl) != template_urls_.end())) {
    // We have a conflict.  Only replace one of the TemplateURLs if they are
    // either both extensions, or both not extensions.
    DCHECK_NE(existing_keyword_turl, template_url);
    bool are_same_type = ...

     DCHECK_NE(existing_keyword_turl, template_url);
     // Only replace one of the TemplateURLs if they are either both extensions,
     // or both not extensions.
     bool are_same_type = existing_keyword_turl->GetType() ==
         template_url->GetType();
     if (CanReplace(existing_keyword_turl) && are_same_type) {
       RemoveNoNotify(existing_keyword_turl);
     } else if (CanReplace(template_url) && are_same_type) {
       delete template_url;
       return false;
@@ skipping 43 matching lines @@
 
     // Inform sync of the deletion.
     ProcessTemplateURLChange(FROM_HERE,
                              template_url,
                              syncer::SyncChange::ACTION_DELETE);
 
     UMA_HISTOGRAM_ENUMERATION(kDeleteSyncedEngineHistogramName,
                               DELETE_ENGINE_USER_ACTION, DELETE_ENGINE_MAX);
   }
 
-  if (profile_) {
+  if (loaded_ && profile_) {
     content::Source<Profile> source(profile_);
     TemplateURLID id = template_url->id();
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_TEMPLATE_URL_REMOVED,
         source,
         content::Details<TemplateURLID>(&id));
   }
 
   // We own the TemplateURL and need to delete it.
   delete template_url;
 }
 
 bool TemplateURLService::ResetTemplateURLNoNotify(
     TemplateURL* url,
     const base::string16& title,
     const base::string16& keyword,
     const std::string& search_url) {
-  if (!loaded_)
-    return false;
   DCHECK(!keyword.empty());
   DCHECK(!search_url.empty());
   TemplateURLData data(url->data());
   data.short_name = title;
   data.SetKeyword(keyword);
   if (search_url != data.url()) {
     data.SetURL(search_url);
     // The urls have changed, reset the favicon url.
     data.favicon_url = GURL();
   }
@@ skipping 322 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }