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

VERIFIED FIXED in Firefox 63

Status

()

enhancement
P1
normal
VERIFIED FIXED
8 months ago
a month ago

People

(Reporter: danibodea, Assigned: adw)

Tracking

(Depends on 2 bugs, Regressed 1 bug)

Trunk
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 unaffected, firefox63 verified, firefox64 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Reporter

Description

8 months ago
[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.
Reporter

Updated

8 months ago
Severity: normal → enhancement
See Also: → 1488879
Duplicate of this bug: 1493543
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.
Flags: needinfo?(adw)
Priority: -- → P1
Whiteboard: [fxsearch]

Comment 3

8 months ago
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.
Axel, I'm sorry, reopening your report and keeping it separate, it looks like I misread your log.
Assignee

Updated

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

Comment 5

8 months ago
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.
(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.
Assignee

Comment 7

8 months ago
Ah yes, you're right
Assignee

Comment 9

8 months ago
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.
Attachment #9014235 - Flags: feedback?(mak77)
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
Attachment #9014235 - Flags: feedback?(mak77) → feedback+
Assignee

Comment 11

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

Comment 13

8 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9c7d14fc4a9
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak
Assignee

Updated

8 months ago
Flags: needinfo?(adw)

Comment 15

8 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/237c55ddb112
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/237c55ddb112
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee

Comment 17

8 months ago
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.
Flags: qe-verify+
Assignee

Comment 18

8 months ago
Posted patch Beta/63 patchSplinter Review
[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
Attachment #9015100 - Flags: approval-mozilla-beta?
Assignee

Updated

8 months ago
Summary: Improve the auto-fill process of the address bar, considering bookmarked pages → Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold

Comment 19

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

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

8 months ago
I have tested as well, it is working back again in Firefox 64.0a1. Thank you!
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.
Attachment #9015100 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 24

8 months ago
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!
Status: RESOLVED → VERIFIED
Assignee

Updated

8 months ago
Duplicate of this bug: 1489763

Comment 26

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

Comment 27

8 months ago
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!
Flags: needinfo?(maarnuwels)

Comment 28

8 months ago
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).
Flags: needinfo?(maarnuwels)

Comment 29

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

Comment 30

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

We do handle both http and https.
Reporter

Comment 31

8 months ago
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!
Flags: qe-verify+ → needinfo?(maarnuwels)

Comment 32

8 months ago
If it would work in safe mode; how would I then determine what is causing the issue?
Flags: needinfo?(maarnuwels)

Comment 33

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

Comment 34

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

Comment 35

8 months ago
Safe mode isn't working either for me, I'll now see if a new profile is working.
Depends on: 1502666
Depends on: 1503293
Depends on: 1502821
No longer depends on: 1503293
Regressions: 1503293
You need to log in before you can comment on or make changes to this bug.