Closed Bug 1773025 Opened 2 months ago Closed 2 months ago

Adaptive history logic breaks when the user 'forgets' the website and also has a bookmark of it

Categories

(Firefox :: Address Bar, defect, P1)

Desktop
All
defect
Points:
3

Tracking

()

VERIFIED FIXED
103 Branch
Tracking Status
firefox102 --- disabled
firefox103 --- verified

People

(Reporter: cbaica, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Affected versions

  • Fx 102.0b4
  • Fx 103.0a1

Affected platforms

  • Windows 10
  • Ubuntu 20.04
  • macOS

Steps to reproduce

  1. Launch Firefox.
  2. Navigate to Reddit and click on a post from the page.
  3. Open a new tab and close the previously accessed page from step 2.
  4. Type 'redd' in the address bar and choose the reddit post from the Firefox Suggest section. (at this point the adaptive history for 'redd' is created).
  5. Bookmark the page.
  6. Open a new tab and close the previously accessed page from step 4.
  7. Open history (either library or sidebar) and use "Forget about this site" to remove the reddit post entry from history.
  8. Delete the bookmark from the bookmark toolbar.

Expected result

  • No adaptive history for autofil remains (the autofill for 'redd' no longer exists).
  • No records exist when checked for in the browser console.

Actual result

  • Adaptive history autofill triggers briefly (flickers) for every letter of the word 'redd' that is typed in the address bar.

Regression range

  • This is not a regression.

Additional notes

  • This seems to be because of the weird interaction between 'Forget about this site' functionality and bookmarking.

What I mean by this is that:

  • if bookmark is removed first, then the forget about this site is used, there is no adaptive history.
  • if the user just DELETES the history entry and then removes the bookmark, no adaptive history entry remains.
  • if an adaptive autofill entry is done on a record that exists only in history and the 'forget about this site' functionality is used on the history entry, the adaptive autofill entry is removed.
  • if an adaptive autofill entry is done on a record that exists only in bookmarks and then the user deletes the bookmark, the adaptive autofill entry is removed.
Has STR: --- → yes

Thanks Cristian, this looks like an autofill placeholder problem. It's a relatively minor severity because it doesn't affect functionality (correct me if I'm wrong), but it's ugly flickering we should fix soon.

Are there STR that don't require adaptive history autofill, i.e., with it disabled? I'm wondering if it's actually unrelated. Don't worry about spending too much time trying to find out.

Priority: -- → P1

I was wrong, this isn't just about the placeholder. There are two problems:

  1. The placeholder is incorrectly autofilled when the provider does not return an autofill result (before you type the full input in the record), causing flickering
  2. Using the STR above, the provider still returns an autofill result (after you type the full input in the record)

The second problem is pretty bad. It happens because the moz_inputhistory record still exists; however, in moz_places, the frecency and visit count are both zero. For adaptive autofill, all we check is moz_inputhistory. We need to also make sure the frecency and/or visit count are > 0, probably.

Note that this is not a problem for the input history provider. If I disable adaptive autofill and type the record's input, no input history result is shown. AFAICT that's because the input history provider uses autocomplete_match() to filter rows, and autocomplete_match() normally allows a row only when its visit count is > 0.

That said, the adaptive history record is eventually deleted, I'm not sure by what or how long it takes (a few minutes in my testing) but IIRC Places performs periodic cleanups.

Blocks: 1597791
Assignee: nobody → adw
Status: NEW → ASSIGNED
Points: --- → 3

Problem 1 (related to the placeholder) was kind of addressed by bug 1584545, but AFAICT in order to avoid flicker we also need to set _autofillPlaceholder to an empty string when the input receives the first result and it is not an autofill. The code added by that bug is now here.

Depends on: 1584545
See Also: → 1773918

I filed bug 1773918 for problem 1 since I ended up fixing some other problems related to the placeholder. Let's focus this bug on problem 2.

No longer depends on: 1584545

This makes the adaptive history query use the same conditions for each of the
sources that the URL query uses. This fixes the bug, and it also makes sure that
adaptive history autofill and URL autofill generally occur under the same
conditions, which is desirable IMO.

I also moved urlCondition to the end of the WHERE clause as an optimization,
so the other parts of the WHERE clause can short-circuit its string comparison.

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c8aa5bb8751
Make the adaptive history autofill conditions the same as the URL autofill conditions so non-visited zero-frecency URLs are not autofilled. r=daisuke
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

The patch landed in nightly and beta is affected.
:adw, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox102 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(adw)
Flags: needinfo?(adw)
Flags: qe-verify+
Flags: in-testsuite+

This is verified fixed in Fx103.0a1 on Windows 10. The adaptive history record is now correctly removed from firefox.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.