Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled

VERIFIED FIXED in Firefox 63

Status

()

P1
normal
VERIFIED FIXED
3 months ago
a month ago

People

(Reporter: zygozenith, Assigned: adw)

Tracking

({regression})

62 Branch
Firefox 64
regression
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136

Steps to reproduce:

1) Have firefox to only search my bookmarks
2) Make 'www.reddit.com' and 'www.amazon.com' a bookmark
3) Set 'browser.urlbar.matchBehavior' to a value of 3
4) Type 'r' or 'a' in the urlbar (awesomebar?)


Actual results:

Nothing shows up.


Expected results:

Before Firefox 62, typing 'r' alone will show up "www.reddit.com" from my bookmark in the url bar, typing 'a' will show up "www.amazon.com", and so on.

But after Firefox 62, only by typing 'www.r' will "www.reddit.com" then show up, and typing "www.a" will 'www.amazon.com' then show up.
status-firefox62: --- → affected
Component: Untriaged → Address Bar
(Assignee)

Comment 1

2 months ago
Thanks for filing this.

I don't think this has anything to do with matchBehavior.  Rather, there's a problem with the SQL query we use when we search only bookmarked places.

bookmarkedFragment is here: https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/toolkit/components/places/UnifiedComplete.js#332

It's used here: https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/toolkit/components/places/UnifiedComplete.js#302

The problem (I think) is that that SELECT is aggregate due to both the TOTAL(frecency) column and the GROUP BY.  But bookmarkedFragment is not aggregate.  So it ends up looking only at whatever moz_place ends up first in its result set.  If that moz_place's foreign_count is zero, then the whole bookmarkedFragment is zero, even if there are moz_places in the result set that are bookmarked.  I confirmed this by running some SQL in sqlite3.

When I change bookmarkedFragment to use TOTAL, things seem to work:

  `(SELECT TOTAL(foreign_count) > 0 FROM moz_places
    WHERE moz_places.origin_id = moz_origins.id)`
Assignee: nobody → adw
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
(Assignee)

Updated

2 months ago
Blocks: 1239708
(Assignee)

Comment 2

2 months ago
And the reason this used to work (on 61 and earlier) is that the query was not an aggregate query: https://hg.mozilla.org/mozilla-central/annotate/c58f0b4dd849/toolkit/components/places/UnifiedComplete.js#l266

So it just found all the matching moz_places, ordered them, and took the top one.
Keywords: regression
(Assignee)

Comment 3

2 months ago
Actually, looks like I broke this in bug 1469651, not the original autofill bug.  I'll update the blocking status accordingly.  The changeset is: https://hg.mozilla.org/mozilla-central/rev/cb9a1ff4a249

Also updating the bug summary.  STR for this bug are to set browser.urlbar.suggest.bookmark = true (the default) and browser.urlbar.suggest.history = false (not the default).
Blocks: 1469651
No longer blocks: 1239708
Summary: browser.urlbar.matchBehavior = 3 changed to include 'www.' since Firefox 62 → Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled
[Tracking Requested - why for this release]:

Unsure if we need to track this for a dot release, but putting it on the radar.
status-firefox63: --- → affected
status-firefox64: --- → affected
tracking-firefox62: --- → ?
(Assignee)

Comment 5

2 months ago
Created attachment 9010475 [details]
Bug 1489060 - Tweak how autofill works with bookmarks.
(Assignee)

Comment 6

