Don't report origin changes during loading.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index bc44c0207813b8c465ddf306f65d08640da25647..7f5cb26d21ddf800203f69964e5f827a96194578 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -875,6 +875,11 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
   default_search_manager_.ClearUserSelectedDefaultSearchEngine();
 
   if (!default_search_provider_) {
+    // If the default search provider came from a user pref we would have been
+    // notified of the new (fallback-provided) value in
+    // ClearUserSelectedDefaultSearchEngine() above. Since we are here, the
+    // value was presumably originally a fallback value (which may have been
+    // repaired).
     DefaultSearchManager::Source source;
     const TemplateURLData* new_dse =
         default_search_manager_.GetDefaultSearchEngine(&source);
@@ -1670,10 +1675,16 @@ void TemplateURLService::ChangeToLoadedState() {
   provider_map_->Init(template_urls_, search_terms_data);
   loaded_ = true;
 
-  // This will cause a call to NotifyObservers().
-  ApplyDefaultSearchChange(initial_default_search_provider_ ?
-                               &initial_default_search_provider_->data() : NULL,
-                           default_search_provider_source_);
+  {
+    base::AutoReset<DefaultSearchChangeOrigin> change_origin(
+        &dsp_change_origin_, DSP_CHANGE_INITIAL_LOAD);

[Peter Kasting, 2014/05/21 19:38:41]

Can't we just move the |loaded_| assignment statement below this block, and then
check the |loaded_| variable below instead of adding this new enum value?

If I'm not mistaken, you don't ever want to record a change that happens
pre-database load?

[erikwright (departed), 2014/05/21 20:38:48]

ApplyDefaultSearchChange behaves differently before/after loading completes. A
bunch of other code does, too. It's definitely not as easy as moving the loaded
statement - it would have at least some ripple effects.

[Peter Kasting, 2014/05/21 20:45:13]

My primary concern is really that you're adding a value to an enum that's kept
in sync with histograms.xml -- but this isn't actually a histogrammed value. 
This makes the two diverge some.  For that reason alone, I'd probably do this as
a separate bool instead.

One reason I asked about values in the enum no longer being in use is because in
theory you could stuff this into one of those slots, if you need a placeholder
anyway; but I worry that might make things _really_ confusing to a reader.

[erikwright (departed), 2014/05/21 21:18:01]

I agree with deprecating the old values (in a separate CL).

I don't think re-using a slot makes anything simpler.

Changing the semantics of the "loaded_" bit scares me.

How about inserting a "don't send" value in the enum, but as "-1". So it will be
in the very beginning, separate from the others, and clearly different by dint
of being negative.

+    // This will cause a call to NotifyObservers().
+    ApplyDefaultSearchChange(
+        initial_default_search_provider_ ?
+            &initial_default_search_provider_->data() : NULL,
+        default_search_provider_source_);
+  }
+
   initial_default_search_provider_.reset();
   on_loaded_callbacks_.Notify();
 }
@@ -1954,8 +1965,8 @@ void TemplateURLService::ApplyDefaultSearchChange(
       TemplateURL::MatchesData(default_search_provider_, data))
     return;
 
-  UMA_HISTOGRAM_ENUMERATION("Search.DefaultSearchChangeOrigin",
-                            dsp_change_origin_, DSP_CHANGE_MAX);
+  // Use void* to prevent a potential UAF - this is only for pointer comparison.
+  void* previous_default_search_engine = default_search_provider_;

[Peter Kasting, 2014/05/21 19:38:41]

If you're only doing a pointer comparison, you should be able to leave this as a
TemplateURL*.  Freeing the memory the pointer points to will not invalidate the
pointer, so there's no UAF danger.

[erikwright (departed), 2014/05/21 20:38:48]

This pointer could be invalidated in a variety of ways between here and when it
is used for comparison, and this method has no easy way to detect when that has
happened and set it to NULL.

It seems quite plausible to me that someone could some day introduce a check
using the pre-pop ID, the sync GUID, the id(), or other properties, which would
be a potential UAF.

By storing it as a void* we eliminate the risk of accidentally dereferencing it
while preserving the ability to do the comparison.

[Peter Kasting, 2014/05/21 20:45:13]

Right, I understand that.
I just think it would be better to store it as the correct type and comment that
it needs to be used only for a pointer comparison below and not dereferenced. 
To me, that's clearer.  Maybe I just have a nails-on-chalkboard reaction to any
use of void*.

Even if you used void*, the comment above is not clear; it makes it sound as if
not using void* will _cause_ a UAF, not just allow someone else to theoretically
add one.  You need a comment that's a little more fleshed-out.  And by the time
you have that, you have what you need to just make this non-void*, IMO.

Whatever.

 
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   if (default_search_provider_source_ == DefaultSearchManager::FROM_POLICY ||
@@ -1969,17 +1980,10 @@ void TemplateURLService::ApplyDefaultSearchChange(
 
   if (!data) {
     default_search_provider_ = NULL;
-    default_search_provider_source_ = source;
-    NotifyObservers();
-    return;
-  }
-
-  if (source == DefaultSearchManager::FROM_EXTENSION) {
+  } else if (source == DefaultSearchManager::FROM_EXTENSION) {
     default_search_provider_ = FindMatchingExtensionTemplateURL(
         *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
-  }
-
-  if (source == DefaultSearchManager::FROM_FALLBACK) {
+  } else if (source == DefaultSearchManager::FROM_FALLBACK) {
     default_search_provider_ =
         FindPrepopulatedTemplateURL(data->prepopulate_id);
     if (default_search_provider_) {
@@ -2005,8 +2009,7 @@ void TemplateURLService::ApplyDefaultSearchChange(
       if (AddNoNotify(new_dse, true))
         default_search_provider_ = new_dse;
     }
-  }
-  if (source == DefaultSearchManager::FROM_USER) {
+  } else if (source == DefaultSearchManager::FROM_USER) {
     default_search_provider_ = GetTemplateURLForGUID(data->sync_guid);
     if (!default_search_provider_ && data->prepopulate_id) {
       default_search_provider_ =
@@ -2047,7 +2050,14 @@ void TemplateURLService::ApplyDefaultSearchChange(
 #endif
   }
 
-  NotifyObservers();
+  if (default_search_provider_ != previous_default_search_engine) {
+    if (dsp_change_origin_ != DSP_CHANGE_INITIAL_LOAD) {
+      UMA_HISTOGRAM_ENUMERATION("Search.DefaultSearchChangeOrigin",
+                                dsp_change_origin_,
+                                DSP_CHANGE_MAX);
+    }
+    NotifyObservers();
+  }
 }
 
 bool TemplateURLService::AddNoNotify(TemplateURL* template_url,