Bug 1641261 - Handle the case when an app provided engine is hidden/removed, and a WebExtension wants to select as default. r=daleharvey

Differential Revision: https://phabricator.services.mozilla.com/D77429

browser/components/extensions/parent/ext-chrome-settings-overrides.js

@@ -379,8 +379,11 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
 
     let engineName = searchProvider.name.trim();
     if (searchProvider.is_default) {
-      if (await Services.search.maybeSetAndOverrideDefault(extension)) {
+      let result = await Services.search.maybeSetAndOverrideDefault(extension);
+      if (result.canChangeToAppProvided) {
         await this.setDefault(engineName);
+      }
+      if (!result.canInstallEngine) {
         return;
       }
     }

browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js

@@ -63,6 +63,48 @@ add_task(async function test_extension_setting_default_engine() {
   );
 });
 
+/* This tests what happens when the engine you're setting it to is hidden. */
+add_task(async function test_extension_setting_default_engine_hidden() {
+  let engine = Services.search.getEngineByName("DuckDuckGo");
+  engine.hidden = true;
+
+  let ext1 = ExtensionTestUtils.loadExtension({
+    manifest: {
+      chrome_settings_overrides: {
+        search_provider: {
+          name: "DuckDuckGo",
+          search_url: "https://example.com/?q={searchTerms}",
+          is_default: true,
+        },
+      },
+    },
+    useAddonManager: "temporary",
+  });
+
+  await ext1.startup();
+  await AddonTestUtils.waitForSearchProviderStartup(ext1);
+
+  is(
+    (await Services.search.getDefault()).name,
+    defaultEngineName,
+    "Default engine should have remained as the default"
+  );
+  is(
+    ExtensionSettingsStore.getSetting("default_search", "defaultSearch"),
+    null,
+    "The extension should not have been recorded as having set the default search"
+  );
+
+  await ext1.unload();
+
+  is(
+    (await Services.search.getDefault()).name,
+    defaultEngineName,
+    `Default engine is ${defaultEngineName}`
+  );
+  engine.hidden = false;
+});
+
 // Test the popup displayed when trying to add a non-built-in default
 // search engine.
 add_task(async function test_extension_setting_default_engine_external() {

toolkit/components/search/SearchService.jsm

@@ -717,8 +717,15 @@ SearchService.prototype = {
     let searchProvider =
       extension.manifest.chrome_settings_overrides.search_provider;
     let engine = this._engines.get(searchProvider.name);
-    if (!engine || !engine.isAppProvided) {
-      return false;
+    if (!engine || !engine.isAppProvided || engine.hidden) {
+      // If the engine is not application provided, then we shouldn't simply
+      // set default to it.
+      // If the engine is application provided, but hidden, then we don't
+      // switch to it, nor do we try to install it.
+      return {
+        canChangeToAppProvided: false,
+        canInstallEngine: !engine?.hidden,
+      };
     }
     let params = this.getEngineParams(
       extension,
@@ -736,7 +743,10 @@ SearchService.prototype = {
     ) {
       // Don't allow an extension to set the default if it is already the default.
       if (this.defaultEngine.name == searchProvider.name) {
-        return false;
+        return {
+          canChangeToAppProvided: false,
+          canInstallEngine: false,
+        };
       }
       if (
         !(await this._defaultOverrideAllowlist.canOverride(
@@ -750,7 +760,10 @@ SearchService.prototype = {
         );
         // We don't allow overriding the engine in this case, but we can allow
         // the extension to change the default engine.
-        return true;
+        return {
+          canChangeToAppProvided: true,
+          canInstallEngine: false,
+        };
       }
       // We're ok to override.
       engine.overrideWithExtension(params);
@@ -758,7 +771,10 @@ SearchService.prototype = {
         "Allowing default engine to be set to app-provided and overridden.",
         extension.id
       );
-      return true;
+      return {
+        canChangeToAppProvided: true,
+        canInstallEngine: false,
+      };
     }
 
     if (
@@ -773,10 +789,16 @@ SearchService.prototype = {
         "Re-enabling overriding of core extension by",
         extension.id
       );
-      return true;
+      return {
+        canChangeToAppProvided: true,
+        canInstallEngine: false,
+      };
     }
 
-    return false;
+    return {
+      canChangeToAppProvided: false,
+      canInstallEngine: false,
+    };
   },
 
   /**

toolkit/components/search/nsISearchService.idl

@@ -500,8 +500,11 @@ interface nsISearchService : nsISupports
    *
    *  @param extension
    *        The extension to load from.
-   *  @returns A boolean that indicates if the WebExtension engine may set the
-   *           named engine as default (because it is application provided).
+   *  @returns An object with two booleans:
+   *        - canChangeToAppProvided: indicates if the WebExtension engine may
+   *            set the named engine as default e.g. it is application provided.
+   *        - canInstallEngine: indicates if the WebExtension engine may be
+   *            installed, e.g. it is not an app-provided engine.
    */
   Promise maybeSetAndOverrideDefault(in jsval extension);
 

toolkit/components/search/tests/xpcshell/test_override_allowlist.js

@@ -29,6 +29,7 @@ const tests = [
     },
     expected: {
       switchToDefaultAllowed: false,
+      canInstallEngine: true,
       overridesEngine: false,
     },
   },
@@ -43,6 +44,7 @@ const tests = [
     },
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -57,6 +59,7 @@ const tests = [
     },
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -76,6 +79,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: true,
       searchUrl: kSearchEngineURL,
     },
@@ -96,6 +100,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: true,
       searchUrl: kSearchEngineURL,
     },
@@ -116,6 +121,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -137,6 +143,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: true,
       searchUrl: `${kBaseURL}?q={searchTerms}&enc=UTF-8`,
     },
@@ -159,6 +166,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -180,6 +188,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: true,
       searchUrl: `${kBaseURL}`,
       postData: "q={searchTerms}&enc=UTF-8",
@@ -203,6 +212,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -224,6 +234,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: true,
       searchUrl: `${kBaseURL}`,
       searchForm: "https://example.com/form",
@@ -247,6 +258,7 @@ const tests = [
     ],
     expected: {
       switchToDefaultAllowed: true,
+      canInstallEngine: false,
       overridesEngine: false,
     },
   },
@@ -300,11 +312,17 @@ for (const test of tests) {
       ]);
     }
 
+    let result = await Services.search.maybeSetAndOverrideDefault(extension);
     Assert.equal(
-      await Services.search.maybeSetAndOverrideDefault(extension),
+      result.canChangeToAppProvided,
       test.expected.switchToDefaultAllowed,
       "Should have returned the correct value for allowing switch to default or not."
     );
+    Assert.equal(
+      result.canInstallEngine,
+      test.expected.canInstallEngine,
+      "Should have returned the correct value for allowing to install the engine or not."
+    );
 
     let engine = await Services.search.getEngineByName(kOverriddenEngineName);
     Assert.equal(