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

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[Drew Willcoxon :adw]

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?

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yeah, I had a few doubts about that, but didn't investigate. Your analysis and solution LGTM.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1494471 - Ignore the autofill threshold when autofilling URLs

Comment 3

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25d5bc2f9540de9a8882b39015b9258bc385a326

Comment 4

[Drew Willcoxon :adw]

Attachment: explain-select-1.txt

Attaching a couple of files that I'll discuss over in phabricator.

Comment 5

[Drew Willcoxon :adw]

Attachment: explain-select-2.txt

Comment 6

[Drew Willcoxon :adw]

Attachment: autofill-url-perf.txt

Posting some more data that I'll discuss in phabricator

Comment 7

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a295fa3214f5de64b7a8dfb244f4e71f9266a85

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94bc301b5bd4
Ignore the autofill threshold when autofilling URLs r=mak

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/94bc301b5bd4

Comment 10

[Drew Willcoxon :adw]

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/"

Comment 11

[Drew Willcoxon :adw]

Attachment: Beta/63 patch

[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

Comment 12

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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 13

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/0a9b0f637ecb
https://hg.mozilla.org/releases/mozilla-beta/rev/33d3fba56989f5782602063b40c35dd3742c0e59 (FIREFOX_63b_RELBRANCH)

Comment 15

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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/".

Comment 16

[Drew Willcoxon :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.

Comment 17

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.