1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement, P1)

Description

[Daniel Bodea [:danibodea]]

[Note]:
- This is a followup on bug 1488879, based on comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c36, using the STR from comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c49 and considering the user's dissatisfaction from bug's comments, the process of the auto-filling of the address bar with previously saved bookmarks is not working as expected. It appears that a sub-groups of users would like bookmarks to get auto-filled by default, indifferent to the frequency of the bookmark's visitation (as explained in comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c35).


[Affected versions]:
- 64.0a1, 63.0b9, 62.0.2

[Affected platforms]:
-All

[Steps to reproduce]:
1. Clear browser history and uncheck "remember my history"
2. Save two bookmark of different domains, https://www.mozilla.org/ and https://www.youtube.com/ (autofill should work properly for now)
3. Save another bookmark to https://www.youtube.com/user/Mozilla
4. Type "moz" in the address bar. The URL should be auto-filled as mozilla.org, but it does not.
5. Type "you" in the address bar. 

[Expected result]:
- The URL should is auto-filled as youtube.com; any of the bookmarks should get auto-filled.

[Actual result]:
- It turns out If I have two or more bookmarks of the same domain, other bookmarks of unique domains will not be auto-filled.

Comment 2

[Marco Bonardo [:mak]]

Unfortunately if history is disabled we don't have the last time a bookmark was accessed, the only data we could rely on is the decayed frecency, though it may not be that good (because maybe I created a bookmark 10 years ago, but I use it everyday). It was working before because we were not clearing the typed information with history, that is also wrong.

We could distinguish 2 cases:
1. history is enabed: use the normal threshold
2. history is disabled, or clearonshutdown is enabled: autofill bookmarks
The problem is in the middle, what if history is enabled but the user clears history completely? From that point on, nothing gets autofilled anymore.

Maybe in the end the simplest path is what you suggested, just always autofill bookmarks. Though, that requires a performant way to figure out if an origin is bookmarked.

Comment 3

[Axel]

I don't get it: When the "browsing and download history" checkbox is unchecked, the browsing and download history is NOT supposed to get deleted. That's the reason for this checkbox, as far as I apprehend.

Comment 4

[Marco Bonardo [:mak]]

Axel, I'm sorry, reopening your report and keeping it separate, it looks like I misread your log.

Comment 5

[Drew Willcoxon :adw]

I still think exempting bookmarks from the autofill frecency threshold is probably the right thing to do, but I'd also like to test/investigate why exactly they aren't getting autofilled in these cases -- and there are slightly different cases being reported.  As long as your history is 100% cleared, then the threshold should be zero, so bookmarks *should* be autofilled since we fixed bug 1488879.  Once you start visiting URLs after that, if you are not in permanent private browsing mode, then the threshold will start to go up, and in that case it's clear why unvisited bookmarks would not autofill.  Exempting bookmarks is the fix for that.  But, if you are in permanent private browsing mode, theoretically autofilling bookmarks should Just Work, because presumably frecencies never change, so the threshold remains at zero.  I need to look into how that works.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #5)
> I still think exempting bookmarks from the autofill frecency threshold is
> probably the right thing to do, but I'd also like to test/investigate why
> exactly they aren't getting autofilled in these cases -- and there are
> slightly different cases being reported.  As long as your history is 100%
> cleared, then the threshold should be zero

I'm not sure about this.
https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/toolkit/components/places/SQLFunctions.cpp#832
a bookmark, even if unvisited, won't have a zero frecency, and as such I'd expect the threshold to not be zero.

Comment 7

[Drew Willcoxon :adw]

Ah yes, you're right

Comment 8

[Drew Willcoxon :adw]

Attachment: Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 9014235 [details]
Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold.

Marco, this just unconditionally includes the bookmarked subquery in the main query, and then does `HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD} OR bookmarked`.  This seems like a simple fix that we can uplift to address the problem immediately, as opposed to a more comprehensive fix that we've briefly discussed that possibly involves a schema change.  IMO we could take this now but still continue to evaluate a more performant fix involving a schema change.  However, we do have an index on moz_places.origin_id, so maybe there's not much of a perf win to be had here?  And obviously we already use this exact query when sugest.history is disabled, but we'd be extending that perf to all users with this, not only those with suggest.history=false.

What do you think?  I know you already had some comments on this in a similar patch in bug 1489060 comment 7, but here I'm not doing the extra sorting.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 9014235 [details]
Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold.

off-hand it sounds ok to me, there is a small perf hit but it looks acceptable

Comment 11

[Drew Willcoxon :adw]

Thanks.  This bitrots bug 1494471, but I'll handle that one after this one.

Comment 12

[Drew Willcoxon :adw]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9c7d14fc4a9
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak

Comment 14

[Andrei Ciure[:aciure]]

Backed out 1 changesets (bug 1493636) for high frequency test_escape_autocomplete.py failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=203766661&revision=e9c7d14fc4a9360b2a4ae94636aa8fe65b16ed72

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&searchStr=en-us

backout: https://hg.mozilla.org/integration/autoland/rev/9c9ef9cced433d48a75465f4a9245206d073b8c2

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/237c55ddb112
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/237c55ddb112

Comment 17

[Drew Willcoxon :adw]

Please see comment 0 for one set of STR.

Additional STR, similar to comment 0 but not the same:

