Closed Bug 1494471 Opened 3 years ago Closed 3 years ago

The URL autofill query incorrectly compares the origin frecency threshold to URL frecencies


(Toolkit :: Places, defect, P1)




Tracking Status
firefox-esr60 --- unaffected
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified


(Reporter: adw, Assigned: adw)



(Keywords: regression, Whiteboard: [fxsearch])


(5 files)

We broke this in bug 1467627.  Before that bug, the autofill frecency threshold was the max frecency of all URLs with a given origin -- i.e., it was the frecency of a single URL, albeit the one with the max frecency.  So it wasn't invalid to compare moz_places in the URL query to this threshold.  But that's not true anymore, and we should have fixed the URL query (somehow) as part of that bug.

I think what we should do is just drop the threshold for the URL query.  The threshold will still apply as the user is typing the origin.  But after that, we should just autofill the matching URL with the highest frecency as the user continues typing.  Especially because in that case there's no danger of autofilling something the user doesn't want autofilled.  This will be a simple patch we can easily uplift.

Marco, do you agree?
Flags: needinfo?(mak77)
Yeah, I had a few doubts about that, but didn't investigate. Your analysis and solution LGTM.
Flags: needinfo?(mak77)
Assignee: nobody → adw
Flags: qe-verify+
Attached file explain-select-1.txt
Attaching a couple of files that I'll discuss over in phabricator.
Attached file explain-select-2.txt
Attached file autofill-url-perf.txt
Posting some more data that I'll discuss in phabricator
Pushed by
Ignore the autofill threshold when autofilling URLs r=mak
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

1. New profile
2. Visit
3. Visit
4. Visit 10 times.  All you need to do is copy and paste the URL into the current tab, hit enter, and then 9 more times press Ctrl+L (or Command-L on a Mac) and enter.
5. In the urlbar, type "ny".  It should be autofilled to ""
6. Press the right arrow key so that the caret moves to the end of the text.  Continue typing "se".  It should be autofilled to "section/", and the entire text in the urlbar should be ""
7. Press the right arrow key so that the caret moves to the end of the text.  Continue typing "wo".  It should be autofilled to "world/", and the entire text in the urlbar should be ""
Attached patch Beta/63 patchSplinter Review
[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1239708

User impact if declined: The new autofill algorithm (bug 1239708) landed in 62, and this bug landed in 64, so if declined, users will need to wait another cycle for this fix. We've already fixed and uplifted at least two other autofill bugs recently.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Please see comment 10

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small patch that only slightly tweaks a SQL query used in autofill. Has test, and I've manually verified

String changes made/needed: None
Attachment #9016779 - Flags: approval-mozilla-beta?
I have reproduced this issue using Firefox 64.0a1 (2018.09.26) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 64.0a1 (latest nightly) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.13.6.
Comment on attachment 9016779 [details] [diff] [review]
Beta/63 patch

P1 regression, mlinimal patch that seemss afe, I am approving for 63RC1 as we are already past our beta cycle.
Attachment #9016779 - Flags: approval-mozilla-beta? → approval-mozilla-release+
I verified this issue using build 63.0 (build ID = 20181015152800; on Windows 10 x64 and Windows 7 x64, the issue is still reproducible, at step 5 from Comment 10, typing "ny" in URL bar not working the autofill to "".
Flags: needinfo?(adw)
Sorry, Nightly and beta/release have different initial bookmarks and start pages, so the frecency calculations are slightly different.  In the STR, in step 4, please visit only 3 times total.  The other steps are the same.

Also, step 7 has a typo: "world" shouldn't actually have a "/" at the end.
Flags: needinfo?(adw)
I can confirm this issue is fixed, I verified using 63.0RC build on Windows 10 x64, Ubuntu 16.04 x64 and on Mac OS X 10.13.6, based on Comment 16 I mark this bug verified fixed on Fx63.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.