1654377 - maybeReloadEngines has a race condition when removing engines.

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Mark Banner (:standard8)]

When I rewrote maybeReloadEngines in bug 1610293, one thing I changed was to additionally remove app-provided engines.

This went via the add-on manager to uninstall the WebExtension, and then remove the engine.

In bug 1653976, Thunderbird has shown there's an intermittent failure on test_reload_engines.js.

After debugging, the cause is seen to be that the engine-removed notification does not happen until after engines-reloaded is completed. This is an issue because as a result we might be incorrectly setting useDBForOrder when we don't want to.

We were slightly uncomfortable with this set-up when the patch landed but it seemed to work fine. I now think we should remove the engine from the search service, then remove it from the add-on manager and not assert if it is app-provided and missing from the list.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1654377 - In maybeReloadEngines remove search engines from the list before uninstalling the WebExtension. r?daleharvey!

This avoids calling into removeEngine, and ensure we don't get potential issues with race conditions that might cause useDBForOrder to be set when we don't want it to be.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1610293

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9162a11a7128
In maybeReloadEngines remove search engines from the list before uninstalling the WebExtension. r=daleharvey

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9162a11a7128