Search engine is reset even if WebExtension version is installed

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dominik.henter, Assigned: mkaply)

Tracking

57 Branch
Firefox 59
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox58blocking verified, firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Steps to reproduce:

* Install a (legacy) addon in < 57 that adds a search engine
* Upgrade to 57 and get the webextension version of the same addon
* Upgrade to 57.0.1 


Actual results:

The search engine is being reset to Google without any prompts.


Expected results:

The search engine should be the one provided by the webextension version of the addon
(Reporter)

Comment 1

a year ago
This is very likely due to https://bugzilla.mozilla.org/show_bug.cgi?id=1419941
The problem is probably that the legacy version (https://addons.mozilla.org/de/firefox/addon/ecosia-the-green-search/versions/?page=1#version-3.4.5) uses the same name (ecosia) for the search engine as the webextension one (https://addons.mozilla.org/de/firefox/addon/ecosia-the-green-search/versions/?page=1#version-4.0.0) and when upgrading Firefox never marks that entry as provided by a modern webextension addon.
This really seems less than ideal, as users have upgraded to a webextensions version of the addon and should be treated accordingly.
(Reporter)

Comment 2

a year ago
I tested this some more and it seems to be the exact same issue even if not using the same name. On upgrade to 57 the webextension version is installed and a new entry is added to the list of search engines, but the one provided by the legacy addon is still there and still selected as default, even if "is_default" is used in the webextension addon. Upgrading this setup to 57.0.1 will still reset the default search to google.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

a year ago
Analyst at Ecosia here.

Currently lost 50% of our FF userbase in this update up (57.0 to 57.0.1) until today.
That equates to around 8.4m searches since Dec 1st & right now around we're losing 600k daily, expectant to grow to 1.2m daily lost when propagation of 57.0.1 is complete.

30% of our userbase use Firefox, mostly environmentally-aware Europeans.

We understand your efforts to remove unwanted search engines, but Ecosia has never used such tactics to gather searches.
Duplicate of this bug: 1425825
Severe regression, we probably want to take a fix in 58 for that.

gerry, ni FYI.
Mike, please provide a fix asap. Thanks
status-firefox58: --- → affected
tracking-firefox58: --- → blocking
Flags: needinfo?(mozilla)
Flags: needinfo?(gchang)
(sorry, we have a patch, I missed the mozreview-request)
Flags: needinfo?(mozilla)
(Reporter)

Comment 8

a year ago
Mike, if I understand the patch correctly, this would fix the upgrade process when coming from a legacy addon and migrating to a webextension one and would correctly mark the search entry as no longer legacy, right? But this code will not run anymore if that upgrade already happened, will it?
Sadly most of our users (and from what I see in other bug tickets the users of a lot of other legitimate search providers) have already been reset as everybody has already been updated to the new Firefox version. Is there any way to restore these users?
Component: Untriaged → Search

Comment 9

a year ago
Mike,

This is Vivek Subramanian from Symantec.

As Dominik explained above, we also see lot of customers already impacted because of this issue.

Is there way firefox can restore already affected users ?

Comment 10

a year ago
mozreview-review
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

https://reviewboard.mozilla.org/r/208450/#review214630

I would like this to be covered by automated tests. I'm not sure how much effort it would take here, but if you follow my suggestion of moving this to the search service, we should be able to do it reasonably well with xpcshell tests.

::: browser/components/extensions/ext-chrome-settings-overrides.js:218
(Diff revision 1)
> -        extensionID: extension.id,
> +      extensionID: extension.id,
> -        suggestURL: searchProvider.suggest_url,
> +      suggestURL: searchProvider.suggest_url,
> -        queryCharset: "UTF-8",
> +      queryCharset: "UTF-8",
> -      };
> +    };
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name.trim(), params);

nit: let name = searchProvider.name.trim(); to avoid duplication.

