1582234 - browser.urlbar.suggest.bookmark=false doesn't autofill anymore visited bookmarks

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Roman]

1. Set browser.urlbar.suggest.bookmark = false
2. Have en.wikipedia.org in my history as one of my most visited sites
3. Have a bookmark for en.wikipedia.org.
4. Type "en.wikip" in address bar

As I'm typing that, all kinds of sites get autocompleted, but not en.wikipedia.org. This site is the top suggestion, but won't get autocompleted. Eventually Firefox runs out of autocompletions and gives up, offering "http://en.wikip - Visit" - with wikipedia as the first suggestion.

Setting browser.urlbar.suggest.bookmark = true fixes this, and now en.wikipedia.org is autocompleted just from the letter "e" because of how frequently it's been opened.

Expected behaviour: browser.urlbar.suggest.bookmark = false should not actively prevent autocompletions that would otherwise be appropriate based on visit frequency.

Possibly a regression due to fix for bug 1502821?

Comment 1

[Marco Bonardo [:mak]]

This is very similar to bug 1578414, likely the same. So we have multiple reports, that seem to indicate there is some mishandling of user expectation.

Could you please check if in about:config you have a browser.urlbar.autoFill.stddevMultiplier pref?

Could you please run the following script in the devtools scratchpad and copy paste what it prints in the Browser Console? (you may have to enable chrome debugging in the devtools options)

(async function () {
  let db = await PlacesUtils.promiseDBConnection();
  let rows = await db.execute(`
    WITH s(count, sum, squares) AS (
      SELECT
        CAST((SELECT IFNULL(value, 0.0) FROM moz_meta WHERE key = "origin_frecency_count") AS REAL),
        CAST((SELECT IFNULL(value, 0.0) FROM moz_meta WHERE key = "origin_frecency_sum") AS REAL),
        CAST((SELECT IFNULL(value, 0.0) FROM moz_meta WHERE key = "origin_frecency_sum_of_squares") AS REAL)
    )
    SELECT count, sum, squares, CASE count
    WHEN 0 THEN 0.0
    WHEN 1 THEN sum
    ELSE (sum / count) + sqrt((squares - ((sum * sum) / count)) / count)
    END
    FROM s
  `);
  for (let i = 0; i < 4; i++) {
    console.log(rows[0].getResultByIndex(i));
  }
  console.log("*** origins:");
  rows = await db.execute(`
    SELECT frecency, prefix, host
    FROM moz_origins
    WHERE like("%wikipedia.org%", host)
  `);
  for (let row of rows) {
    console.log(row.getResultByIndex(0),
                row.getResultByIndex(1),
                row.getResultByIndex(2));
  }
})();

I suspect with just history the urls don't clear the frecency threshold, that makes me think the threshold is still too high, we may want to re-evaluate.

Comment 2

[Roman]

browser.urlbar.autoFill.stddevMultiplier isn't listed in my about:config

7153
25834169
26652921113603
64546.65175288319
*** origins:
9074 http:// en.wikipedia.org
67 http:// ru.wikipedia.org
808702 https:// en.wikipedia.org
6990 https:// en.m.wikipedia.org
42272 https:// ru.wikipedia.org
100 https:// nl.wikipedia.org
75 https:// www.wikipedia.org
1490 https:// uk.wikipedia.org
100 https:// ru.m.wikipedia.org
50 https:// wikipedia.org
2338 https:// it.wikipedia.org
100 https:// mn.wikipedia.org
200 https:// de.wikipedia.org

Comment 3

[Marco Bonardo [:mak]]

so the threshold is 25834169 / 7153 = 3611
en.wikipedia.org is well over that and thus it should fill.

We should check again the bookmarks exclusion query.

Comment 4

[Marco Bonardo [:mak]]

Just to be sure, do you have History enabled, or do you have it set to Never Remember History?

Comment 5

[Roman]

History is enabled.

I do believe this worked very reliably until recently. I suspected v69 regressed this, so I went looking for recent changes and found the one related to bookmark exclusions (bug 1502821). This is what gave me the idea to try turning off bookmark exclusion (which had been enabled for multiple versions prior) and lo and behold, autocomplete started working.

The "bookmark exclusion query" does sound suspicious - it's almost like Firefox is seeing the history hits, but then goes "oh but that URL is bookmarked and bookmarks are excluded". This may well be how it was indended by the developer, but that's not how it used to work, nor is it what I personally wish to see from this setting: my expectation is that this setting stops bookmarks from being considered altogether, rather than using them as a sort of an autocomplete blacklist.

(edit: but to be fair, the last paragraph above is pure speculation on my part, I can't easily test it as I'd have to delete too many bookmarks)

Comment 6

