Closed Bug 1494471 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(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
Status: NEW → ASSIGNED
Flags: qe-verify+
Attached file explain-select-1.txt (deleted) —
Attaching a couple of files that I'll discuss over in phabricator.
Attached file explain-select-2.txt (deleted) —
Attached file autofill-url-perf.txt (deleted) —
Posting some more data that I'll discuss in phabricator
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/94bc301b5bd4 Ignore the autofill threshold when autofilling URLs r=mak
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
STR: 1. New profile 2. Visit https://www.nytimes.com/ 3. Visit https://www.nytimes.com/section/world 4. Visit http://example.com/ 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 "nytimes.com/" 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 "nytimes.com/section/" 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 "nytimes.com/section/world/"
Attached patch Beta/63 patch (deleted) — Splinter 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; https://archive.mozilla.org/pub/firefox/candidates/63.0-candidates/build1/win64/en-US/) 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 "nytimes.com/".
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 example.com 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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: