1694237 - Remote search suggestions that dupe form history are no longer discarded

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

STR

1. Do a search for testing in Google
2. Start to type testing in the urlbar again

You'll see both a form history result and a remote suggestion result for testing.

I broke this with the muxer changes in bug 1676469. Previously deduping was done by the suggestions provider. I removed that logic and meant to add it to the muxer -- where it could be smarter -- but didn't.

Comment 1

[Drew Willcoxon :adw]

[Tracking Requested - why for this release]: This is a regression introduced in 87 that we should fix.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1694237 - Properly discard duplicate suggestions in urlbar results.

This was a little more work than I first thought because fixing it uncovered
another problem: The recurse logic in the muxer isn't quite right. There are a
couple of problems actually:

1. By re-using the results from children that did not fill up, we skip updating
   stateCopy (and therefore state), which messes up subsequent buckets in
   the recursion because they're working with the wrong state.
2. By simply assigning state = stateCopy after handling children that didn't
   fill up, we're not really doing anything because at that point the function
   is done. The caller and subsequent buckets in the recursion won't see the
   updated state. We need to update state in place.

These problems were revealed in test_resultBuckets.js, which is pretty thorough.

To fix the actual problem that the bug is about (not deduping remote suggestions
and form history), we just need to keep a set of suggestions that the muxer has
seen so far, and then _canAddResult can discard dupe suggestions. This patch
adds state.suggestions for that, and it includes form history, remote
suggestions, and the heuristic query when it's a search result. This way the
relative ordering of form history vs. remote suggestions doesn't matter. We'll
dedupe whichever comes later.

A bunch of tasks in test_resultBuckets.js needed to be updated to account for
this because they were incorrectly not expecting dupes to be removed.

Comment 3

[Drew Willcoxon :adw]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cff284da4c22
Properly discard duplicate suggestions in urlbar results. r=mak

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/cff284da4c22

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 9204686 [details]
Bug 1694237 - Properly discard duplicate suggestions in urlbar results.

Beta/Release Uplift Approval Request

• User impact if declined: The same search suggestion can appear as both a normal suggestion and as a suggestion from the user's search history ("form history"), if the suggestion is in the user's search history. This is a regression in 87.
• 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: 1. Type "testing" in the urlbar and press enter to do a search

2. Start typing "testing" again. As you are typing it, verify that "testing" appears as a suggestion from your search history (it has a clock icon), but it does not appear as a normal suggestion from Google (there is not another "testing" suggestion with a search glass icon).

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a fairly small, well understood patch, and we have lots of test coverage.
• String changes made/needed:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9204686 [details]
Bug 1694237 - Properly discard duplicate suggestions in urlbar results.

Approved for 87.0b3.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8e9638abf2b3

Comment 9

[Adrian Florinescu [:aflorinescu]]

Reproduced the duped suggestions behavior on: 87.0a1 2021-02-20

Verified fixed on : 88.0a1 2021-02-25 using: Ubuntu 18.04 , Mac 10.14.6 and Windows 10.

Drew, any sensitive areas that this regression/fix might've affected and might need an extended spot checking?

Comment 10

[Drew Willcoxon :adw]

Thanks for asking. This particular regression is only about search history and search engine suggestions not being properly de-duplicated, and I think the STR in my uplift comment are enough to test it.

The bug that caused the regression, bug 1676469, did introduce much larger changes. (It landed in 87 before the merge.) If you have any existing test cases for the composition of results in the urlbar, it might be good to run them if you have time. I'm thinking of the "Show search suggestions ahead of browsing history in address bar results" checkbox in about:preferences#search, or tests for search history, for example.

We do have pretty extensive automated tests for this, so I wouldn't go out of your way to do a lot of testing, but it's always good to have QA coverage too. :-) If you do end up testing and have any questions, please let me know. Thanks Adrian!

Comment 11

[Adrian Florinescu [:aflorinescu]]

Let's get this uplift out of the way. Verified on 87.0b5 2021-03-02, Ubuntu 18.04, Mac 10.14.6 and Win 10. We'll try to cover some extended scenarios as part of beta 87 pref'd off testing on Photon Address bar (our urlbar composition tests need some updates there anyways)

Comment 12

[Adrian Florinescu [:aflorinescu]]

A bit unrelated to this regression, but I'll note it here nevertheless: when transitioning through typing a history contained query string, we're going through two phases, which in my view might hold less value than actually showing one more search suggestion. To elaborate, use the following:

STR:

1. Type "testing" in the urlbar or search-mode and press enter to do a search
2. Close the tab, open a new tab.
3. In the urlbar, type test
4. Continue typing +ing
5. Continue typing adding a space.

AR:
3. testing shown as history suggestion
4. testing shown as search term - no history icon
5. both search suggestion and search term

In my view, both step 4 and step 5 results should show only one entry for testing, and in the case in which there is search history mark it as such. It would be arguable that there is a need to mark that this is a google search or whatever the default engine might be, but when you consider the search-mode now, it just seems a redundant detail + ocupying needesly a search suggestion spot.

Comment 13

[Drew Willcoxon :adw]

By design, we don't show suggestions that match what you've typed. So that's why when you type "testing" in step 4, the "testing" search history goes away. I'm not 100% sure, but I think the idea is that there's no point in suggesting what you've already typed, even if it's a suggestion from your search history. (We don't show a matching suggestion from the search engine either.)

The AR from step 5 is just a bug though. Typing a trailing space should not cause the "testing" search history to go from not shown to shown. This isn't a regression from bug 1676469 (I checked). I didn't check how far back it goes, but probably a long time. I'll file a bug for it, thanks.

Comment 14

[Drew Willcoxon :adw]

Also, I think you might also be referring to the "heuristic" result -- that's the result that's always shown first when you're typing something and is always pre-selected. It's not a suggestion result, at least not as we normally use the term "suggestion". It just shows what's going to happen when you press enter.