Make extensions DSE persistent in browser prefs (Reland)

components/search_engines/default_search_manager.cc

Index: components/search_engines/default_search_manager.cc
diff --git a/components/search_engines/default_search_manager.cc b/components/search_engines/default_search_manager.cc
index c448ebe60d2a16c1b9357aeb666da167c25997d5..a69aa74a9d507706378b4e1e913e10b6d00c1cf9 100644
--- a/components/search_engines/default_search_manager.cc
+++ b/components/search_engines/default_search_manager.cc
@@ -15,7 +15,6 @@
 #include "base/compiler_specific.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -140,7 +139,6 @@ const TemplateURLData* DefaultSearchManager::GetDefaultSearchEngine(
       *source = FROM_USER;
     return prefs_default_search_.get();
   }
-
   if (source)
     *source = FROM_FALLBACK;
   return GetFallbackSearchEngine();
@@ -171,20 +169,6 @@ void DefaultSearchManager::SetUserSelectedDefaultSearchEngine(
                      *TemplateURLDataToDictionary(data));
 }
 
-void DefaultSearchManager::SetExtensionControlledDefaultSearchEngine(
-    const TemplateURLData& data) {
-  extension_default_search_.reset(new TemplateURLData(data));
-  if (GetDefaultSearchEngineSource() == FROM_EXTENSION)
-    NotifyObserver();
-}
-
-void DefaultSearchManager::ClearExtensionControlledDefaultSearchEngine() {
-  Source old_source = GetDefaultSearchEngineSource();
-  extension_default_search_.reset();
-  if (old_source == FROM_EXTENSION)
-    NotifyObserver();
-}
-
 void DefaultSearchManager::ClearUserSelectedDefaultSearchEngine() {
   if (pref_service_) {
     pref_service_->ClearPref(kDefaultSearchProviderDataPrefName);
@@ -195,13 +179,13 @@ void DefaultSearchManager::ClearUserSelectedDefaultSearchEngine() {
 }
 
 void DefaultSearchManager::OnDefaultSearchPrefChanged() {
-  Source source = GetDefaultSearchEngineSource();
+  bool source_was_fallback = GetDefaultSearchEngineSource() == FROM_FALLBACK;
+
   LoadDefaultSearchEngineFromPrefs();
 
-  // If we were/are FROM_USER or FROM_POLICY the effective DSE may have changed.
-  if (source != FROM_USER && source != FROM_POLICY)
-    source = GetDefaultSearchEngineSource();
-  if (source == FROM_USER || source == FROM_POLICY)
+  // The effective DSE may have changed unless we were using the fallback source
+  // both before and after the above load.
+  if (!source_was_fallback || (GetDefaultSearchEngineSource() != FROM_FALLBACK))
     NotifyObserver();
 }
 
@@ -249,6 +233,7 @@ void DefaultSearchManager::LoadDefaultSearchEngineFromPrefs() {
     return;
 
   prefs_default_search_.reset();
+  extension_default_search_.reset();
   const PrefService::Preference* pref =
       pref_service_->FindPreference(kDefaultSearchProviderDataPrefName);
   DCHECK(pref);
@@ -270,8 +255,13 @@ void DefaultSearchManager::LoadDefaultSearchEngineFromPrefs() {
   if (!turl_data)
     return;
 
-  prefs_default_search_ = std::move(turl_data);
-  MergePrefsDataWithPrepopulated();
+  // Check if default search preference is overriden by extension.
+  if (pref->IsExtensionControlled()) {
+    extension_default_search_ = std::move(turl_data);
+  } else {
+    prefs_default_search_ = std::move(turl_data);
+    MergePrefsDataWithPrepopulated();

[Peter Kasting, 2017/02/28 00:56:37]

Do we need to make this call regardless?

It seems like not doing it means that if an extension is overriding the DSE,
then no prepopulate data updates will be applied to any of the prepopulated
engines.  That sounds bad?  What are the consequences of making this call even
when an extension is controlling the DSE?

[Alexander Yashkin, 2017/02/28 11:59:56]

MergePrefsDataWithPrepopulated is updating values of default search engine
restored from prefs with values from prepopulated engine if engine from prefs
has valid prepopulate_id. 
No updates are applied to the prepopulated engines. 

Currently extension installed search engine can have non zero prepopulated_id
field in extension manifest.
This field is used to initialize extension engine fields on extension load to
values from prepopulated engine. This is done in ConvertSearchProvider function.
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_o...

After initialization prepopulate_id field of newly created TemplateURLData is
assigned zero at 
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_o...
It seems logical since extension engine is not prepopulated and must not be
returned by functions like FindPrepopulatedTemplateURL.

So in current implementation MergePrefsDataWithPrepopulated will not affect
default search engine installed by extension because extension engine has
prepopulate_id = 0.
I see no sense performing MergePrefsDataWithPrepopulated for extension engine.


What I have thought of - where is possibility that rarely, after browser update,
default search engine installed by extension will be reset to nullptr.
This could happen if prepopulated engines change with browser update but
extension engine fields were initialized and written to prefs while previous
prepopulated engines version was active.
In this case on TemplateURLService load we could fail to match TemplateURLData
from prefs to TemplateURL created by extension on load and
default_search_provider_  will be assigned nullptr in line
https://cs.chromium.org/chromium/src/components/search_engines/template_url_s....

After browser restart default engine will be returned to extension controlled,
but still.
What solutions  I could think of:
1. Again I think about storing extension_id of extension engine in prefs to use
it for matching.
2. Another idea is keeping original prepopulate_id of extension search engines
and updating their fields by MergePrefsDataWithPrepopulated in
DefaultSearchManager. 
In this case we should ignore extension engines in all places there
prepopulate_id is checked in TemplateURLService. 
This seems to me more error prone, but possible.
3. Or we can just ignore this situation due to its rare possibility.

WDYT?

[Peter Kasting, 2017/02/28 23:30:30]

Could you spell this out more?

The scenario is that the user loads an extension which specifies an engine with
a prepopulate_id, and we populate its fields from that data (and set this engine
as default? Not sure).  Then on the next startup, the browser has been updated
with new prepopulated data, and... something about loading with
|initial_default_search_provider_| not set to values that match the ones in the
extension.  But I'm not really sure precisely how that happens?  Can you walk me
through the call chains in more detail here?

[Alexander Yashkin, 2017/03/01 05:36:43]

I think I was wrong.

This was my a theory:
1. Extension with default search is installed with prepopulated_id in manifest =
1. 
From SettingsOverridesAPI::RegisterSearchProvider it will write to extensions
preferences TemplateURLData with some of fields initialized with values from
prepopulated engine with id =1. For example search_url = "www.google.com/1".
2. Browser is updated and new version contains changed fields for prepopulated
engine with id = 1. search_url = "www.google.com/2"
3. On updated browser start
DefaultSearchManager::LoadDefaultSearchEngineFromPrefs loads from prefs
TemplateURLData with previous values of extension engine, especially search_url,
and set it to  extension_default_search_ in DefaultSearchManager and
initial_default_search_provider_ in TemplateURLService.
4. Extension is loaded and performs initialization in
SettingsOverridesAPI::RegisterSearchProvider. It will load new fields for
prepopulated engine with id =1.
It will add AddExtensionControlledTURL with new value of search_url to
TemplateURLService and write new TemplateURLData to prefs.
5. When turning to loaded state TemplateURLService will fail in
ApplyDefaultSearchChangeNoMetrics when FindMatchingDefaultExtensionTemplateURL
will fail to match initial_default_search_provider_ TemplateURLData with old
value of search_url to extension TemplateURL  with new search_url.

Now I think that in step 4 when extension will write to prefs its new
TemplateURLData, pref observer in DefaultSearchManager will reread new value and
update extension_default_search_ and initial_default_search_provider_ in
TemplateURLService to valid values.

So no worry, false alarm :).

+  }
 }
 
 void DefaultSearchManager::LoadPrepopulatedDefaultSearch() {