[Marco Bonardo [:mak]]

Oh yeah, based on the many reports, I suspect we went too far away. if suggest bookmarks is disabled, we should still autofill bookmarks IFF they have visits.

Comment 8

[Jukka Alasalmi]

I have the same problem, which I reported under bug 1581550. However, even after enabling the autofill from bookmarks, I have one site that doesn't work, namely mbnet.fi. The other sites I tried are working correctly after the setting change, however. I'm not sure if this is related to the frecency threshold or not, so here is my information related to that:

I also don't have the browser.urlbar.autoFill.stddevMultiplier listed in my about:config. When running a modified version of the script (wikipedia.org replaced by mbnet.fi), I get this output:

20751766 debugger eval code:18:13
111832866302206 debugger eval code:18:13
207431.3516452619 debugger eval code:18:13
*** origins: debugger eval code:20:11
2000 http:// www.mbnet.fi debugger eval code:27:13
4193 https:// www.mbnet.fi debugger eval code:27:13

If I'm doing the correct calculation, the result is 20751766 / 2792 = 7432 so presumably this should be above the threshold as well.

Earlier Firefox autofilling domains from even unvisited bookmarks, but it wouldn't fill any other part. Now it's no longer doing that, but indeed it is filtering even visited domains out. I think that this was working correctly something like 1 year or more ago.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Jukka Alasalmi from comment #8)

2000 http:// www.mbnet.fi debugger eval code:27:13
4193 https:// www.mbnet.fi debugger eval code:27:13
If I'm doing the correct calculation, the result is 20751766 / 2792 = 7432 so presumably this should be above the threshold as well.

4193 is not above 7432... it doesn't seem to be visited "often enough".

Comment 11

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #6)

Oh yeah, based on the many reports, I suspect we went too far away. if suggest bookmarks is disabled, we should still autofill bookmarks IFF they have visits.

But isn't that what people in the other bug didn't want to happen? That's why we implemented it like that iirc. i.e., suggest.bookmarks = false means that bookmarks shouldn't be autofilled.

Comment 12

[Roman]

