1051830 - Unexpected in-the-middle completion in the urlbar (e.g. "go >> google.com")

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Andrei Eftimie]

We're seeing this via several mozmill tests (see bug 1051777)

Just started to fail because bug 1050683 stopped taking into consideration `browser.urlbar.delay`. With this pref set to 0 I was able to reproduce the issue and got bug 1040335 as a regressor (which enabled Unified Autocomplete).

----

Typing in the urlbar really fast (<25ms between characters) appends ">> google.com" in the urlbar.

I wasn't able to reproduce this issue manually, but this is reproducible via the mozmill test. We have a testcase as attachment 8470810 [details].

To run it:
1. Get the latest environment for your system from http://mozqa.com/mozmill-env/
2. Unzip, run the start script (or activate the environment via source bin/activate)
3. Run > mozmill -t testcase.js -b %path/to/firefox/binary%

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

[Tracking Requested - why for this release]:

Comment 2

[Marco Bonardo [:mak]]

I can reproduce by typing "go " in the urlbar, then I get go >> google.com, the trailing space seems to reproduce the bug every time.

this is definitely unexpected and should be fixed soon, possibly not later than the next iteration

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of bug

Comment 5

[Marco Bonardo [:mak]]

so, a first problem is with search engines completion, we should never autoFill if the string contains a space.
Basically before calling this._matchSearchEngineUrl() we should check this._shouldAutofill...

This solves the most common case with "text/s >> searchengine".

But it doesn't seem to apply to https://bugzilla.mozilla.org/show_bug.cgi?id=749923#c8 or https://bugzilla.mozilla.org/show_bug.cgi?id=749923#c9

the problem more generically is that what autoFill tries to suggest doesn't match the current input, and we hit the last else in nsAutocompleteController::CompleteValue. To avoid that we try to never suggest an autoFill value that doesn't match the current input, we failed to do so in the above case cause "go " doesn't match "google.com" but we still suggested google.com, so we ended up with " go  >> google.com".

The other cases may be due to us not cancelling the search fast enough. By the time autoFill returns, cancel() might have been called already, we should not return any result. I think adding an early return if (!this.pendingQuery) in _addMatch will solve this.

I'll attach a patch, in case I can't finish it, someone else could easily re-use this analysis.

Comment 6

[Marco Bonardo [:mak]]

Marco, could you please add this to the current iteration? my other bug is pending needinfo and I have a patch basically done.

Comment 7

[Marco Mucci [:MarcoM]]

Added to Iteration 34.2

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #6)
> Marco, could you please add this to the current iteration? my other bug is
> pending needinfo and I have a patch basically done.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1

Might eventually ask Blair if Tim is busy

Comment 9

[Marco Bonardo [:mak]]

Blair please note you will have to update your patch (sorry for bitrotting). Tests should catch eventual issues.
In case you want to steal the review, feel free to.

Comment 10

[Ekanan Ketunuti]

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #5)
> so, a first problem is with search engines completion, we should never
> autoFill if the string contains a space.

Possibly related to bug 767364?

Unfortunately the patch in comment 8 doesn't fix bug 767364.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Ekanan Ketunuti from comment #10)
> (In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #5)
> > so, a first problem is with search engines completion, we should never
> > autoFill if the string contains a space.
> 
> Possibly related to bug 767364?

no relation, no. that bug is also old and unrelated to unified complete.

Comment 12

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f8624be969ac

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8472843 [details] [diff] [review]
patch v1

Review of attachment 8472843 [details] [diff] [review]:
-----------------------------------------------------------------

Patch and tests look good, did you ensure the latter are failing without your patch applied?

::: toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@@ +28,5 @@
>  [test_swap_protocol.js]
>  [test_tabmatches.js]
>  [test_trimming.js]
>  [test_typed.js]
> +[test_avoid_middle_complete.js]

Should move that to the top a little to be sorted properly.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Tim Taubert [:ttaubert] from comment #13)
> Patch and tests look good, did you ensure the latter are failing without
> your patch applied?

yep, it's born to fail!

(In reply to Tim Taubert [:ttaubert] from comment #13)
> Should move that to the top a little to be sorted properly.

I did that locally 1 minutes after attaching, but I was too lazy to update the patch in the bug.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/2ccc5c9a58fa

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ccc5c9a58fa

Comment 17

[Andrei Vaida [:avaida]]

This is now verified fixed based on Bug 1051777 - Comment 16, per my conversation with :andrei_eftimie.

Comment 18

[Witold Cichecki]

Unfortunately this bug still persists.

Steps to reproduce (31.4.0):
1. Have two adressess collected in Address Book like:
xxxfroxxxx
yyyfroyyyy

2. Typing "fro" in TO: field and changing focus to message body leaves you with ugly search text instead of full name of the contact.

Output:
fro >> Full Name <[e-mail]>

Comment 19

[Marco Bonardo [:mak]]

that's a Thunderbird bug, unrelated to this change that only affects Firefox.

Comment 20

[Magnus Melin [:mkmelin]]

Indeed, and fixed for 31.5.0 with bug 1043310.