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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[u623755]

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.

Comment 1

[Drew Willcoxon :adw]

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)`

Comment 2

[Drew Willcoxon :adw]

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.

Comment 3

[Drew Willcoxon :adw]

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).

Comment 4

[Mike Taylor [:miketaylr]]

[Tracking Requested - why for this release]:

Unsure if we need to track this for a dot release, but putting it on the radar.

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1489060 - Tweak how autofill works with bookmarks.

Comment 6

[Drew Willcoxon :adw]

Marco, could you give f? on the approach in the WIP revision please?  (There's no Phabrictor f?, right?)

Comment 7

[Marco Bonardo [:mak]]

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?)

Comment 8

[Drew Willcoxon :adw]

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

[Marco Bonardo [:mak]]

(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

[u624563]

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.

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Drew Willcoxon :adw]

Attachment: 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 13

[Marco Bonardo [:mak]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35b71d287e96
Urlbar autofill is broken when autofilling history is disabled and autofilling bookmarks is enabled r=mak

Comment 15

[Drew Willcoxon :adw]

Please see comment 0 for STR

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/35b71d287e96

Comment 17

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

Marco, do you want to nominate it for Beta backport?

Comment 18

[Drew Willcoxon :adw]

Yes

Comment 19

[Drew Willcoxon :adw]

Attachment: 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

Comment 20

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

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.

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/79aae3b724a5

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

I'm going to call this wontfix for 62 as 63 is less than 3 weeks away.

Comment 23

[Kanchan Kumari QA]

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!

Comment 24

[Drew Willcoxon :adw]

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.

Comment 25

[Kanchan Kumari QA]

(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

Comment 26

[Drew Willcoxon :adw]

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

[u624563]

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.

Comment 28

[Drew Willcoxon :adw]

You probably want bug 1493636, not this one.

Comment 29

[u623755]

Pardon me as I can't understand all the technical comments among the developers here, but as far as what I have described in the very beginning (step to reproduce / actual results / expected results), it hasn't changed one bit even in Firefox 64 - the problem still isn't fixed and is still exactly like how I described in the beginning. May I get a layman update as to what's going on?