1638888 - Disabling and re-enabling a WebExtension which provides a default search engine, doesn't re-set the engine as default

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Mark Banner (:standard8)]

STR:

1. Start up Firefox with a fresh profile.
2. Check your default engine in preferences.
3. Install Ecosia add-on
4. When the prompt to set as default engine appears, accept it.
5. Check the default engine

• Should be Ecosia.

6. Disable the Ecosia add-on
7. Check the default engine

• Should have reverted to the original

8. Enable the Ecosia add-on

Actual Results

Ecosia is not shown as enabled.

Expected Results

Ecosia is enabled.

I checked with the builds before and after, and this is a regression from bug 1578513.

Comment 1

[Andrew Swan [:aswan]]

I can't find previous dicussion of this topic, but I could swear this was intentional behavior.
The extension search engine feature doesn't have great test coverage of behaviors like this that are deliberate, but maybe mkaply remembers more about earlier discussions.

Comment 2

[Mike Kaply [:mkaply]]

Actually it wasn't. Disable with immediate reenable should keep the default. We have code that explicitly handled that.

https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/components/extensions/parent/ext-chrome-settings-overrides.js#381

Comment 3

[Mark Banner (:standard8)]

I think what is happening here, is that bug 1578513 added an onEnabling handler, this generally gets called first.

This calls processDefaultSearchSetting with enabled, which attempts to apply the default setting, but fails because the engine doesn't exist yet.

Then, ExtensionCommon's asyncEmitManifestEntry calls through to processSearchProviderManifestEntry which:

• Add the search engine to the search service.
• Calls processDefaultSearchSetting again.
• Now, the default engine (e.g. Google) is different to what the value is (Ecosia), so it removes the setting from the store.

Interestingly if I try debugging and set a breakpoint in processDefaultSearchSetting, that seems to pause the onEnabling handling whilst the other one runs through asynchronously, in that case the Ecosia engine gets set as default.

Comment 4

[Mark Banner (:standard8)]

FWIW there's a little bit of the explanation of the intended in bug 1397975, though it doesn't say where that came from.

Shane, any idea why bug 1578513 needed the separate onEnabling handler?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Banner (:standard8) from comment #4)

FWIW there's a little bit of the explanation of the intended in bug 1397975, though it doesn't say where that came from.

Shane, any idea why bug 1578513 needed the separate onEnabling handler?

Because addons are not started in safe mode, but we still need to disable/enable settings for them in safe mode.

looking through this code I see a lot of things that should be revised.

I don't understand how the code linked in comment 2 is supposed to retain default when disable/enable happens in the same session. My understanding of the purpose of that code is entirely different. If we have Engine_Name_A as a builtin engine, and an extension is installed that provides a search engine with Engine_Name_A that we would set that engine as default. This was done to prevent extensions changing the urls of our builtin engines, but allowing them to set them to default.

What I would probably explore doing is changing setDefault to something like this though the overall onManifestEntry code is confusing:

  async setDefault(engineName) {
    let { extension } = this;
    let defaultEngine = await Services.search.getDefault();
    await ExtensionSettingsStore.initialize();
    if (extension.startupReason === "ADDON_INSTALL") {
      // We should only get here if an extension is setting a builtin engine to default and 
      // we are ignoring the addons other engine settings.  In this case we do not show the prompt to the user.
      let item = await ExtensionSettingsStore.addSetting(
        extension.id,
        DEFAULT_SEARCH_STORE_TYPE,
        DEFAULT_SEARCH_SETTING_NAME,
        engineName,
        () => defaultEngine.name
      );
      await Services.search.setDefault(
        Services.search.getEngineByName(item.value)
      );
    } else if (extension.startupReason === "ADDON_ENABLE") {
      // We would be called for every extension being enabled, we should verify that it has control and only then set it as default
      let control = await ExtensionSettingsStore.getLevelOfControl(extension.id, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME);
      if (control === "controlled_by_this_extension") {
        await Services.search.setDefault(
          Services.search.getEngineByName(engineName) <-- have to get engineName from somewhere
        );
      }
    }
  }

