1712297 - Pinned topsites search shortcuts left hanging when @search_engine gets removed

Status: VERIFIED FIXED

(Firefox :: Top Sites, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

Attachment: user.js

[Description:]

In the case when a @search_engine_shortcut is pinned and by some reason, the search_engine gets removed, the search_engine_shortcut is left hanging, pointing to nothing.

[Steps:]

1. Download an hr locale - let's say 87.0b3.
2. Use an user.js that sets the region to IN: user_pref("browser.search.region", "IN");
3. Start the hr/IN Firefox, observe the amazon.co.uk search engine + pinned topsite amazon.co.uk
4. Close browser down, modify the user.js to point towards staging (on staging the amazon gets removed from any IN regions that are not in a shortlist)
5. Reopen the browser using the user.js pointing towards staging (attached)

[Actual Result:]

The amazon search shortcut tile is still pinned and available, pointing to an search shortcut that has no engine.

[Expected Result:]

Not sure what is the best approach to handle this case elegantly.

[Note:]

This is not a regression, but changes like bug 1705374 will make this issue much more visible.

Comment 1

[Adrian Florinescu [:aflorinescu]]

Attachment: pinned_tile_hanging.gif

Comment 2

[Adrian Florinescu [:aflorinescu]]

Let's raise this to an S3 and block the amazon search-config changes until we inquire deeper into how many people are going to be affected.

Comment 3

[Adrian Florinescu [:aflorinescu]]

Also, for the sake of clarity, the 87.0b3 in step 3 has the dump = true, which means it shall load the topsites data present at the time of the 87.0b3 build, which has amazon as pinned search-shortcut.
Redoing the above step with dump = false will not have amazon as search shortcut or pinned for IN as per current Remote Settings top-sites configuration. (https://settings.prod.mozaws.net/v1/buckets/main/collections/top-sites/records)

Later edit:
The same test with release 87.0 20210318103112 for IN has amazon.co.uk unpinned, so a new profile with 87.0 won't encounter this issue.

Comment 4

[Dão Gottwald [:dao]]

Does the issue persist after another restart?

Comment 5

[Adrian Florinescu [:aflorinescu]]

(In reply to Dão Gottwald [::dao] from comment #4)

Does the issue persist after another restart?

Yes, I was doing a force sync for RS and then a default restart anyways for search-config to get applied, but I redid the test to be sure. In the case in which you started with amazon search shortcut tile pinned, I'm pretty sure any version top-site migration won't touch it and when search engine gets removed, we won't touch it again since it's pinned.

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 1712297 - Hide pinned search shortcut when its engine gets removed/hidden.

Comment 7

[Mark Banner (:standard8)]

[Tracking Requested - why for this release]: We're planning to release a set of Amazon changes to rework our Amazon deployments. As part of that, various region/locale combinations will loose an Amazon search engine, and will therefore hit this bug.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ae61361cde0
Hide pinned search shortcut when its engine gets removed/hidden. r=Standard8

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1712297 - Add test for checking pinned search shortcuts are hidden when the associate engine is removed. r?amy!

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 9222986 [details]
Bug 1712297 - Hide pinned search shortcut when its engine gets removed/hidden.

Beta/Release Uplift Approval Request

• User impact if declined: Users may be left with a unusable Amazon search shortcut pinned on the new tab page. This will happen after a reconfiguration that we are planning on doing soon.

Automated test in follow-up patch.

• 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: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change to how search shortcuts are handled on top sites. Only affects when an engine is removed.
• String changes made/needed: None

Comment 11

[Mark Banner (:standard8)]

(requested both beta & release, as I'm not quite sure the best one given the state of the trees).

Comment 12

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

Comment on attachment 9222986 [details]
Bug 1712297 - Hide pinned search shortcut when its engine gets removed/hidden.

Removing beta approval as we have merged mozilla-beta to mozilla-release today, uplift requests now target our 89 Release Candidate.

Comment 13

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

Mark, do we need this patch in ESR as well? Thanks

Comment 14

[Mark Banner (:standard8)]

Attachment: 1712297esr.txt

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Updating the Amazon configuration may leave a highly visible piece of UI functioning incorrectly - clicking the Amazon shortcut and entering a query would end up doing a search on the default engine for @amazon <my query>
User impact if declined: See above
Fix Landed on Version: Current nightly & 89 uplift requested
Risk to taking this patch (and alternatives if risky): Low

This is the unbitrotted code patch. The test patch will apply without changes.

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9ae61361cde0

Comment 16

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

Comment on attachment 9222986 [details]
Bug 1712297 - Hide pinned search shortcut when its engine gets removed/hidden.

Approved for RC, thanks.

Comment 17

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

Comment on attachment 9223225 [details] [diff] [review]
1712297esr.txt

Approved for ESR, thanks.

Comment 18

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

https://hg.mozilla.org/releases/mozilla-esr78/rev/58d0b007bced

Comment 19

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

https://hg.mozilla.org/releases/mozilla-release/rev/0271650baa15

Comment 20

[Adrian Florinescu [:aflorinescu]]

Indepth verification has been done here covering various update paths and scenarios, with more details about the scenarios/versions/regions/locales combinations used here.

Most regression testing scenarios involved ensuring that the removal and restore/addition of a search-engine, either manually or by search-config update will cause no unforseen regressions related to this fix. The focus was around 78.11.0esr, 89.0 RC and 90.0a1 with different update paths and search-config starting points while covering Ubuntu 20.04, Windows 10 and Mac OSX 11 and 10.13.6.
Additionally we also ensured that the sponsored tiles are not affected either by this fix.

Comment 21

[Mihai Boldan, Desktop QA [:mboldan]]

Removing qe-verify+ flag, based on Comment 20.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03c869dfeb38
Add test for checking pinned search shortcuts are hidden when the associate engine is removed. r=amy,dao

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/03c869dfeb38