1. Save two bookmarks of different domains, https://www.mozilla.org/ and https://www.youtube.com/
2. Save another bookmark, https://www.youtube.com/user/Mozilla
3. Clear history (using "last hour" or "everything", shouldn't matter)
4. Type "moz" in the address bar. The URL should be auto-filled as mozilla.org.
5. Type "you" in the address bar. The URL should be auto-filled as youtube.com.

Comment 18

[Drew Willcoxon :adw]

Attachment: Beta/63 patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1239708

User impact if declined: Bug 1239708 (new autofill algorithm) landed in 62, and this bug landed in 64. So if it's not uplifted to 63, users will have to wait another cycle before they see this bug fixed.

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 17

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch makes slight tweaks to two SQL queries used in autofill. This code has automated tests, and I've verified manually. Most of this patch is test additions/changes.

String changes made/needed: None

Comment 19

[u624563]

I've just installed Nightly to test it out and I can confirm that autofill works once again, great! Looking forward to the backport to 63 beta.

Comment 20

[Amantu Khan]

I've successfully reproduced this bug on Firefox 63.0a9 and I can Confirm Autofill is working back again on Firefox 64.0a1.

Comment 21

[raja.muniappan]

I have tested as well, it is working back again in Firefox 64.0a1. Thank you!

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9015100 [details] [diff] [review]
Beta/63 patch

Follow up fix to bug 1488879 which was a tracked regression for 63 that already got uplifted, fix verified on Nightly, uplift approved for 63 beta 13, thanks.

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/29b5c763db45

Comment 24

[Daniel Bodea [:danibodea]]

The STR from comment 0 and comment 17 were attempted and the results are promising: This issue is now fixed in Nightly v64.0a1 from 2018-10-08.
Awaiting Beta build for uplift verification. 
Thank you!

Comment 26

[u624563]

I've been testing it out properly with beta 13 and I'm sad to report that it's not completely fixed yet.
The autofill works the first time when you haven't visited any pages on the bookmarked page yet.
Once you visit some pages on the bookmarked page (tabs or no tabs doesn't seem to matter) and you then type in the name of the bookmarked page again it's no longer autofilling anything.

Comment 27

[Daniel Bodea [:danibodea]]

The test cases in comment 0 and comment 17 (with both history enabled and not) are passed (as expected) in Firefox Beta v63.0b13, on my test machine with Windows 10 x64.

@maarnuwels: I could not reproduce your case and I've tried this STR:
1. Clear browser history and set browser not to "remember my history";
2. Save two bookmarks of different domains, https://www.mozilla.org/ and https://www.youtube.com/;
3. Save another bookmark, https://www.youtube.com/user/Mozillam;
4. Navigate to several different child pages of the https://www.mozilla.org/ and https://www.youtube.com/ domains;
5. Type "moz" in the address bar. -> The URL is auto-filled as mozilla.org as expected.
6. Type "you" in the address bar. -> The URL is auto-filled as youtube.com as expected.

Are my steps different from yours? If yes, please write detailed steps to reproduce with actual and expected results that will show the issue you are experiencing.

Thank you for your contribution!

Comment 28

[u624563]

https://bugzilla.mozilla.org/show_bug.cgi?id=1493636#c26

I have many bookmarks set.
As an example, I'm using http://tweakers.net (which I also have as a homepage).
After manually clearing history, if I enter tweak, it autofills OK.
I then visit the homepage, i.e., just open the bookmark in same tab.
Then I try to autofill tweakers again (with 'tweak' again), and tweakers.net isn't autofilled anymore.

Actually, while I'm writing this, I think I found the problem.
Most of my bookmarks are still http:// (been a Firefox user since 1.0!), but most sites nowadays use https:// of course and redirect to it.
It seems that all bookmarks set to http:// will break autofill after the page has been loaded once. If the bookmark is https://, it seems to work OK.
I think this issue needs to be addressed. Not on my end as it used to work fine in previous Firefox version (+ migrating all bookmarks to https:// is no option for me, I have about 2500).

Comment 29

[u624563]

*visit the homepage -> visit the filled bookmark. I can't find out how to edit my previous comment.

Comment 30

[Drew Willcoxon :adw]

Sorry, I can't reproduce that.  Once I bookmark http://tweakers.net, tweakers.net always autofills.

We do handle both http and https.

Comment 31

[Daniel Bodea [:danibodea]]

I have tested your presumption that your issue could be related to having bookmarks saved with http instead of https but I still can't reproduce it. Considering that you are performing the tests in your old profile, it may be caused by add-ons or extensions. 

Please test if the issue is reproducible in safe mode, here is a link that can help you:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

If you still have the issue please create a new profile, you have the steps here: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager
and retest the issue. These tests are necessary to know whether the issue is linked to extensions or profile related settings.

I will have to mark this issue as verified, but let's continue talking on this thread until we will hopefully find the cause.

Thank you!

Comment 32

[u624563]

If it would work in safe mode; how would I then determine what is causing the issue?

Comment 33

[u624563]

I just disabled places.sqlite and so far so good. Shouldn't that file be reset if you clear history on close?

Comment 34

[u624563]

It's no longer working again, sadly. I'll investigate this in safe mode too.

Comment 35

[u624563]

Safe mode isn't working either for me, I'll now see if a new profile is working.