Protector adds the default prepopulated engine if it was removed.

Protector adds the default prepopulated engine if it was removed.


*) If the backup default search provider was deleted from the keywords, it is taken from the prepopulated data array.
*) If a search provider with same URLs already exists, it is updated; otherwise, the prepopulated SP is added.

BUG=105423
TEST=TemplateURLServiceTest.*; For Protector: manual with sqlite3

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112661

[Ivan Korotkov, 2011-11-28 11:46:42]

PTAL

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

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:41: int id_;
int -> TemplateURLID

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:44: // Matcher for
TemplateURLs based on their search and instant URLs.
The code doesn't look at instant url.

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:296: TemplateURL*
new_url =
Put this in a scoped_ptr and use release so that it's less error prone.

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:327:
UpdateNoNotify(existing_turl, new_values);
Add test coverage of this.

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

Updated, with a couple of checks:

TemplateURL.last_modified() says that it's the time it was last modified by
user, so I suppose Update() shouldn't modify it, right?

I am also not sure about safe_for_autoreplace. It will be reset to true when we
update an existing turl to a prepopulated one; this happens when the DSP ID is
erased in meta table or set to an invalid (non-existing) value but there exists
a search provider with the search URL same as the default prepopulated SP; is
this acceptable?

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:41: int id_;
On 2011/11/28 17:03:11, sky wrote:
> int -> TemplateURLID

Done.

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:44: // Matcher for
TemplateURLs based on their search and instant URLs.
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.

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:296: TemplateURL*
new_url =
On 2011/11/28 17:03:11, sky wrote:
> Put this in a scoped_ptr and use release so that it's less error prone.

Done.

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:327:
UpdateNoNotify(existing_turl, new_values);
On 2011/11/28 17:03:11, sky wrote:
> Add test coverage of this.

Done.

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

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:305:
TemplateURLService::TemplateURLVector::const_iterator i =
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:14]

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:305:
TemplateURLService::TemplateURLVector::const_iterator i =
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?

[sky, 2011-11-29 21:45:43]

On Tue, Nov 29, 2011 at 11:10 AM,  <ivankr@chromium.org> wrote:
>
>
http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
> File chrome/browser/protector/default_search_provider_change.cc (right):
>
>
http://codereview.chromium.org/8704007/diff/2001/chrome/browser/protector/def...
> chrome/browser/protector/default_search_provider_change.cc:305:
> TemplateURLService::TemplateURLVector::const_iterator i =
> 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?

I think the check should then be on all the values set by prepopulate
data, including the prepopulate id. If there is a match, we should set
it as the default and not bother using Update.

  -Scott

[Ivan Korotkov, 2011-11-30 18:01:55]

Agreed, updated.

[sky, 2011-11-30 21:44:08]

LGTM

[commit-bot: I haz the power, 2011-11-30 21:50:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/8704007/12001

[commit-bot: I haz the power, 2011-11-30 23:37:03]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 112329, job name
was 8704007-12001 (retry) (retry) (previous was lost) (retry).