(In reply to Drew Willcoxon :adw from comment #11)

But isn't that what people in the other bug didn't want to happen? That's why we implemented it like that iirc. i.e., suggest.bookmarks = false means that bookmarks shouldn't be autofilled.

Well. Yes, there are two ways to interpret the phrase "don't suggest bookmarks". One is "come up with all suggestions, then exclude everything that's bookmarked". The other is "ignore bookmarks altogether". As a user, I want my suggestions and autofill to be based only on visit history. I want the existence of a bookmark to have no impact whatsoever on autofill and suggestions. This is the "ignore bookmarks altogether" interpretation.

Comment 13

[Jukka Alasalmi]

(In reply to Drew Willcoxon :adw from comment #11)

But isn't that what people in the other bug didn't want to happen? That's why we implemented it like that iirc. i.e., suggest.bookmarks = false means that bookmarks shouldn't be autofilled.

If I have autofill / search enabled from history, but not bookmarks, then I expect that only the history affects the results.

Thinking about the opposite case, if I only had search from bookmarks enabled, I would not expect having visited some site to blacklist something from the bookmark autofill.

But when I have "suggest.bookmarks = false", I would not want any results that are in the bookmarks only.

(In reply to Marco Bonardo [::mak] from comment #10)

(In reply to Jukka Alasalmi from comment #8)

2000 http:// www.mbnet.fi debugger eval code:27:13
4193 https:// www.mbnet.fi debugger eval code:27:13
If I'm doing the correct calculation, the result is 20751766 / 2792 = 7432 so presumably this should be above the threshold as well.

4193 is not above 7432... it doesn't seem to be visited "often enough".

I'm not sure where I got the divider, but also clearly I misunderstood how the threshold works as well. Anyways, I can confirm that after visiting the site a couple of times, it does indeed have autofill now.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #11)

But isn't that what people in the other bug didn't want to happen? That's why we implemented it like that iirc. i.e., suggest.bookmarks = false means that bookmarks shouldn't be autofilled.

Well, these pages are bookmarks, but they are also in history, so by filling visited bookmarks we are not breaking the suggest.bookmarks = false promise. I agree that, as comment 12 says, things can be interpreted both ways, we took one interpretation, it doesn't seem to be appreciated.
The reason is likely that it looks like a regression, because this is how things used to work, and in general we always tried to provide more results, then less. Before our change, people started complaining because we made bookmarks autofill by default without any threshold, that pretty much means we autofilled any bookmark (bug 1493636), so we were clearly violating suggest.bookmarks = false.

So, my suggestion is: when suggest.bookmarks is false, we could just fallback to autofill based on the threshold, and not autofill unvisited bookmarks. This would pretty much make us autofill based on history.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix for 70 but we could still take a patch in 71/72.
Can you help find an owner for this P1 bug?

Comment 16

[Marco Bonardo [:mak]]

We are struggling to find resources in the Search team at this time. I hope to find some time next week. I know where the bug is (we can fix the query in unifiedComplete.jsm by just checking for visitedness status when bookmarks are excluded) but fixing the many tests may require some additional time.

Comment 17

[Drew Willcoxon :adw]

Attachment: Bug 1582234 - Change the meaning of browser.urlbar.suggest.bookmark = false so that visited bookmarks are autofilled but unvisited bookmarks still are not.

When suggest.history = true and suggest.bookmark = false, change the inclusion logic from NOT bookmarked to visited OR NOT bookmarked. That will include visited bookmarks above the autofill threshold but still exclude unvisited bookmarks.

This also renames the various SQL query consts to better reflect when they are used: HISTORY_BOOKMARK for when both suggest.history and suggest.bookmark = true, HISTORY for when only suggest.history = true, and BOOKMARK for when only suggest.bookmark = true.

Finally, it adds a bunch of test tasks consistent with the other existing autofill test tasks.

Comment 18

[Drew Willcoxon :adw]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c32d4e19a92
Change the meaning of browser.urlbar.suggest.bookmark = false so that visited bookmarks are autofilled but unvisited bookmarks still are not. r=mak

Comment 20

[Razvan Maries]

Backed out for lint failure on UnifiedComplete.jsm.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=272596993&resultStatus=testfailed%2Cbusted%2Cexception&revision=3c32d4e19a929a92e875e63e92d189e06cefc3ed&searchStr=linting%2Copt%2Cjavascript%2Cchecks%2Csource-test-mozlint-eslint%2Cjs%28es%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272596993&repo=autoland&lineNumber=54

Backout: https://hg.mozilla.org/integration/autoland/rev/62d6bd13bb5638c06e6cc609b3df43587bfda675

Comment 21

[Drew Willcoxon :adw]

Fixed and re-queued for landing.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36e230321fd0
Change the meaning of browser.urlbar.suggest.bookmark = false so that visited bookmarks are autofilled but unvisited bookmarks still are not. r=mak

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/36e230321fd0

Comment 24

[Marco Bonardo [:mak]]

I think it's worth uplifting this

Comment 25

[Drew Willcoxon :adw]

STR for QA

1. New profile
2. Open the bookmarks library and add a new bookmark with URL http://example.com/, title doesn't matter
3. Type "example" in the urlbar, example.com should be autofilled
4. Open preferences, Privacy & Security, uncheck Bookmarks in the Address Bar section
5. Type "example" in the urlbar, example.com should not be autofilled
6. Finish typing "example.com" and hit enter so that you visit example.com
7. Type "example" in the urlbar, example.com should be autofilled again

Comment 26

[Drew Willcoxon :adw]

Comment on attachment 9103407 [details]
Bug 1582234 - Change the meaning of browser.urlbar.suggest.bookmark = false so that visited bookmarks are autofilled but unvisited bookmarks still are not.

Beta/Release Uplift Approval Request

• User impact if declined: Users expect visited bookmarks to be autofilled even when they disable bookmark suggestions in the urlbar. Without this patch, visited bookmarks will not be autofilled.
• 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 25
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patches modifies urlbar SQL queries, which can be hard to get right, so I would rate this medium if not for the fact that we have comprehensive test coverage for this.
• String changes made/needed:

Comment 27

[Pascal Chevrel:pascalc]

Given that this is a fix for an essential feature in Firefox, I would like it to be verified by QA in nightly before uplifting it to beta.

Comment 28

[Maria Berlinger [:mberlinger], Services QA]

I’ve managed to reproduce this issue on an affected nightly (2019-09-18) using Windows 10x64 by following the STR from comment 25.
This is verified fixed on Firefox 72.0a1 (2019-10-27) on macOS 10.14, windows 10x64, windows 7x64 and Ubuntu 18.04x64.

Comment 29

[Pascal Chevrel:pascalc]

Comment on attachment 9103407 [details]
Bug 1582234 - Change the meaning of browser.urlbar.suggest.bookmark = false so that visited bookmarks are autofilled but unvisited bookmarks still are not.

P1 regression, has tests and was verified by QA on Nightlu, uplift approved for 71 beta 5, thanks.

Comment 30

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e5f81abd3ffb

Comment 31

[Maria Berlinger [:mberlinger], Services QA]

This is also verified fixed using Firefox 71.0b5 on macOS 10.14, windows 10x64, windows 7x64 and Ubuntu 18.04x64.