Made the deletion of the default search provider defensive in TemplateURLService.

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 "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/guid.h"
@@ skipping 914 matching lines @@
         FindNonExtensionTemplateURLForKeyword(turl->keyword());
     if (iter->change_type() == syncer::SyncChange::ACTION_DELETE) {
       if (!existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_DELETE");
         LOG(ERROR) << "Trying to delete a non-existent TemplateURL.";
         continue;
       }
-      bool delete_default = (existing_turl == GetDefaultSearchProvider());
+      if (existing_turl == GetDefaultSearchProvider()) {
+        // The only way Sync can attempt to delete the default search provider
+        // is if we had changed the kSyncedDefaultSearchProviderGUID
+        // preference, but perhaps it has not yet been received. To avoid
+        // situations where this has come in erroneously, we will un-delete
+        // the current default search from the Sync data. If the pref really
+        // does arrive later, then default search will change to the correct
+        // entry, but we'll have this extra entry sitting around. The result is
+        // not ideal, but it prevents a far more severe bug where the default is
+        // unexpected swapped to something else. The user can safely delete the

[Nicolas Zea, 2012/07/10 17:07:47]

unexpected -> unexpectedly

[SteveT, 2012/07/10 17:58:35]

Done.

+        // extra entry again later, if they choose. Most users who do not look
+        // at the search engines UI will not notice this.
+        // Note that we append a special character to the end of the keyword in
+        // an attempt to avoid a ping-poinging situation where receiving clients
+        // may try to continually delete the resurrected entry.
+        string16 updated_keyword = UniquifyKeyword(*existing_turl, true);
+        TemplateURLData data(existing_turl->data());
+        data.SetKeyword(updated_keyword);
+        TemplateURL new_turl(existing_turl->profile(), data);
+        UIThreadSearchTermsData search_terms_data(existing_turl->profile());
+        if (UpdateNoNotify(existing_turl, new_turl, search_terms_data))
+          NotifyObservers();
 
-      if (delete_default && is_default_search_managed_) {
-        NOTREACHED() << "Tried to delete managed default search provider";
-      } else {
-        if (delete_default)
-          default_search_provider_ = NULL;
+        syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(new_turl);
+        new_changes.push_back(
+            syncer::SyncChange(syncer::SyncChange::ACTION_ADD, sync_data));
+        // Ignore the delete attempt. This means we never end up reseting the
+        // default search provider due to an ACTION_DELETE from sync.
+        continue;
+      }
 
-        Remove(existing_turl);
-
-        if (delete_default) {
-          AutoReset<DefaultSearchChangeOrigin> change_origin(
-              &dsp_change_origin_, DSP_CHANGE_SYNC_DELETE);
-          SetDefaultSearchProvider(FindNewDefaultSearchProvider());
-        }
-      }
+      Remove(existing_turl);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_ADD) {
       if (existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_ADD");
         LOG(ERROR) << "Trying to add an existing TemplateURL.";
         continue;
       }
       std::string guid = turl->sync_guid();
@@ skipping 1120 matching lines @@
     TemplateURL* existing_keyword_turl =
         FindNonExtensionTemplateURLForKeyword(template_url->keyword());
     if (existing_keyword_turl != NULL) {
       DCHECK_NE(existing_keyword_turl, template_url);
       if (CanReplace(existing_keyword_turl)) {
         RemoveNoNotify(existing_keyword_turl);
       } else if (CanReplace(template_url)) {
         delete template_url;
         return false;
       } else {
-        string16 new_keyword = UniquifyKeyword(*existing_keyword_turl);
+        string16 new_keyword = UniquifyKeyword(*existing_keyword_turl, false);
         ResetTemplateURL(existing_keyword_turl,
                          existing_keyword_turl->short_name(), new_keyword,
                          existing_keyword_turl->url());
       }
     }
   }
   template_urls_.push_back(template_url);
   AddToMaps(template_url);
 
   if (newly_adding) {
@@ skipping 114 matching lines @@
   DCHECK(loaded_);
   DCHECK(!guid.empty());
 
   TemplateURLData data(url->data());
   data.sync_guid = guid;
   TemplateURL new_url(url->profile(), data);
   UIThreadSearchTermsData search_terms_data(url->profile());
   UpdateNoNotify(url, new_url, search_terms_data);
 }
 
-string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) {
-  // Already unique.
-  if (!GetTemplateURLForKeyword(turl.keyword()))
-    return turl.keyword();
+string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
+                                             bool force) {
+  if (!force) {
+    // Already unique.
+    if (!GetTemplateURLForKeyword(turl.keyword()))
+      return turl.keyword();
 
-  // First, try to return the generated keyword for the TemplateURL.
-  GURL gurl(turl.url());
-  if (gurl.is_valid()) {
-    string16 keyword_candidate = GenerateKeyword(gurl);
-    if (!GetTemplateURLForKeyword(keyword_candidate))
-      return keyword_candidate;
+    // First, try to return the generated keyword for the TemplateURL.
+    GURL gurl(turl.url());
+    if (gurl.is_valid()) {
+      string16 keyword_candidate = GenerateKeyword(gurl);
+      if (!GetTemplateURLForKeyword(keyword_candidate))
+        return keyword_candidate;
+    }
   }
 
   // We try to uniquify the keyword by appending a special character to the end.
   // This is a best-effort approach where we try to preserve the original
   // keyword and let the user do what they will after our attempt.
   string16 keyword_candidate(turl.keyword());
   do {
     keyword_candidate.append(ASCIIToUTF16("_"));
   } while (GetTemplateURLForKeyword(keyword_candidate));
 
@@ skipping 28 matching lines @@
     // with us.  Note that if we're being called from
     // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
     // than having just been brought down, then this is wrong, because the
     // server doesn't yet know about this entity; but in this case,
     // PruneSyncChanges() will prune out the ACTION_DELETE we create here.
     syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
     change_list->push_back(
         syncer::SyncChange(syncer::SyncChange::ACTION_DELETE, sync_data));
     Remove(local_turl);
   } else if (local_is_better) {
-    string16 new_keyword = UniquifyKeyword(*sync_turl);
+    string16 new_keyword = UniquifyKeyword(*sync_turl, false);
     DCHECK(!GetTemplateURLForKeyword(new_keyword));
     sync_turl->data_.SetKeyword(new_keyword);
     // If we update the cloud TURL, we need to push an update back to sync
     // informing it that something has changed.
     syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
     change_list->push_back(
         syncer::SyncChange(syncer::SyncChange::ACTION_UPDATE, sync_data));
   } else {
-    string16 new_keyword = UniquifyKeyword(*local_turl);
+    string16 new_keyword = UniquifyKeyword(*local_turl, false);
     TemplateURLData data(local_turl->data());
     data.SetKeyword(new_keyword);
     TemplateURL new_turl(local_turl->profile(), data);
     UIThreadSearchTermsData search_terms_data(local_turl->profile());
     if (UpdateNoNotify(local_turl, new_turl, search_terms_data))
       NotifyObservers();
     // Since we're processing sync changes, the UpdateNoNotify() above didn't
     // generate an ACTION_UPDATE.  We need to do it manually to keep the server
     // in sync with us.  Note that if we're being called from
     // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
@@ skipping 100 matching lines @@
     // TODO(mpcomplete): If we allow editing extension keywords, then those
     // should be persisted to disk and synced.
     if (template_url->sync_guid().empty() &&
         !template_url->IsExtensionKeyword()) {
       template_url->data_.sync_guid = base::GenerateGUID();
       if (service_.get())
         service_->UpdateKeyword(template_url->data());
     }
   }
 }