New issue
Advanced search Search tips

Issue 789913 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Exactly-Typed Non-Substituting Keywords Aren't Made the Default Match

Project Member Reported by grt@chromium.org, Nov 30 2017

Issue description

Something seems to have regressed between r518583 (green test run here: https://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/32664) and r520082 (red test run here: https://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/36810). The test is now failing consistently with:

[ RUN      ] OmniboxViewTest.NonSubstitutingKeywordTest
../../chrome/browser/ui/omnibox/omnibox_view_browsertest.cc(1310): error:       Expected: AutocompleteMatchType::HISTORY_KEYWORD
      Which is: 4
To be equal to: popup_model->result().default_match()->type
      Which is: 6
[5184:2328:1129/061833.120:INFO:chrome_cryptauth_service.cc(222)] Profile is not authenticated yet; waiting before starting CryptAuth managers.
[5184:2328:1129/061833.145:WARNING:url_request_context_getter.cc(43)] URLRequestContextGetter leaking due to no owning thread.
[  FAILED  ] OmniboxViewTest.NonSubstitutingKeywordTest, where TypeParam =  and GetParam() =  (799 ms)

See the green and red tryjobs on https://chromium-review.googlesource.com/c/chromium/src/+/784937/2.

Assigning to some random amateur in chrome/browser/ui/omnibox/OWNERS for triage and assignment.
 
Cc: pkasting@chromium.org
Owner: mpear...@chromium.org
https://chromium.googlesource.com/chromium/src/+/31c2066fb04ade8b1474c27054f425fe8a72d04f
seems likely.

I'll investigate.
Status: Started (was: Assigned)
I haven't even tested to see if this is the cause of the error, but I think I know what's going on.

On this line
https://cs.chromium.org/chromium/src/components/omnibox/browser/keyword_provider.cc?l=348
instead of
input.allow_exact_keyword_match()
we should use
input.allow_exact_keyword_match() || !template_url->SupportsReplacement(model_->search_terms_data())

Comment 3 by grt@chromium.org, Dec 1 2017

Thank you for picking this up so quickly!
Labels: ReleaseBlock-Stable OS-Chrome OS-Linux OS-Mac
Summary: Exactly-Typed Non-Substituting Keywords Aren't Made the Default Match (was: Consistent OmniboxViewTest.NonSubstitutingKeywordTest failure)
> Thank you for picking this up so quickly!
You're welcome.  Thanks for reporting.  This is a real (user-facing) bug.  I'm glad you tried to revive the tests and thus discovered it.

Repro steps:
1. Start chrome.
2. Create a new "search" engine that doesn't actually do a search.  For example, create an engine "Testing Engine" with keyword "test" that goes to http://example.com/.  Note the lack of the "%s" in the "search" URL.
3. Type "test" in the omnibox

Expected results:
4. "Testing engine" is the default match.  If you press enter, you go to example.com.

Actual results:
4. The default match is "test".  If you press enter, you go to a google search for "test".  The second match was for "Testing engine".
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d2d779e0c03c8c7a3df30b33a33f494cf148629

commit 0d2d779e0c03c8c7a3df30b33a33f494cf148629
Author: Mark Pearson <mpearson@chromium.org>
Date: Sat Dec 02 06:34:44 2017

Omnibox - Exactly-Typed Non-Substituting Keywords Should Be Default

This was broken by
https://chromium.googlesource.com/chromium/src/+/31c2066fb04ade8b1474c27054f425fe8a72d04f

Tested interactively using repro steps on bug.

Also re-enables programmatic test for this bug.

Bug:  789913 
Change-Id: I43a59ef112da6f1e69c30a46c55586d5f9280f41
Reviewed-on: https://chromium-review.googlesource.com/804676
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521203}
[modify] https://crrev.com/0d2d779e0c03c8c7a3df30b33a33f494cf148629/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
[modify] https://crrev.com/0d2d779e0c03c8c7a3df30b33a33f494cf148629/components/omnibox/browser/keyword_provider.cc

