1631241 - Engines are in wrong order after restart

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Dale Harvey (:daleharvey)]

STR: Start new profile of Firefox in the UK, engine list will look like

Google
Amazon.com
DuckDuckGo
Bing
Twitter
Wikipedia
eBay
Amazon.co.uk

Then restart, engine order looks like:

eBay
Amazon.co.uk
Bing
DuckDuckGo
Google
Twitter
Wikipedia

Comment 1

[Dale Harvey (:daleharvey)]

And to explain why, We load with a fresh profile, that gives us the default engines

SearchEngineSelector fetchEngineConfiguration: [e-mail],[e-mail],[e-mail],[e-mail],[e-mail],[e-mail],[e-mail]

Then we do a region fetch that gives us the british amazon + ebay (other engines have the same name), we call maybeReloadEngines that calls addEngineToStore for ebay + amazon, addenginetostrore then adds these engines to the __sortedEngines array manually then triggers saving the sorted order[1], now the engine objects in __sortedEngines that refer to engines that were replaced (ones with the same name) are stale, so when the order is saved it is only for the new engines (amazon and ebay)

On next load these engines will come first and the rest will be sorted alphabetically

[1] - https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#1764

Comment 2

[Dale Harvey (:daleharvey)]

This code hasnt changed recently and looks like it will affect at the least all non US users (US engines look to have the same names as defaults), plus anyone who has installed an engine will be affected but less severely (it will store the order but not miss out any engines)

I feel like we probably want to fix this fairly quickly, there are various issues this highlights with varying complexity of fixes, we are already looking to change the way maybereloadengines works and removing __sortedEngines both of which could help this but are quite complex.

Off the top of my head, I think we should change the way our ordering code works to only store the index of on the specific engine the user has moved, have the default sorting take that into account, the ignore the previous "order" fields and the useDBForOrder pref, this means we will lose some user configurations, however the vast majority of these fields are going to be set incorrectly

Comment 3

[Mark Banner (:standard8)]

(In reply to Dale Harvey (:daleharvey) from comment #2)

Off the top of my head, I think we should change the way our ordering code works to only store the index of on the specific engine the user has moved, have the default sorting take that into account, the ignore the previous "order" fields and the useDBForOrder pref, this means we will lose some user configurations, however the vast majority of these fields are going to be set incorrectly

I've been thinking that if an engine is added, then we should still take the default sorting into account, and not set useDBForOrder in that case.

I'm not sure it would be a good idea to ignore everyone's orders because some people have purposely set them. If I remember your case correctly, we didn't actually have orders set on all the engines. Maybe in that case, we could revert to default orders?

I'm pretty sure this didn't used to be too bad. It might be a modern-only thing, or it might be a fairly recent introduction.

Comment 4

[Dale Harvey (:daleharvey)]

In some cases (when we hit via maybeReloadEngines) the order wont be set on all engines, in that case a user may have previously reordered the engines but we cant really know for sure.

Even in the case that this works as expected, I am not really sure it behaves the way we would want it to, if the user has ever installed a search engine we will have tried to fix the order at that point, so any engine we install after that will be tacked on at the end (even if we were to install a new defaultEngine)

Comment 5

[Dale Harvey (:daleharvey)]

How about we remove the saveSortedEngineList from addEngineToStore and instead we can reset this.__sortedEngines, fix the bug for now and then think about people who have had it saved incorrectly and whether we want to change the behaviour in seperate bug(s)?

Comment 6

[Mark Banner (:standard8)]

(In reply to Dale Harvey (:daleharvey) from comment #5)

How about we remove the saveSortedEngineList from addEngineToStore and instead we can reset this.__sortedEngines.

Of course, for some reason I'd been thinking about this from the maybeReloadEngines angle, rather than addEngineToStore.

For changing _addEngineToStore, I haven't got a response either way from UX/product on if we should change or not for user added engines. Therefore, I think we should only change it for ones where we're adding built-in engines.

fix the bug for now and then think about people who have had it saved incorrectly and whether we want to change the behaviour in seperate bug(s)?

I don't think we need to worry about people who have it incorrect - the worst is only on nightly + beta at the moment afaict, and we should be able to uplift it to beta. People might have triggered it in the past by adding engines, but they're likely to have got used to whatever order they have now, so I'd be tempted to say we shouldn't try and change it. I also suspect here that the order would have somehow rebuilt itself properly at some stage (probably after a subsequent rebuild cache), so we're unlikely to detect it.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Bug 1631241 - Dont fix engine orders during maybeReloadEngines

Comment 8

[Mark Banner (:standard8)]

[Tracking Requested - why for this release]: New users may get incorrect and strange orders of search engines

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f665eea97fa0
Dont fix engine orders during maybeReloadEngines r=Standard8

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 9142816 [details]
Bug 1631241 - Dont fix engine orders during maybeReloadEngines

Beta/Release Uplift Approval Request

• User impact if declined: The sort orders of engines may be semi random for new users (especially where the defaults are slightly different depending on the region/locale that are in use).
• 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 fix to ensure the order is correct when regenerating the engine list after the region details are obtained.
• String changes made/needed: None

Comment 11

[Razvan Maries]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9142816 [details]
Bug 1631241 - Dont fix engine orders during maybeReloadEngines

Simple patch which fixes possibly-broken search engine sorting. Thanks for including new tests. Approved for 76.0rc1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is no longer reproducible.
I used nordVPN with the latest nightly on Windows 10 x64.

Comment 15

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is no longer reproducible on 76.0RC.
Updating the flags accordingly.