Protector adds the default prepopulated engine if it was removed.

chrome/browser/protector/default_search_provider_change.cc

Index: chrome/browser/protector/default_search_provider_change.cc
diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc
index a9ade1d2c9cbadf24c5cf3774c2636717b020070..64d1ab1283a7f390f1f769786484ecb62278956d 100644
--- a/chrome/browser/protector/default_search_provider_change.cc
+++ b/chrome/browser/protector/default_search_provider_change.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/protector/histograms.h"
 #include "chrome/browser/protector/protector.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "chrome/browser/webdata/keyword_table.h"
@@ -26,6 +27,43 @@ namespace {
 // Maximum length of the search engine name to be displayed.
 const size_t kMaxDisplayedNameLength = 10;
 
+// Predicate that matches a TemplateURL with given ID.
+class TemplateURLHasId {
+ public:
+  explicit TemplateURLHasId(int id) : id_(id) {
+  }
+
+  bool operator()(const TemplateURL* url) {
+    return url->id() == id_;
+  }
+
+ private:
+  int id_;

[sky, 2011/11/28 17:03:11]

int -> TemplateURLID

[Ivan Korotkov, 2011/11/29 10:13:47]

Done.

+};
+
+// Matcher for TemplateURLs based on their search and instant URLs.

[sky, 2011/11/28 17:03:11]

The code doesn't look at instant url.

[Ivan Korotkov, 2011/11/29 10:13:47]

Hmm, I though that suggest_url is actually the instant URL. On second thought,
though, it's safer here to compare by the search URL only.

+class TemplateURLMatches {
+ public:
+  // Creates a matcher based on |other|.
+  explicit TemplateURLMatches(const TemplateURL* other) : other_(other) {
+  }
+
+  // Returns true if both |other| and |url| are NULL or have the same search
+  // and instant URLs.
+  bool operator()(const TemplateURL* url) {
+    if (url == other_ )
+      return true;
+    if (!url || !other_)
+      return false;
+    return TemplateURLRef::SameUrlRefs(url->url(), other_->url()) &&
+        TemplateURLRef::SameUrlRefs(url->suggestions_url(),
+                                    other_->suggestions_url());
+  }
+
+ private:
+  const TemplateURL* other_;
+};
+
 }  // namespace
 
 class DefaultSearchProviderChange : public BaseSettingChange,
@@ -115,16 +153,26 @@ bool DefaultSearchProviderChange::Init(Protector* protector) {
   if (!default_search_provider_)
     return false;
 
+  int restored_histogram_id =
+      GetSearchProviderHistogramID(default_search_provider_);
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramSearchProviderRestored,
+      restored_histogram_id,
+      kProtectorMaxSearchProviderID);
+
   if (!old_id_ || default_search_provider_->id() != old_id_) {
     // Old settings is lost or invalid, so we had to fall back to one of the
     // prepopulated search engines.
     fallback_id_ = default_search_provider_->id();
     fallback_name_ = default_search_provider_->short_name();
-    VLOG(1) << "Fallback to " << fallback_name_;
+
+    VLOG(1) << "Fallback to search provider: " << fallback_name_;
+    UMA_HISTOGRAM_ENUMERATION(
+        kProtectorHistogramSearchProviderFallback,
+        restored_histogram_id,
+        kProtectorMaxSearchProviderID);
   }
 
-  // This must be called after the initial |SetDefaultSearchProvider| call
-  // because the latter will remove the observer.
   protector->GetTemplateURLService()->AddObserver(this);
 
   return true;
@@ -136,6 +184,7 @@ void DefaultSearchProviderChange::Apply() {
       new_histogram_id_,
       kProtectorMaxSearchProviderID);
 
