Omnibox: Fix Crash by Setting Provider Type for SearchTermReplacement Queries

Omnibox: Fix Crash by Setting Provider Type for SearchTermReplacement Queries

Test:
1. run chrome with a metrics enabled and with force-fieldtrials to
  get into a search-term-replacement group
2. enter a query, hit return
3. on the search results page, click in the omnibox and press
  return again (no typing), this reissuing the query while keeping
  the omnibox closed.
4. before this change, the browser would crash.  now it does not.

BUG=370119
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270879

[Mark P, 2014-05-14 23:10:22]

Turned out to be an easy fix in the end.

--mark

[Peter Kasting, 2014-05-14 23:16:00]

LGTM.  Can we add a test for this?

[Mark P, 2014-05-14 23:35:40]

On 2014/05/14 23:16:00, Peter Kasting wrote:
> LGTM.  Can we add a test for this?

Done.

--mark

[Peter Kasting, 2014-05-15 00:28:07]

On 2014/05/14 23:35:40, Mark P wrote:
> On 2014/05/14 23:16:00, Peter Kasting wrote:
> > LGTM.  Can we add a test for this?
> 
> Done.

Hmm, the latest patch set just seems to add a DCHECK, not an actual test case?

[Mark P, 2014-05-15 03:16:48]

On 2014/05/15 00:28:07, Peter Kasting wrote:
> On 2014/05/14 23:35:40, Mark P wrote:
> > On 2014/05/14 23:16:00, Peter Kasting wrote:
> > > LGTM.  Can we add a test for this?
> > 
> > Done.
> 
> Hmm, the latest patch set just seems to add a DCHECK, not an actual test case?

I'm not sure what else to check.  The cause of the bug was not as I theorized on
the earlier changelist; it was merely that |provider| was NULL when we try to
lookup the provider name to write it to the logs.  Hence a simple DCHECK would
catch errors like this.  I placed the DCHECK at an earlier (yet still
appropriate) spot than log-writing, so it's more likely to catch something when
it appears.

--mark

[Peter Kasting, 2014-05-15 18:24:49]

On 2014/05/15 03:16:48, Mark P wrote:
> On 2014/05/15 00:28:07, Peter Kasting wrote:
> > On 2014/05/14 23:35:40, Mark P wrote:
> > > On 2014/05/14 23:16:00, Peter Kasting wrote:
> > > > LGTM.  Can we add a test for this?
> > > 
> > > Done.
> > 
> > Hmm, the latest patch set just seems to add a DCHECK, not an actual test
case?
> 
> I'm not sure what else to check.  The cause of the bug was not as I theorized
on
> the earlier changelist; it was merely that |provider| was NULL when we try to
> lookup the provider name to write it to the logs.  Hence a simple DCHECK would
> catch errors like this.

A DCHECK is documentation to the reader of this code, rather than an actual
regression test.

I meant, we don't have a regression test that will validate this codepath.  A
DCHECK is fine and dandy, but it will only help us if some sort of testing code,
that gets run on debug bots, would exercise this control flow -- otherwise the
DCHECK won't actually fire.

[Mark P, 2014-05-15 20:18:05]

On Thu, May 15, 2014 at 11:24 AM, <pkasting@chromium.org> wrote:

> On 2014/05/15 03:16:48, Mark P wrote:
>
>> On 2014/05/15 00:28:07, Peter Kasting wrote:
>> > On 2014/05/14 23:35:40, Mark P wrote:
>> > > On 2014/05/14 23:16:00, Peter Kasting wrote:
>> > > > LGTM.  Can we add a test for this?
>> > >
>> > > Done.
>> >
>> > Hmm, the latest patch set just seems to add a DCHECK, not an actual test
>>
> case?
>
>  I'm not sure what else to check.  The cause of the bug was not as I
>> theorized
>>
> on
>
>> the earlier changelist; it was merely that |provider| was NULL when we
>> try to
>> lookup the provider name to write it to the logs.  Hence a simple DCHECK
>> would
>> catch errors like this.
>>
>
> A DCHECK is documentation to the reader of this code, rather than an actual
> regression test.
>
> I meant, we don't have a regression test that will validate this codepath.
>  A
> DCHECK is fine and dandy, but it will only help us if some sort of testing
> code,
> that gets run on debug bots, would exercise this control flow -- otherwise
> the
> DCHECK won't actually fire.
>

Good point.

I went and looked to see what tests we have to search term replacement.
 There are very few.  I think a new test would either be narrowly focussed
(test GetInfoForCurrentText()), which I don't think is particularly
worthwhile, or a control flow test (a browser test that has search term
replacement, focuses on the omnibox, presses enter, then listens for the
resulting OMNIBOX_NAVIGATION event and checks the resulting values), which
I think would be nice (yet complex) and don't think it's worth my time to
write at the moment.  (It's not similar to any currently-existing tests.)

I think I'll just submit this as-is with the DCHECK.

--mark




> https://codereview.chromium.org/288073002/
>

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

[Mark P, 2014-05-15 21:56:47]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-05-15 21:58:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpearson@chromium.org/288073002/10001

[commit-bot: I haz the power, 2014-05-16 00:49:37]

Change committed as 270879