1973899 - Search settings use name instead of id to check if app provided search engine is inactive

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Moritz Beier [:mbeier]]

https://searchfox.org/mozilla-central/rev/ec8a326713f60dec138a3e3383b03ac739870fc7/toolkit/components/search/SearchSettings.sys.mjs#316

In this line, search settings persists old entries for app provided engines if no app provided engine with the entry's name is currently available. If we rename an app provided engine, search settings will create a new entry with the new name but also keep the old entry with the old name but same id.

The following test case

const CONFIG = [{ identifier: "default" }];

add_setup(async function () {
  SearchTestUtils.updateRemoteSettingsConfig(CONFIG);
  await Services.search.init();
});

add_task(async function () {
  CONFIG[0].base = { name: "default2" };
  await SearchTestUtils.updateRemoteSettingsConfig(CONFIG);

  let settings = await Services.search.wrappedJSObject._settings.get();
  console.log(settings.engines);
});

produces the following output

[{id:"default", _name:"default2", _isAppProvided:true, _metaData:{}}, {id:"default", _name:"default", _isAppProvided:true, _metaData:{}}]

This doesn't cause the search service to crash immediately, but I think it might still lead to issues.

Comment 1

[CalebChan]

Attachment: Bug 1973899 - Changes Search Settings to compare IDs instead of names to check if app provided search engine is inactive. r=#search-reviewers

Comment 2

[pstanciu]

Backed out for causing xpc failures @ test_override_allowlist_switch.js

• https://hg.mozilla.org/integration/autoland/rev/1752ed6e21e1ce23bbe08edcdc1eaa47f24ad727
• Push with failures
• Failure Log

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/52f532d017db
https://hg.mozilla.org/integration/autoland/rev/216068a0e63f
Changes Search Settings to compare IDs instead of names to check if app provided search engine is inactive. r=search-reviewers,Standard8

Comment 4

[CalebChan]

ID checking ensures if an engine is renamed, we don't save both engines while checking names ensures if a different engine overrides an inactive engine, we don't save both and cause a collision on next startup.

test_override_allowlist_switch.js failed because of the name comparison removal, so both ID and name comparisons were added

Comment 5

[BugBot [:suhaib / :marco]]

There is an r+ patch which didn't land and no activity in this bug for 1 week.
:cachan, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ef77f0c33de8
https://hg.mozilla.org/integration/autoland/rev/276ae6aabee9
Changes Search Settings to compare IDs instead of names to check if app provided search engine is inactive. r=Standard8

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/276ae6aabee9