+  protector()->GetTemplateURLService()->RemoveObserver(this);
   if (!new_id_) {
     // Open settings page in case the new setting is invalid.
     OpenSearchEngineSettings();
@@ -150,6 +199,7 @@ void DefaultSearchProviderChange::Discard() {
       new_histogram_id_,
       kProtectorMaxSearchProviderID);
 
+  protector()->GetTemplateURLService()->RemoveObserver(this);
   if (!old_id_) {
     // Open settings page in case the old setting is invalid.
     OpenSearchEngineSettings();
@@ -214,10 +264,11 @@ string16 DefaultSearchProviderChange::GetDiscardButtonText() const {
 }
 
 void DefaultSearchProviderChange::OnTemplateURLServiceChanged() {
-  if (protector()->GetTemplateURLService()->GetDefaultSearchProvider() !=
-      default_search_provider_) {
-    default_search_provider_ = NULL;
+  TemplateURLService* url_service = protector()->GetTemplateURLService();
+  if (url_service->GetDefaultSearchProvider() != default_search_provider_) {
     VLOG(1) << "Default search provider has been changed by user";
+    default_search_provider_ = NULL;
+    url_service->RemoveObserver(this);
     // This will delete the Protector instance and |this|.
     protector()->DismissChange();
   }
@@ -231,29 +282,47 @@ const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
     NOTREACHED() << "Can't get TemplateURLService object.";
     return NULL;
   }
+
+  TemplateURLService::TemplateURLVector urls = url_service->GetTemplateURLs();
   const TemplateURL* url = NULL;
   if (id) {
-    const TemplateURLService::TemplateURLVector& urls =
-        url_service->GetTemplateURLs();
-    for (size_t i = 0; i < urls.size(); ++i) {
-      if (urls[i]->id() == id) {
-        url = urls[i];
-        break;
-      }
-    }
+    TemplateURLService::TemplateURLVector::const_iterator i =
+        find_if(urls.begin(), urls.end(), TemplateURLHasId(id));
+    if (i != urls.end())
+      url = *i;
   }
   if (!url && allow_fallback) {
-    url = url_service->FindNewDefaultSearchProvider();
-    DCHECK(url);
+    // Fallback to the prepopulated default search provider.
+    TemplateURL* new_url =

[sky, 2011/11/28 17:03:11]

Put this in a scoped_ptr and use release so that it's less error prone.

[Ivan Korotkov, 2011/11/29 10:13:47]

Done.

+        TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(
+            NULL  // Ignore any overrides in prefs.
+        );
+    DCHECK(new_url);
+    VLOG(1) << "Prepopulated search provider: " << new_url->short_name();
+
+    // Check if a matching (see |TemplateURLMatches|) provider already exists
+    // and update it from this prepopulated template then.
+    TemplateURLService::TemplateURLVector::const_iterator i =

[sky, 2011/11/29 16:44:59]

This worries me. What are we hoping to do with this step?
Why can't we always add new_url?

[Ivan Korotkov, 2011/11/29 19:10:15]

There are examples of malware that just remove the DSP ID from meta table but
not the DSP itself from keywords. If such software also removes the backup ID,
DSP will be reset to the first prepopulated SP. However, that SP already exists
in list, so there would be two exactly same providers which is not good, right? 

So we first check if that DSP we're going to set already exists and reset it
with the prepopulated values (so even if malware tampers with suggestion/instant
URL, it won't matter). Otherwise, we add new_url. This ensures that no
duplicates are introduced.

Does that sound good?

+        find_if(urls.begin(), urls.end(), TemplateURLMatches(new_url));
+    if (i != urls.end()) {
+      VLOG(1) << "Provider already exists, updating";
+      url = *i;
+      url_service->Update(url, *new_url);
+      delete new_url;
+    } else {
+      VLOG(1) << "No match, adding new provider";
+      url = new_url;
+      url_service->Add(new_url);
+      UMA_HISTOGRAM_ENUMERATION(
+          kProtectorHistogramSearchProviderMissing,
+          GetSearchProviderHistogramID(url),
+          kProtectorMaxSearchProviderID);
+    }
   }
+
   if (url) {
-    // Remove ourselves from the observer list to prevent from catching our own
-    // change. It is safe to do this multiple times or before adding ourselves.
-    url_service->RemoveObserver(this);
-    url_service->SetDefaultSearchProvider(url);
     VLOG(1) << "Default search provider set to: " << url->short_name();
-    // No need to re-add observer again because any further changes to the
-    // default search provider are of no interest.
+    url_service->SetDefaultSearchProvider(url);
   }
   return url;
 }