::: browser/components/extensions/ext-chrome-settings-overrides.js:223
(Diff revision 1)
> +      Services.search.addEngineWithDetails(searchProvider.name.trim(), params);
> +    } catch (e) {
> +      // Check if previous engine was actually from an old extension. If so,
> +      // Remove and replace. In theory, this should only need to be done for
> +      // one release.
> +      let engineOld = Services.search.getEngineByName(searchProvider.name.trim());

The engine already existing isn't the only reason that can make addEngineWithDetails fail, so I think we need to nullcheck engineOld here.

::: browser/components/extensions/ext-chrome-settings-overrides.js:224
(Diff revision 1)
> +    } catch (e) {
> +      // Check if previous engine was actually from an old extension. If so,
> +      // Remove and replace. In theory, this should only need to be done for
> +      // one release.
> +      let engineOld = Services.search.getEngineByName(searchProvider.name.trim());
> +      if (engineOld.wrappedJSObject._loadPath.startsWith(`jar:[profile]/extensions/${extension.id}`)) {

I assume you ensured that an updated add-on always keeps the same extension.id.

I would prefer this to be an early return, to avoid indenting all the code block.

I dislike that this makes webextension code mess with the load path, which is something meant to be an internal implementation detail of the search service (which is the reason why you have to .wrappedJSObject._ here). Given that addEngineWithDetails already receives the extensionID field, shouldn't the search service take case of figuring out that we are attempting to update an extension provided plugin?

And actually, this last point also applies to the code block above that removes an existing engine with the same add-on id. Shouldn't that also be handled automatically (including preserving the index and current status) when calling addEngineWithDetails?
Attachment #8937743 - Flags: review?(florian) → review-
Flags: qe-verify+
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

https://reviewboard.mozilla.org/r/208450/#review214630

Test added.
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Due to a bug in how we migrated default search engines during the transition from Firefox 56 to Firefox Quantum, when we attempted to reset the search engines of add-ons that were no longer enabled after users upgraded to Firefox Quantum (bug 1419941) some WebExtensions were included. This affected users that had a legacy extension installed before the WebExtension and were migrated to it; it did NOT affect users that had opted-in to using a search engine when installing a WebExtension for the first time.

As part of this bug, we’re putting a fix in to properly migrate these search engines to be identified as belonging to a WebExtension so this doesn’t happen again.

If you’ve been affected by this, you have the option of engaging with your users to change the default search engine. This can be accomplished by right clicking on the appropriate search icon in the URL bar search bar dropdown and selecting “Set As Default Search Engine.” They do not need to go to preferences.
(Reporter)

Comment 14

a year ago
(In reply to Mike Kaply [:mkaply] from comment #13)
> Due to a bug in how we migrated default search engines during the transition
> from Firefox 56 to Firefox Quantum, when we attempted to reset the search
> engines of add-ons that were no longer enabled after users upgraded to
> Firefox Quantum (bug 1419941) some WebExtensions were included. This
> affected users that had a legacy extension installed before the WebExtension
> and were migrated to it; it did NOT affect users that had opted-in to using
> a search engine when installing a WebExtension for the first time.
> 
> As part of this bug, we’re putting a fix in to properly migrate these search
> engines to be identified as belonging to a WebExtension so this doesn’t
> happen again.
> 
> If you’ve been affected by this, you have the option of engaging with your
> users to change the default search engine. This can be accomplished by right
> clicking on the appropriate search icon in the URL bar search bar dropdown
> and selecting “Set As Default Search Engine.” They do not need to go to
> preferences.

Thanks for the update. And thank you for the fix. Will this also mark search provider entries as provided by a webextension for users that already updated to webextensions before this fix was released or only for users that update from a legacy extension directly to FF 58?

Also, as you can probably imagine, nagging our users to manually change their search settings is less than ideal for both us and them. Is there no way to detect this case after it happened and restore the previous default search?

Comment 15

a year ago
Hi,

Is the fix available in Firefox Beta / Nightly / Developer version ?
Not yet, the patch has to be reviewed before it lands.
Then, we will discuss about uplifting it to beta/devedition.

Comment 17

a year ago
Re-iterating the above question from the bug reporter.

Will this also mark search provider entries as provided by a webextension for users that already updated to webextensions before this fix was released ?

OR

Only for users that update from a legacy extension directly to FF 58?

Also, as you can probably imagine, nagging our users to manually change their search settings is less than ideal for both us and them. Is there no way to detect this case after it happened and restore the previous default search?

Comment 18

a year ago
mozreview-review
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

https://reviewboard.mozilla.org/r/208450/#review215992

Looks much better than the previous version :-)

::: toolkit/components/search/nsSearchService.js:3902
(Diff revision 2)
>        FAIL("Invalid name passed to addEngineWithDetails!");
>      if (!params.template)
>        FAIL("Invalid template passed to addEngineWithDetails!");
> -    if (this._engines[aName])
> +    if (this._engines[aName]) {
> +      if (params.extensionID &&
> +          this._engines[aName]._loadPath.startsWith(`jar:[profile]/extensions/${params.extensionID}`)) {

this._engines[aName] here is duplicated, you can simplify by moving the "let engine = ..." line before the "if (params.extensionID &&" line.

::: toolkit/components/search/nsSearchService.js:3904
(Diff revision 2)
>        FAIL("Invalid template passed to addEngineWithDetails!");
> -    if (this._engines[aName])
> +    if (this._engines[aName]) {
> +      if (params.extensionID &&
> +          this._engines[aName]._loadPath.startsWith(`jar:[profile]/extensions/${params.extensionID}`)) {
> +        // This is a legacy extension engine that needs to be migrated to WebExtensions.
> +        let engine = this._engines[aName];

I'm surprised this doesn't make eslint complain about this variable shadowing the "var engine" from the upper scope. Still, it would probably be less confusing to rename the first 'engine' into 'existingEngine' and the second 'engine' into 'newEngine'.

::: toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js:7
(Diff revision 2)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const kSearchEngineID = "addEngineWithDetails_test_engine";
> +const kSearchEngineURL = "http://example.com/?search={searchTerms}";

I would replace these 6 const with:
const kSearchEngineDetails = {
  template: ...
to reduce the duplication when calling Services.search.addEngineWithDetails twice.

::: toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js:15
(Diff revision 2)
> +const kDescription = "Test Description";
> +const kAlias = "alias_foo";
> +const kExtensionID = "test@example.com";
> +
> +add_task(async function test_migrateLegacyEngine() {
> +  do_check_false(Services.search.isInitialized);

do_check_false and do_check_eq no longer exist on central, since bug 1421992 landed.

::: toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js:24
(Diff revision 2)
> +  Services.search.addEngineWithDetails(kSearchEngineID, {
> +    template: kSearchEngineURL,
> +    description: kDescription,
> +    iconURL: kIconURL,
> +    suggestURL: kSearchSuggestURL,
> +    alias: "alias_foo",

Why is this not using the kAlias constant?

::: toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js:30
(Diff revision 2)
> +    extensionID: kExtensionID,
> +  });
> +
> +  // Modify the loadpath so it looks like an legacy plugin loadpath
> +  let engine = Services.search.getEngineByName(kSearchEngineID);
> +  engine.wrappedJSObject._loadPath = `jar:[profile]/extensions/${kExtensionID}.xpi!/engine.xml`;

I would also set engine.wrappedJSObject._extensionID to null.

::: toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js:43
(Diff revision 2)
> +    alias: "alias_foo",
> +    extensionID: kExtensionID,
> +  });
> +
> +  engine = Services.search.getEngineByName(kSearchEngineID);
> +  do_check_eq(engine.wrappedJSObject._loadPath, "[other]addEngineWithDetails:" + kExtensionID);

I would Assert.equal(engine.wrappedJSObject._extensionIDj, kExtensionID);
Attachment #8937743 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (obsolete)

Comment 21

a year ago
mozreview-review
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

https://reviewboard.mozilla.org/r/208450/#review216192

Looks good to me.

(Ignore comment 20, this doesn't cause a warning.)
Attachment #8937743 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
Hi :mkaply, 
We are about to go to RC next week, do you have any updates?
Flags: needinfo?(gchang) → needinfo?(mozilla)
(Assignee)

Comment 24

a year ago
Sorry, I was holding off on a separate issue that is now resolved. I'll get this landed now.
Flags: needinfo?(mozilla)

Comment 25

a year ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/a3e930a4edfd
Migrate legacy search engines to WebExtensions. r=florian

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3e930a4edfd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
(Assignee)

Comment 27

a year ago
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

Approval Request Comment
[Feature/Bug causing the regression]: WebExtension search
[User impact if declined]: When a WebExtension is uninstalled, the corresponding engine doesn't get installed.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Non
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Only affects this specific scenario, has automated test.
[String changes made/needed]:
Attachment #8937743 - Flags: approval-mozilla-beta?
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

Important fix. Beta58+.
Attachment #8937743 - Flags: approval-mozilla-release+
Attachment #8937743 - Flags: approval-mozilla-beta?
Attachment #8937743 - Flags: approval-mozilla-beta+
I have managed to verify this bug using the STR from comment 0, on 58.0 RC2 (20180115093319) across platforms: Windows 10 x64, Mac OS 10.11.6 and Ubuntu 16.04 x64. 

I can confirm that the search engine remains the one provided by the Webextension version of the addon. Tested with the Ecosia search engine linked in comment 1.
status-firefox58: fixed → verified
After discussing with Mike on Slack, it seems that are still some problems affecting the Ecosia's Add-on migration in Firefox 59, and it will be addressed in bug 1439600.

Considering that this bug depends on the above bug, I'll leave this opened until bug 1439600 is resolved.
Depends on: 1439600
I’ve re-tested this bug using the STR provided in bug 1439600 comment 0, on latest Beta 59.0b14 under Win 10 x64 and macOS 10.12
I can confirm that after bug 1439600 landed the Ecosia Add-on properly migrates to the WebExtension on latest Beta.

However, it seems that Ecosia is not remaining the default search engine and is being reset to Google after opening the same profile (previously created on FF 56.0.2) with latest Beta 59.0b14.

Can you please take a look at this, Mike?
Flags: needinfo?(mozilla)
(Assignee)

Comment 34

a year ago
> However, it seems that Ecosia is not remaining the default search engine and is being reset to Google after opening the same profile (previously created on FF 56.0.2) with latest Beta 59.0b14.

> Can you please take a look at this, Mike?

Yes, I will investigate this.
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.