Fix a bug where two loaded keywords clash after a GoogleBaseURL change.

Fix a bug where two loaded keywords clash after a GoogleBaseURL change.

Repros when a keyword exists for google.ca, in addition to a pre-pop for google.com, and the user is in Canada. The google.com is rewritten to google.ca and the latter should be uniquified to google.ca_. But it was not working as intended.

NOTREECHECKS=True
NOTRY=True
BUG=365762
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270258

[erikwright (departed), 2014-05-13 15:35:10]

pkasting: PTAL. Note that this is desired for merge to M36 before EOD.

[Peter Kasting, 2014-05-13 17:58:19]

I don't understand a thing about how this change works :(

How was this problem addressed before your refactoring?  Does this duplicate
that functionality?

Can we add comments in the places you're changing about why some actions should
be done only if we're loaded, but others need to be done before that?

[erikwright (departed), 2014-05-13 18:25:44]

On 2014/05/13 17:58:19, Peter Kasting wrote:
> I don't understand a thing about how this change works :(
> 
> How was this problem addressed before your refactoring?  Does this duplicate
> that functionality?
> 
> Can we add comments in the places you're changing about why some actions
should
> be done only if we're loaded, but others need to be done before that?

The bug occurs as follows:

google.com is a pre-populated engine.

The user adds google.ca in the UI and makes it the default.

At startup, we "AddNoNotify" each of the things from web data, and the
user-defined google.ca (which is also the default) happens to be first.

Then we add the google.com, but we call ResetKeywordIfNecessary, which changes
it to google.ca. So we would uniquify the user-defined google.ca to google.ca_.
The code then called ResetTemplateURLNoNotify, but that code would always be a
no-op because |loading_| is false. This is the case even in the previous
implementation.

I'm not quite sure why this would have worked before. SetTemplateURLs was always
called with |loading_| false. So ResetTemplateURLNoNotify would have had no
effect. Maybe something earlier would have uniquified the value.

I will add some comments in any case.

[chromium-reviews, 2014-05-13 18:31:19]

PTAL. Added comments.
On 13 May 2014 14:25, <erikwright@chromium.org> wrote:

> On 2014/05/13 17:58:19, Peter Kasting wrote:
>
>> I don't understand a thing about how this change works :(
>>
>
>  How was this problem addressed before your refactoring?  Does this
>> duplicate
>> that functionality?
>>
>
>  Can we add comments in the places you're changing about why some actions
>>
> should
>
>> be done only if we're loaded, but others need to be done before that?
>>
>
> The bug occurs as follows:
>
> google.com is a pre-populated engine.
>
> The user adds google.ca in the UI and makes it the default.
>
> At startup, we "AddNoNotify" each of the things from web data, and the
> user-defined google.ca (which is also the default) happens to be first.
>
> Then we add the google.com, but we call ResetKeywordIfNecessary, which
> changes
> it to google.ca. So we would uniquify the user-defined google.ca to
> google.ca_.
> The code then called ResetTemplateURLNoNotify, but that code would always
> be a
> no-op because |loading_| is false. This is the case even in the previous
> implementation.
>
> I'm not quite sure why this would have worked before. SetTemplateURLs was
> always
> called with |loading_| false. So ResetTemplateURLNoNotify would have had no
> effect. Maybe something earlier would have uniquified the value.
>
> I will add some comments in any case.
>
> https://codereview.chromium.org/287563004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-05-13 18:47:29]

On 2014/05/13 18:31:19, chromium-reviews_chromium.org wrote:
> > At startup, we "AddNoNotify" each of the things from web data, and the
> > user-defined google.ca (which is also the default) happens to be first.
> >
> > Then we add the http://google.com, but we call ResetKeywordIfNecessary,
which
> > changes
> > it to google.ca. So we would uniquify the user-defined google.ca to
> > google.ca_.
> > The code then called ResetTemplateURLNoNotify, but that code would always
> > be a
> > no-op because |loading_| is false. This is the case even in the previous
> > implementation.