Labels: Merge-Request-64
 Issue 791272  has been merged into this issue.
Project Member

Comment 8 by sheriffbot@chromium.org, Dec 3 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 4 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/743227c56e6d75d2689b7e8220654356df1e7843

commit 743227c56e6d75d2689b7e8220654356df1e7843
Author: Mark Pearson <mpearson@chromium.org>
Date: Mon Dec 04 17:52:23 2017

Omnibox - Exactly-Typed Non-Substituting Keywords Should Be Default

This was broken by
https://chromium.googlesource.com/chromium/src/+/31c2066fb04ade8b1474c27054f425fe8a72d04f

Tested interactively using repro steps on bug.

Also re-enables programmatic test for this bug.

Bug:  789913 
Change-Id: I43a59ef112da6f1e69c30a46c55586d5f9280f41
Reviewed-on: https://chromium-review.googlesource.com/804676
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#521203}(cherry picked from commit 0d2d779e0c03c8c7a3df30b33a33f494cf148629)
Reviewed-on: https://chromium-review.googlesource.com/806795
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#14}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/743227c56e6d75d2689b7e8220654356df1e7843/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
[modify] https://crrev.com/743227c56e6d75d2689b7e8220654356df1e7843/components/omnibox/browser/keyword_provider.cc

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1ccd58125c75660a7790724ea5faed78a055d0c

commit e1ccd58125c75660a7790724ea5faed78a055d0c
Author: Greg Thompson <grt@chromium.org>
Date: Mon Dec 04 19:49:53 2017

Enable interactive_ui_tests on Windows that now run mostly reliably.

r516772 was the last of many commits that resolved a number of problems
that made this test suite flaky on Windows. This CL enables tests that
had been disabled over time due to flakes, yet now seem to run okay.

BUG= 133341 ,751031, 751543 , 764415 , 789913 
This CL was uploaded by git cl split.

R=sky@chromium.org

Change-Id: I753c741a9194f489f072cd86d72407a1294150c3
Reviewed-on: https://chromium-review.googlesource.com/784937
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521416}
[modify] https://crrev.com/e1ccd58125c75660a7790724ea5faed78a055d0c/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16ac914c0806c5885ef112f621b5ae62d13fdbf3

commit 16ac914c0806c5885ef112f621b5ae62d13fdbf3
Author: Greg Thompson <grt@chromium.org>
Date: Tue Dec 05 10:24:50 2017

Revert "Enable interactive_ui_tests on Windows that now run mostly reliably."

This reverts commit e1ccd58125c75660a7790724ea5faed78a055d0c.

Reason for revert: Widespread "Check failed: false. Observers can only be added once!" failures.

Original change's description:
> Enable interactive_ui_tests on Windows that now run mostly reliably.
> 
> r516772 was the last of many commits that resolved a number of problems
> that made this test suite flaky on Windows. This CL enables tests that
> had been disabled over time due to flakes, yet now seem to run okay.
> 
> BUG= 133341 ,751031, 751543 , 764415 , 789913 
> This CL was uploaded by git cl split.
> 
> R=​sky@chromium.org
> 
> Change-Id: I753c741a9194f489f072cd86d72407a1294150c3
> Reviewed-on: https://chromium-review.googlesource.com/784937
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Greg Thompson <grt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#521416}

TBR=sky@chromium.org,grt@chromium.org

Change-Id: I33b192c628fdfebe9cfd93b71abd3b394faad9e5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  133341 , 751031,  751543 ,  764415 ,  789913 
Reviewed-on: https://chromium-review.googlesource.com/808204
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521652}
[modify] https://crrev.com/16ac914c0806c5885ef112f621b5ae62d13fdbf3/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

Components: Tests>Disabled
Labels: Test-Disabled

Sign in to add a comment