Don't report origin changes during loading.

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 857 matching lines @@
        ++i) {
     AddNoNotify(new TemplateURL(profile_, *i), true);
   }
 
   base::AutoReset<DefaultSearchChangeOrigin> change_origin(
       &dsp_change_origin_, DSP_CHANGE_PROFILE_RESET);
 
   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);
     // ApplyDefaultSearchChange will notify observers once it is done.
     ApplyDefaultSearchChange(new_dse, source);
   } else {
     NotifyObservers();
   }
 }
 
@@ skipping 775 matching lines @@
   urls->clear();
 }
 
 void TemplateURLService::ChangeToLoadedState() {
   DCHECK(!loaded_);
 
   UIThreadSearchTermsData search_terms_data(profile_);
   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();
 }
 
 bool TemplateURLService::CanReplaceKeywordForHost(
     const std::string& host,
     TemplateURL** to_replace) {
   DCHECK(!to_replace || !*to_replace);
   const TemplateURLSet* urls = provider_map_->GetURLsForHost(host);
   if (!urls)
@@ skipping 260 matching lines @@
   }
 
   // Prevent recursion if we update the value stored in default_search_manager_.
   // Note that we exclude the case of data == NULL because that could cause a
   // false positive for recursion when the initial_default_search_provider_ is
   // NULL due to policy. We'll never actually get recursion with data == NULL.
   if (source == default_search_provider_source_ && data != NULL &&
       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 ||
       source == DefaultSearchManager::FROM_POLICY) {
     // We do this both to remove any no-longer-applicable policy-defined DSE as
     // well as to add the new one, if appropriate.
     UpdateProvidersCreatedByPolicy(
         &template_urls_,
         source == DefaultSearchManager::FROM_POLICY ? data : NULL);
   }
 
   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_) {
       TemplateURLData update_data(*data);
       update_data.sync_guid = default_search_provider_->sync_guid();
       if (!default_search_provider_->safe_for_autoreplace()) {
         update_data.safe_for_autoreplace = false;
         update_data.SetKeyword(default_search_provider_->keyword());
         update_data.short_name = default_search_provider_->short_name();
       }
       UIThreadSearchTermsData search_terms_data(
           default_search_provider_->profile());
       UpdateNoNotify(default_search_provider_,
                      TemplateURL(profile_, update_data),
                      search_terms_data);
     } else {
       // Normally the prepopulated fallback should be present in
       // |template_urls_|, but in a few cases it might not be:
       // (1) Tests that initialize the TemplateURLService in peculiar ways.
       // (2) If the user deleted the pre-populated default and we subsequently
       // lost their user-selected value.
       TemplateURL* new_dse = new TemplateURL(profile_, *data);
       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_ =
           FindPrepopulatedTemplateURL(data->prepopulate_id);
     }
     TemplateURLData new_data(*data);
     new_data.show_in_default_list = true;
     if (default_search_provider_) {
       UIThreadSearchTermsData search_terms_data(
           default_search_provider_->profile());
@@ skipping 20 matching lines @@
       default_search_provider_->HasGoogleBaseURLs()) {
     if (profile_)
       GoogleURLTracker::RequestServerCheck(profile_, false);
 #if defined(ENABLE_RLZ)
     RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                    RLZTracker::CHROME_OMNIBOX,
                                    rlz_lib::SET_TO_GOOGLE);
 #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,
                                      bool newly_adding) {
   DCHECK(template_url);
 
   if (newly_adding) {
     DCHECK_EQ(kInvalidTemplateURLID, template_url->id());
     DCHECK(std::find(template_urls_.begin(), template_urls_.end(),
                      template_url) == template_urls_.end());
@@ skipping 434 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();
   }
 }