So, the issue occurs when the Google base URL is google.com on shutdown (so the
prepopulated engine would have been saved with a google.com keyword) but
google.ca on startup?  And we get the new base URL before the web data service
finishes loading (so we believe the keyword should be google.ca at the time the
web data service calls us back)?

I really want to fully understand what the old code did in this scenario to be
sure it had this bug too.  It doesn't seem like that's clear right now.

[Peter Kasting, 2014-05-13 19:04:23]

I still want to know what the old code did, but I've now looked at this enough
that I think I understand what's going on.  LGTM.

Side note: I wonder if the conflict resolution in AddNoNotify() should uniquify
the keyword that's being added, instead of the existing keyword.  This is the
second time in a couple weeks that I've seen a case where that code fires and I
think it would be better if the existing keyword in the DB were left unmodified.
 Feel free to investigate this in a followup CL if you agree.

https://codereview.chromium.org/287563004/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/287563004/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1596: // |provider_map_|
is only initialized after loading has completed.
Move this comment above the conditional instead of inside it (2 places)

https://codereview.chromium.org/287563004/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:1742: // |provider_map_|
is only initialized after loading has completed.
Remove this comment, the one above is clear enough.

https://codereview.chromium.org/287563004/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service.cc:2077: if
(existing_keyword_already_stored) {
Nit: How about this, which seems a bit clearer to me:

  template_url->ResetKeywordIfNecessary(false);

  // Check whether |template_url|'s keyword conflicts with any already in the
  // model.  Note that we can reach here during the loading phase while
  // processing the template URLs from the web data service.  In this case,
  // GetTemplateURLForKeyword() will look not only at what's already in the
  // model, but at the |initial_default_search_provider_|.  Since this engine
  // will presumably also be present in the web data, we need to double-check
  // that any "pre-existing" entries we find are actually coming from
  // |template_urls_|, lest we detect a "conflict" between the
  // |initial_default_search_provider_| and the web data version of itself.
  TemplateURL* existing_keyword_turl =
      GetTemplateURLForKeyword(template_url->keyword());
  if (existing_keyword_turl &&
      (std::find(template_urls_.begin(), template_urls_.end(),
                 existing_keyword_turl) != template_urls_.end())) {
    // We have a conflict.  Only replace one of the TemplateURLs if they are
    // either both extensions, or both not extensions.
    DCHECK_NE(existing_keyword_turl, template_url);
    bool are_same_type = ...

[erikwright (departed), 2014-05-13 21:15:39]

The CQ bit was checked by erikwright@chromium.org

[erikwright (departed), 2014-05-13 21:16:15]

Responded to comments. I will investigate the previous behaviour first thing
Thursday.

[commit-bot: I haz the power, 2014-05-13 21:18:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/287563004/40001

[erikwright (departed), 2014-05-13 23:57:31]

The CQ bit was unchecked by erikwright@chromium.org

[erikwright (departed), 2014-05-13 23:57:33]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-05-13 23:59:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/287563004/40001

[commit-bot: I haz the power, 2014-05-14 00:02:22]

Change committed as 270258

[erikwright (departed), 2014-05-22 21:10:24]

Cait supplied me with a profile directory that can reproduce the crash on
startup.  I ran it both against the current ToT and also against builds
from before our changes.

That Web Data DB has the user-defined google.ca entry and the pre-pop'd
google.com entry. It crashes on load before our changes, and with the same
stack trace.

On the other hand, _all_ build I tested instantly update the pre-pop'd
entry to google.ca during startup.

So my interpretation is that this bug would reliably reproduce on new and
old builds if you had a user-defined google.ca keyword and then moved from
the US to Canada (and accepted the prompt to update the Google base URL).
I'm not sure why Cait was able to get into this state without actually
crossing any borders.

It may be that this bug always existed and we happened to come upon it
while testing our changes. At least with our fixes it will not crash.

Let me know if you would like to see further follow up or attempts to
reproduce.

On Tue, May 13, 2014 at 8:02 PM, <commit-bot@chromium.org> wrote:

> Change committed as 270258
>
> https://codereview.chromium.org/287563004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.