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

VERIFIED FIXED in Firefox 34

Status

()

Toolkit
Places
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Andrei Eftimie, Assigned: mak)

Tracking

({regression})

unspecified
mozilla34
regression
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox34+ verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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%
[Tracking Requested - why for this release]:
status-firefox34: --- → affected
tracking-firefox34: --- → ?
Keywords: regression
(Assignee)

Comment 2

3 years ago
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
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1053229
(Assignee)

Updated

3 years ago
Summary: Unexpected in-the-middle completion in the urlbar → Unexpected in-the-middle completion in the urlbar (e.g. "go >> google.com")
Created attachment 8472437 [details]
Screenshot of bug
(Assignee)

Comment 5

3 years ago
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED

Updated

3 years ago
QA Whiteboard: [qa+]
(Assignee)

Comment 6

3 years ago
Marco, could you please add this to the current iteration? my other bug is pending needinfo and I have a patch basically done.
Points: --- → 5
Flags: needinfo?(mmucci)
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.
Iteration: --- → 34.2
Flags: needinfo?(mmucci)
(Assignee)

Comment 8

3 years ago
Created attachment 8472843 [details] [diff] [review]
patch v1

Might eventually ask Blair if Tim is busy
Attachment #8472843 - Flags: review?(ttaubert)
(Assignee)

Comment 9

3 years ago
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.
Flags: needinfo?(bmcbride)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
(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.
tracking-firefox34: ? → +
(Assignee)

Comment 12

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f8624be969ac
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.
Attachment #8472843 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 14

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/2ccc5c9a58fa
Flags: in-testsuite+
Target Milestone: --- → mozilla34
https://hg.mozilla.org/mozilla-central/rev/2ccc5c9a58fa
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
QA Contact: alexandra.lucinet
Flags: needinfo?(bmcbride)
QA Contact: alexandra.lucinet → andrei.vaida
This is now verified fixed based on Bug 1051777 - Comment 16, per my conversation with :andrei_eftimie.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
status-firefox34: fixed → verified
(Reporter)

Updated

3 years ago
Depends on: 1043923

Comment 18

3 years ago
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 <address@example.com>
(Assignee)

Comment 19

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

Comment 20

3 years ago
Indeed, and fixed for 31.5.0 with bug 1043310.
You need to log in before you can comment on or make changes to this bug.