Make extensions DSE persistent in browser prefs

components/search_engines/template_url_service.cc

 // Copyright 2014 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 "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 471 matching lines @@
 }
 
 TemplateURL* TemplateURLService::AddExtensionControlledTURL(
     std::unique_ptr<TemplateURL> template_url,
     std::unique_ptr<TemplateURL::AssociatedExtensionInfo> info) {
   DCHECK(loaded_);
   DCHECK(template_url);
   DCHECK_EQ(kInvalidTemplateURLID, template_url->id());
   DCHECK(info);
   DCHECK_NE(TemplateURL::NORMAL, template_url->type());
-  DCHECK(
-      !FindTemplateURLForExtension(info->extension_id, template_url->type()));
+  // It is possible that extension template url is already created for extension
+  // that is currently overriding default search, before extension itself is
+  // loaded. This is the case when extension overrides DSE in preferences and
+  // extension template url is created when TemplateUrlService changes to loaded
+  // state but before extensions are loaded.
+  TemplateURL* ext_dse =
+      FindTemplateURLForExtension(info->extension_id, template_url->type());
+  if (template_url->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION &&
+      ext_dse) {

[vasilii, 2016/12/06 19:16:42]

Shouldn't it be
if (ext_dse) {
  DCHECK_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, template_url->type());
}
?

[Alexander Yashkin, 2016/12/09 08:19:52]

Agree, fixed.

+    DCHECK(info->wants_to_be_default_engine);
+    // They must be similar at least.
+    DCHECK(ext_dse->data().keyword() == template_url->data().keyword());

[vasilii, 2016/12/06 19:16:42]

Use DCHECK_EQ

[Alexander Yashkin, 2016/12/09 08:19:52]

Fixed

+    DCHECK(ext_dse->data().short_name() == template_url->data().short_name());
+    DCHECK(ext_dse->data().url() == template_url->data().url());
+    return ext_dse;
+  }
+
   template_url->extension_info_.swap(info);
 
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
   TemplateURL* template_url_ptr = AddNoNotify(std::move(template_url), true);
   if (template_url_ptr) {
-    if (template_url_ptr->extension_info_->wants_to_be_default_engine) {
-      UpdateExtensionDefaultSearchEngine();
-    }
     NotifyObservers();
   }
 
   return template_url_ptr;
 }
 
 void TemplateURLService::Remove(TemplateURL* template_url) {
   RemoveNoNotify(template_url);
   NotifyObservers();
 }
 
 void TemplateURLService::RemoveExtensionControlledTURL(
     const std::string& extension_id,
     TemplateURL::Type type) {
   DCHECK(loaded_);
   TemplateURL* url = FindTemplateURLForExtension(extension_id, type);
   if (!url)
     return;
   // NULL this out so that we can call RemoveNoNotify.
-  // UpdateExtensionDefaultSearchEngine will cause it to be reset.
   if (default_search_provider_ == url)
     default_search_provider_ = nullptr;
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
   RemoveNoNotify(url);
-  UpdateExtensionDefaultSearchEngine();
   NotifyObservers();
 }
 
 void TemplateURLService::RemoveAutoGeneratedSince(base::Time created_after) {
   RemoveAutoGeneratedBetween(created_after, base::Time());
 }
 
 void TemplateURLService::RemoveAutoGeneratedBetween(base::Time created_after,
                                                     base::Time created_before) {
   RemoveAutoGeneratedForUrlsBetween(base::Callback<bool(const GURL&)>(),
@@ skipping 1400 matching lines @@
     UpdateProvidersCreatedByPolicy(
         &template_urls_,
         source == DefaultSearchManager::FROM_POLICY ? data : nullptr);
   }
 
   if (!data) {
     default_search_provider_ = nullptr;
   } else if (source == DefaultSearchManager::FROM_EXTENSION) {
     default_search_provider_ = FindMatchingExtensionTemplateURL(
         *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+    if (!default_search_provider_) {
+      // This is possible if extension has installed DSE, writing extension
+      // controlled pref and extension service is not yet loaded. Create
+      // extension template url from preference value and add it. Later when
+      // extension will load, it will again register its DSE in template url
+      // service.
+
+      // Get id of extension that controls preference.
+      std::string ext_id = client_->GetExtensionControllingDSEPref();

[vasilii, 2016/12/06 19:16:42]

I'm not sure about the whole flow. That call makes me suspicions.
There are two cases.
1. TemplateURLService isn't loaded yet. This situation is fully handled in
template_url_service.cc:1923.
2. TemplateURLService is loaded. TemplateURLService::AddExtensionControlledTURL
should add a TemplateURL first. Then you overwrite the preferences, the code
finds the correct search provider and makes it default. It shouldn't be possible
that we need to add a search provider here. 

[Alexander Yashkin, 2016/12/09 08:19:52]

I described earlier the flow in comments to template_url_service_unittest.cc

The problem this code solves is that when TemplateURLService is turned to loaded
state it is possible that extensions are not loaded yet and extension controlled
TURLs are not added yet to TemplateURLService. I beleive that currently where is
no guaranty that SettingsOverridesAPI::OnExtensionLoaded is called for every
extension at the moment when TemplateURLService::ChangeToLoadedState function
called. 

Even if all extensions were loaded before TemplateURLService, they are waiting
for TemplateURLService load registered by RegisterOnLoadedCallback -
settings_overrides_api.cc:197. And TemplateURLService calls its onload callbacks
only after creating its default_search_provider from intial_search_provider. 
So at the moment this function called at TemplateURLService start,
FindMatchingExtensionTemplateURL will certainly return nullptr and
default_search_provider_ will be intialized to nullptr as well. So nullptr will
be returned by TemplateURLService::GetDefaultSearchProvider() for some period
until extension that override default search is loaded and registered in
TemplateURLService.
I beleive such situation is unacceptable.

[vasilii, 2016/12/13 17:16:39]

ExtensionService::NotifyExtensionLoaded is called from
ProfileManager::CreateAndInitializeProfile indirectly in
profile_manager.cc:1307. TemplateURLService is created from
profile_manager.cc:1305. However, it's loaded lazily and asynchronously. Thus,
you can take for granted that TemplateURLService::ChangeToLoadedState happens
much later than OnExtensionLoaded().

The second point indeed means that adding a provider is necessary like in
UpdateProvidersCreatedByPolicy(). However, does somebody care which ext_id it
has? Before loading it's also unknown.
If the answer is yes than I think we should pass it via the overwritten
preference like all other meaningful search provider properties.
If the answer is no, then setting it to something here and then overwriting in 
AddExtensionControlledTURL.

[Alexander Yashkin, 2016/12/14 19:25:21]

Regarding the point that TemplateURLService is created earlier but loads later
than NotifyExtensionLoaded is called - is not it a relying to much on current
implementation? So we know that TemplateURLService loads search engines from DB
and turns to loaded state somewhere later, and we know that extension are loaded
synchronously from CreateAndInitializeProfile. What if this order of events will
change in future?

To the second remark - I think that ext_id must be correct for every extension
engine because it can be accessed by GetExtensionId() method of TemplateUrl and
can be accessed by every listener of TemplateURL service change notification. So
I beleive it must be valid and correct for every engine.
To the question of passing it through search engine properties - this creates
duplication of entities - preferences already know what extension controls (or
does not control) every preference key. So I prefer to get ext_id directly from
preference subsystem than duplicating it in search provider properties. 
With duplication of values where is always risk that values goes out of sync.

[vasilii, 2016/12/15 13:41:57]

It's hardly possible that this will change in the future. Let's assume that
somebody starts loading TemplateURLService from the constructor. Even in this
case it will take some time on another thread and then the task will be posted
back to the UI thread asynchronously. OnExtensionLoaded() is called
synchronously, thus, it will be executed first. If somebody moves extension
loading to n seconds after profile creation then there will be a plenty of bugs
in Chrome like the one you are fixing. An extension can override the new tab
page or inject a script to every page. Thus, they should be available
immediately.

What about calling AddExtensionControlledTURL() before the service loads? As the
extension search engines aren't stored anywhere you can add them to
template_urls_ earlier.

[Alexander Yashkin, 2016/12/15 15:47:10]

Calling AddExtensionControlledTURL even before TemplateURLService is loaded is
rather radical change.
I think it could work, but why it was not done so in a first place? 
Currently SettingsOverridesAPI is explicitly waiting for TemplateURLService load
completion before adding extension engine.

I can implement such solution, but what owners of TemplateURLService say?
@pkasting or @stevet? Can you help?

[Peter Kasting, 2016/12/16 08:41:06]

Maybe no one thought of it?  If you're asking if it will break something, the
answer is I don't know.  I did less with the extension-related code here, and
stevet is gone.  I don't think anyone can answer you other than git blame,
careful pondering, and maybe vasilii's guidance, which I trust much more than my
own instincts on this issue.

[Alexander Yashkin, 2016/12/16 08:45:50]

Ok, I'll try to implement it.

[vasilii, 2016/12/16 10:22:20]

I didn't implement it this way in the beginning because I though that
TemplateURLService isn't usable anyway before it's loaded and Chrome code waits
before changing TemplateURLService. My assumption was almost correct.

My immediate response to the bug was exactly that fix
(https://bugs.chromium.org/p/chromium/issues/detail?id=450534#c2). At the same
time overriding the prefs seems also a necessary step.

[Alexander Yashkin, 2016/12/17 20:02:28]

Updated, enable extensions search engines registration in TemplateURLService
before its loaded.

+      DCHECK(!ext_id.empty());
+      auto ext_turl = base::MakeUnique<TemplateURL>(
+          *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+      ext_turl->extension_info_ =
+          base::MakeUnique<TemplateURL::AssociatedExtensionInfo>(ext_id);
+      ext_turl->extension_info_->wants_to_be_default_engine = true;
+      default_search_provider_ = AddNoNotify(std::move(ext_turl), true);
+    }
   } 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.SetShortName(default_search_provider_->short_name());
@@ skipping 501 matching lines @@
     const TemplateURLData& data,
     TemplateURL::Type type) {
   DCHECK_NE(TemplateURL::NORMAL, type);
   for (const auto& turl : template_urls_) {
     if (turl->type() == type &&
         TemplateURL::MatchesData(turl.get(), &data, search_terms_data()))
       return turl.get();
   }
   return nullptr;
 }
-
-void TemplateURLService::UpdateExtensionDefaultSearchEngine() {
-  TemplateURL* most_recently_intalled_default = nullptr;
-  for (const auto& turl : template_urls_) {
-    if ((turl->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) &&
-        turl->extension_info_->wants_to_be_default_engine &&
-        turl->SupportsReplacement(search_terms_data()) &&
-        (!most_recently_intalled_default ||
-         (most_recently_intalled_default->extension_info_->install_time <
-          turl->extension_info_->install_time)))
-      most_recently_intalled_default = turl.get();
-  }
-
-  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();
-  }
-}