Closed Bug 1426081 Opened 6 years ago Closed 6 years ago

Search engine is reset even if WebExtension version is installed

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Version:
57 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox58 blocking verified
firefox59 --- fixed

People

(Reporter: dominik.henter, Assigned: mkaply)

References

Details

Attachments

(1 file)

Bug 1426081 - Migrate legacy search engines to WebExtensions.
59 bytes, text/x-review-board-request
florian
: review+
gchang
: approval-mozilla-beta+
gchang
: approval-mozilla-release+
Details 
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
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.
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.
Assignee: nobody → mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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)
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
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 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+
Comment on attachment 8937743 [details]
Bug 1426081 - Migrate legacy search engines to WebExtensions.

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

Test added.
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.
(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?
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.
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 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 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+
Hi :mkaply, 
We are about to go to RC next week, do you have any updates?
Flags: needinfo?(gchang) → needinfo?(mozilla)
Sorry, I was holding off on a separate issue that is now resolved. I'll get this landed now.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/a3e930a4edfd
Migrate legacy search engines to WebExtensions. r=florian
https://hg.mozilla.org/mozilla-central/rev/a3e930a4edfd
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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)
> 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.

Attachment

General

Creator:
Created:
Updated:
Size: