Backspacing quickly in Omnibox with tail suggestions triggers DCHECK |
||||
Issue descriptionChrome Version: 60.0.3087.0 (Developer Build) (64-bit) OS: Linux What steps will reproduce the problem? (1) You must point the browser at something which will provide tail suggestions. The only thing that will currently do this is a custom GWS server. (2) Enter "blah blah token game" in the Omnibox. It should provide one inconsequential tail suggestion. (3) Hit backspace 5 times. What is the expected result? It should only remove " game". What happens instead? The browser crashes. Does not appear to happen with traditional suggestions. Relevant stack trace: #9 0x7f43191ab432 gfx::RenderText::TextIndexToGivenTextIndex() DCHECK_LE(index, text().length()); #10 0x7f4319180694 gfx::RenderTextHarfBuzz::TextIndexToDisplayIndex() #11 0x7f431917efe4 gfx::RenderTextHarfBuzz::GetRunContainingCaret() #12 0x7f431917ecfe gfx::RenderTextHarfBuzz::GetGlyphBounds() #13 0x7f4320179aa4 OmniboxResultView::GetDisplayOffset() #14 0x7f432017945d OmniboxResultView::DrawRenderText() #15 0x7f4320178f53 OmniboxResultView::PaintMatch() #16 0x7f432017ab1a OmniboxResultView::OnPaint()
,
May 1 2017
Also reproducible if, while tail suggestions displayed, you hit ^A and a character. ^A plus backspace does not display problem.
,
May 2 2017
First guess: BaseSearchProvider::CreateSearchSuggestion - it handles tail suggest differently, and it builds the index that leads to the DCHECK. Note that that code is concerned with suggestions.length and contents.length, while GetDisplayOffset is concerned with the text stored as additional info kACMatchPropertyInputText
,
May 2 2017
Note: Why do we make this RVG? It requires user interaction in the Omnibox to cause a DCHECK violation (will it crash in Release at all?), and doesn't look like a remote exploit vector.
,
May 2 2017
I only did that because it discussed a coming feature - that is, the bug didn't exist externally - and I was playing it safe. Happy to remove it.
,
May 2 2017
,
May 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6737bb033e394fa654ef8ac2faa12306aaf062d commit c6737bb033e394fa654ef8ac2faa12306aaf062d Author: krb <krb@chromium.org> Date: Thu May 11 13:34:45 2017 [omnibox] Use suggestion instead of input for base text Tail suggestions want to line up the suggested completion e.g. "e" or "f" after the input text that they are completing "a b c d": a b c d ------- a b c d e a b c d f but they replace the some of the input text with an ellipsis: a b c d ... d e ... d f To line up the text correctly, the render code needs to know the text that is missing e.g. "a b c" (since the ellipsis is much shorter.) The code was passing a numerical index just past the "a b c", but was passing the input text, which might not be the prefix of the suggestion. (Suggestions can be stale, albeit temporarily.) This change replaces sending the input text with the actual text that the index was calculated from (the suggestion) guaranteeing that the index is valid. BUG= 717152 Review-Url: https://codereview.chromium.org/2854423002 Cr-Commit-Position: refs/heads/master@{#470941} [modify] https://crrev.com/c6737bb033e394fa654ef8ac2faa12306aaf062d/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm [modify] https://crrev.com/c6737bb033e394fa654ef8ac2faa12306aaf062d/chrome/browser/ui/views/omnibox/omnibox_result_view.cc [modify] https://crrev.com/c6737bb033e394fa654ef8ac2faa12306aaf062d/components/omnibox/browser/autocomplete_match.h [modify] https://crrev.com/c6737bb033e394fa654ef8ac2faa12306aaf062d/components/omnibox/browser/base_search_provider.cc [modify] https://crrev.com/c6737bb033e394fa654ef8ac2faa12306aaf062d/components/omnibox/browser/base_search_provider_unittest.cc
,
May 11 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by k...@chromium.org
, May 1 2017