This is assuming that the call from onEnabling is for the most part doing what it should (and isn't removing the setting). The change here is setting the default engine during startup after enabling if the extension has control of the setting.

1. The initial install would set the setting after the user accepts (in processSearchProviderManifestEntry).
2. When disabling, the setting should remain in the store.
3. When enabling, onEnabling would call, and presumably call ExtensionSettingsStore.enable (gaining control), but since the addon is not yet started there is no engine to set as default.
4. addon is started with ADDON_ENABLE, setDefault is called, we check that the extension has control and set the default engine.

The removesetting logic in processDefaultSearchSetting seems suspect as well. Setting entries should only be removed on uninstall. onDisable shouldn't be removing the setting either.

Basically, calls should match addon state:

addon install -> if appropriate, add setting then set default engine
addon enable -> enable setting, if extension has control, set default engine
addon disable -> disable setting
addon uninstall -> remove setting

If something (e.g. user) changes the default engine, then select[1] should be called to specify it is user-set. That would prevent the setting having controlled_by_this_extension until the user selected the addon as default at a later time.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionSettingsStore.jsm#485-486

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: rough draft.patch

This is roughly the changes I think should happen here.

Comment 7

[Mark Banner (:standard8)]

I've chatted about this with product and UX, and we've decided that we should re-prompt every time the add-on is enabled. There's a little debate over if we should re-prompt if they previously denied, but for now I'm going to keep it simple and make it prompt every time.

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1638888 - Make disabling and enabling a WebExtension restore the default search engine correctly. r?mixedpuppy!

Comment 9

[Mark Banner (:standard8)]

Dropping this from bug 1635220 as it isn't strictly necessary for that, and there's a little bit more work to go before this is ready.

Comment 10

[Mark Banner (:standard8)]

Attachment: Bug 1638888 - Simplify processSearchProviderManifestEntry to handle the not setting as default engine case first. r?mixedpuppy!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/694873158783
Simplify processSearchProviderManifestEntry to handle the not setting as default engine case first. r=mixedpuppy,extension-reviewers
https://hg.mozilla.org/integration/autoland/rev/02a093378227
Make disabling and enabling a WebExtension restore the default search engine correctly. r=mixedpuppy,extension-reviewers

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 9155903 [details]
Bug 1638888 - Simplify processSearchProviderManifestEntry to handle the not setting as default engine case first. r?mixedpuppy!

Beta/Release Uplift Approval Request

• User impact if declined: This fixes disable/re-enabling when a external add-on is setting an engine as default. It is also needed to fix some issues relating to the search override work (bug 1635220).
• 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): The scope of the patches has been kept as small as possible, and have a good amount of automated tests.

The patches should only affect the flows around WebExtensions which provide search engines as part of their configuration.

The test cases that QA have also been running through should help to verify support as well.

• String changes made/needed: None

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/694873158783
https://hg.mozilla.org/mozilla-central/rev/02a093378227

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9151092 [details]
Bug 1638888 - Make disabling and enabling a WebExtension restore the default search engine correctly. r?mixedpuppy!

approved for 78.0b8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/187c1335d4df
https://hg.mozilla.org/releases/mozilla-beta/rev/b3d0f6636452

Comment 16

[Alex Cornestean]

Hello,

Verified the fix using the latest Nightly (79.0a1/20200617215206) and Beta (78.0b8/20200616235426) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS. Also reproduced the issue on the latest Release (77.0.1/20200602222727) for comparison purposes.

Disabling the add-on and afterwards re-enabling it will properly set the search engine to the one provided by the extension, thus confirming the fix.

Comment 17

[Alex Cornestean]

Hello,

Verified the fix using ESR 78.0 (78.0esr/20200623021425) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

Disabling the add-on and afterwards re-enabling it will properly set the search engine to the one provided by the extension, thus confirming the fix.