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

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: adw, Assigned: adw)

Tracking

({regression})

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 wontfix, firefox63 verified, firefox64 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(5 attachments)

(Assignee)

Description

8 months ago
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)

Updated

8 months ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
(Assignee)

Updated

8 months ago
Flags: qe-verify+
(Assignee)

Comment 4

7 months ago
Attaching a couple of files that I'll discuss over in phabricator.
(Assignee)

Comment 5

7 months ago
(Assignee)

Comment 6

7 months ago
Posting some more data that I'll discuss in phabricator

Comment 8

7 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94bc301b5bd4
Ignore the autofill threshold when autofilling URLs r=mak

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94bc301b5bd4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Comment 10

7 months ago
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/"
(Assignee)

Comment 11

7 months ago
Posted 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; 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)
(Assignee)

Comment 16

7 months ago
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.