2 months ago
Marco, could you give f? on the approach in the WIP revision please?  (There's no Phabrictor f?, right?)
Flags: needinfo?(mak77)

Comment 7

2 months ago
I don't think exempting bookmarks from the threshold is a good idea, as many other users I have bookmarks I created years ago and never cleaned up, we don't even have something like a cleanup tool, we'd end up filling things the user really doesn't care about anymore. I'd be ok to do that only if history is disabled (permanent pb), then we are surely lacking data for the feature to work as expected, and we could enlarge the dataset only for that specific case.

"First sorts by the total frecency for all origins with a given host, and then sorts by frecency of individual origins. This is maybe how this should have worked from the beginning. This also breaks tests which is why this patch is a WIP right now."

This sounds expensive (sorting on a grouped column, that clearly doesn't have any kind of index), and the patch includes too many fixes (3 issues) to be clear about what it is trying to achieve. Is this change necessary to fix this bug or is it a nice-to-have? What are the alternatives? (like, could we have a bookmarked field in moz_origins, or use a better subselect in the external query for bookmarked?)
Flags: needinfo?(mak77)
(Assignee)

Comment 8

2 months ago
OK.  My thinking about exempting bookmarks is that a bookmark is a clear expression by the user that they're interested in a URL, regardless of its current frecency.  The point of the threshold is to exclude URLs that the user probably is not interested in.  Bookmarks don't fit that description IMO.

(In reply to Marco Bonardo [::mak] from comment #7)
> be clear about what it is trying to achieve. Is this change necessary to fix
> this bug or is it a nice-to-have? What are the alternatives?

It's just something I realized while working on this patch, since all these questions involve the same query.

I'll pare the patch down to something that just fixes this bug.

Comment 9

2 months ago
(In reply to Drew Willcoxon :adw from comment #8)
> OK.  My thinking about exempting bookmarks is that a bookmark is a clear
> expression by the user that they're interested in a URL, regardless of its
> current frecency.  The point of the threshold is to exclude URLs that the
> user probably is not interested in.  Bookmarks don't fit that description
> IMO.

The point of the threshold is also to exclude urls that are no more relevant when user browsing habits change, if we'd consider relevant a bookmark created 5 years ago, we could just remove the threshold completely and go back to the previous situation where things just got stuck there forever.
I'm not saying the status-quo is perfect, but I also don't like the idea of going the total opposite direction.

Comment 10

2 months ago
But not autofilling anything when there's just one suggestion/bookmark available is not a good design choice. I have search suggestions disabled and clear history on shutdown, it's completely broken.
There are various problems here, and what I suggested in comment 9 is probably not perfect, because when history is missing, any useful information about bookmarks is lost as well.
I suspect in the end what Drew suggested, to autofill bookmarks regardless, is the only simple path forward. Though, I'm not sure currently we can do that in a performant way.
See Also: → bug 1493543

Updated

2 months ago
Whiteboard: [fxsearch]

Updated

2 months ago
Attachment #9010475 - Attachment is obsolete: true
(Assignee)

Comment 12

2 months ago
Created attachment 9011995 [details]
Bug 1489060 - Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled

In the origin query, the SELECT is aggregate due to both the TOTAL(frecency) column and the GROUP BY, but bookmarkedFragment is not aggregate. So it ends up looking only at whatever moz_place ends up first in its result set. If that moz_place's foreign_count is zero, then the whole bookmarkedFragment is zero, even if there are moz_places in the result set that are bookmarked.

Change bookmarkedFragment to use TOTAL.
Comment on attachment 9011995 [details]
Bug 1489060 - Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled

Marco Bonardo [::mak] has approved the revision.
Attachment #9011995 - Flags: review+

Comment 14

2 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35b71d287e96
Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled r=mak
(Assignee)

Comment 15

2 months ago
Please see comment 0 for STR
Flags: qe-verify+

Comment 16

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35b71d287e96
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Marco, do you want to nominate it for Beta backport?
Flags: needinfo?(mak77)
(Assignee)

Comment 18

2 months ago
Yes
Flags: needinfo?(mak77)
(Assignee)

Comment 19

2 months ago
Created attachment 9013675 [details] [diff] [review]
Beta/63 patch

Approval Request Comment
[Feature/Bug causing the regression]:
New autofill algorithm, bug 1239708, which landed in 62

[User impact if declined]:
Users will see broken behavior (this bug) in 62 and 63 instead of only 62

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Not yet

[Needs manual test from QE? If yes, steps to reproduce]:
STR are in comment 0

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Small two-line change to a SQL query that's used in the specific case described in comment 0 (searching history is disabled, searching bookmarks is enabled)

[String changes made/needed]:
None
Attachment #9013675 - Flags: approval-mozilla-beta?
Comment on attachment 9013675 [details] [diff] [review]
Beta/63 patch

Review of attachment 9013675 [details] [diff] [review]:
-----------------------------------------------------------------

Fix for a P1 regression, uplift approved for 63 beta 12, thanks.
Attachment #9013675 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 21

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/79aae3b724a5
status-firefox63: affected → fixed
I'm going to call this wontfix for 62 as 63 is less than 3 weeks away.
status-firefox62: affected → wontfix
status-firefox-esr60: --- → unaffected
tracking-firefox62: ? → -

Comment 23

2 months ago
I was able to reproduce this bug on Fx62 (Build ID: 20180830143136) with some URLs (e.g. amazon.com) only. 

The code fix seems to work fine on the Latest nightly on Windows 10. On mac OS and Ubuntu 16.04 I still see the issue. 

Please see on Win 10 (Build ID 	20181003100127) where code fix is working fine.
https://svtest2222-gmail.tinytake.com/sf/Mjk2ODYzM184OTA2NDYy

I tried testing CI/treeherder beta build (https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=3947b51293f6f3ddd8173b263cee69f7e4850a45&selectedJob=203066624) and still see the issue

https://svtest2222-gmail.tinytake.com/sf/Mjk2ODY1MF84OTA2NDc5 (I see same issue on the Mac OS 10.12, Ubuntu 16.04 with the latest Nightly ((Build ID 	20181003100127))

Please let me know if I am missing anything here. Thanks!
Flags: needinfo?(adw)
(Assignee)

Comment 24

2 months ago
You're probably hitting the autofill frecency threshold, which still comes into play for bookmarks and this bug.  I tried following the steps shown in your videos (thanks for posting them) using Nightly on macOS, and I too saw that the problem still happens for amazon.com but not for reddit.com.  However, if you then visit amazon.com a couple of more times, it starts autofilling.

To eliminate the unrelated issue of frecency so that you can verify this bug, you can Clear Recent History (using the default options), and then both amazon.com and reddit.com should autofill.  (They do for me.)

We have another bug about always autofilling bookmarks regardless of frecency and the threshold, bug 1493636.
Flags: needinfo?(adw)

Comment 25

2 months ago
(In reply to Drew Willcoxon :adw from comment #24)
> You're probably hitting the autofill frecency threshold, which still comes
> into play for bookmarks and this bug.  I tried following the steps shown in
> your videos (thanks for posting them) using Nightly on macOS, and I too saw
> that the problem still happens for amazon.com but not for reddit.com. 
> However, if you then visit amazon.com a couple of more times, it starts
> autofilling.
> 
> To eliminate the unrelated issue of frecency so that you can verify this
> bug, you can Clear Recent History (using the default options), and then both
> amazon.com and reddit.com should autofill.  (They do for me.)
> 
> We have another bug about always autofilling bookmarks regardless of
> frecency and the threshold, bug 1493636.

Thanks for this info! Things are better after clearing history but sometimes after clearing history I have to visit website a couple of more times to get it autofilling.

https://svtest2222-gmail.tinytake.com/sf/Mjk3Mjg3OF84OTE3MjQ0

https://svtest2222-gmail.tinytake.com/sf/Mjk3Mjg5NF84OTE3MjYx
  
Apart from the known issue, I verified this bug fix bug for Nightly ( Build ID 	20181004100222) and CI beta build (Version 63.0b12, Build ID 20181003092230)on Windows 10, Mac OS 10.12 and Ubuntu 16.04
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
status-firefox64: fixed → verified
Flags: qe-verify+
(Assignee)

Comment 26

2 months ago
Thanks for the videos again, they're very helpful.  I don't see anything unexpected in them.  As you visit amazon and reddit, their frecencies change, and so does the autofill frecency threshold.  For example when you visit amazon first at the beginning of the videos, it increases the threshold so that reddit doesn't meet it and it therefore isn't autofilled.  Once you start visiting reddit, its frecenc goes up and the threshold changes again.  When we're testing with new profiles (or almost new profiles), we're dealing with a very small number of pages in history and a small number of visits, so any visit will have a large effect on the threshold and possibly (or probably) exclude some other page from being autofilled.

As for the "never remember history" option, choosing it doesn't clear your current history, so whatever threshold and frecencies that were present when you restart will be present after that, and they'll be preserved until you clear your history.

Comment 27

2 months ago
Not sure if it managed to get in beta 12, but I just updated to beta 12 (received the update a few minutes ago) and I still don't have autofill working.
(Assignee)

Comment 28

a month ago
You probably want bug 1493636, not this one.
(Assignee)

Updated

a month ago
See Also: bug 1493543
You need to log in before you can comment on or make changes to this bug.