739751 - in-the-middle autocomplete unexpectedly appears in urlbar ("searchstring >> full string")

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[:Margaret Leibovic]

I find this happens with these same steps, regardless of search string/domain:

1) type a few characters (e.g. "dr")
2) press right arrow to autocomplete domain (results in "http://dropbox.com/" in urlbar)
3) press space (results in "http://dropbox.com/  >> dropbox.com/" in urlbar)

If I press the right arrow again (or press enter to submit the search) it does not replace my inital search string, it just searches for "http://dropbox.com/  >> dropbox.com/".

Comment 1

[Marco Bonardo [:mak]]

I'll start by making a testcase, though if anyone wants to steal this, it's free, I'm just assigning to not lose it.

Comment 2

[Marco Bonardo [:mak]]

This is the first issue:

1. you are searching for "dropbox.com/ " where the actual value is "dropbox.com/". 2. The search trims the search term and returns "dropbox.com/"
3. nsAutoCompleteController::CompleteValue does a StringBeginsWith(aValue, mSearchString), this fails cause searchstring is longer
4. it then enters the else, that tries to extract a scheme, but we trimmed out http, so it just decides we are completing a simple word from the middle.

still investigating what happens when you confirm.

Comment 3

[Marco Bonardo [:mak]]

looks like we enter GetDefaultCompleteValue with aResultIndex == -1, we try to loop through mResults, but it's empty

Comment 4

[Marco Bonardo [:mak]]

hm, nvm comment 3. in the end it's just that we add a match shorter than the searchstring and that confuses completeDefaultIndex.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I think makes sense to avoid trying to inline autocomplete anything with spaces, since we are autocompleting urls, this ensures we don't end up uselessly tokenizing and then passing fancy results back to completeDefaultValue.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

oops the comment was a bit unclean.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 610920 [details] [diff] [review]
patch v1.1

>diff --git a/toolkit/components/places/nsPlacesAutoComplete.js b/toolkit/components/places/nsPlacesAutoComplete.js

>+    // Don't try to autofill if the search term includes any whitespace.
>+    // This may confuse completeDefaultIndex cause the MATCH_AUTOCOMPLETE
>+    // tokenizer ends up trimming the search string and returning a value
>+    // that doesn't match it, or is even shorter.

What is MATCH_AUTOCOMPLETE? This solution seems reasonable, but I have to admit I still don't fully understand the cause of the bug. Should we also have a more complete solution that fixes this problem more generally (in the controller?).

Comment 8

[Marco Bonardo [:mak]]

MATCH_AUTOCOMPLETE is a SQL function that takes search terms, tokenize them, and compare them to url, tags and title.
The controller is doing the right thing, it is passing us the search string, and would be wrong for it to strip spaces or tokenize, since it can't know what kind of resources the search is searching.
I don't think controller can make any assumption on the search, some search may want to retain and search for spaces.
The problem here is that the specific search applies transformation to the search string, and that transformation ends up returning a result that is shorter than the search string, that doesn't make sense.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 610920 [details] [diff] [review]
patch v1.1

r=me with the comment fixed to mention AUTOCOMPLETE_MATCH

Comment 10

[Marco Bonardo [:mak]]

fixed the comment and pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6527fdc59f4

Comment 11

[Cyko_01]

seems to have caused a 12.1% regresssion of JM on kraken-astar

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a6527fdc59f4