1042938 - Search service should remove extension-installed search plugins when the add-on is uninstalled/blocked/disabled

Status: RESOLVED WORKSFORME

(Firefox :: Search, defect, P3)

Description

[Mike Connor [:mconnor]]

In bug 1040931 we're adding to the API for addEngineWithDetails so we can verify the source of extension-installed add-ons, and remove them along with the add-on.

To enforce this, the search service will need to do two things:

1) observe add-on notifications and remove any search plugins installed by an add-on that's going away.
2) when rebuilding the cache (search.json) the service should verify that the add-on is still installed and enabled before re-adding it to the cache.

Comment 1

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(How would we even attack this?  I am interested in this as part of other work.  We don't really record / know how searchplugins get added, afaik.)

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

I'm working on this bug.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP

Removes engines installed by an addon when it is uninstalled/disabled, and also excludes engines from the cache if the addons that installed them no longer exist.

Not tested this properly yet, I need an addon that adds a search engine to do so and AMO search seems to be down.

I'll try creating a small addon myself and testing this, will request r? or f? after that.

Comment 5

[Drew Willcoxon :adw]

Looks good to me so far FWIW.  Only questions I have are:

What happens for non-restartless add-ons that require a restart to fully uninstall/disable?  Will they trigger calls to your observer at some point?  I seem to recall the answer is yes, before the restart, but Mossop would know for sure, or maybe the AddonManager docs.

When does _buildCache get called?  In this case we're trying to stop potentially bad behavior, so ideally the bad add-on shouldn't stick around in the cache too long after it's been disabled.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Drew Willcoxon :adw from comment #5)
> Looks good to me so far FWIW.  Only questions I have are:
> 
> What happens for non-restartless add-ons that require a restart to fully
> uninstall/disable?  Will they trigger calls to your observer at some point? 
> I seem to recall the answer is yes, before the restart, but Mossop would
> know for sure, or maybe the AddonManager docs.

The answer is no, you won't see the notifications listened to here at all for non-restartless add-ons.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP 2

This removes engines added a non-restartless addon at shutdown if the addon is flagged for removal.

I need to investigate how the cache works and what exactly it is used for (and when) to decide if anything more (or less) needs to be done regarding that.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP 2.1

Made an if condition slightly easier to read and added a comment.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

I won't be working on this for the foreseeable future. Unassigning.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8643363 [details] [diff] [review]
WIP 2.1

Florian:

How did you feel about this patch so far? Were you OK witht he Task.spawn on the cache in order to be able to use getAddonById? Do we even need to mess with the cache?

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8643363 [details] [diff] [review]
WIP 2.1

Review of attachment 8643363 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mike Kaply [:mkaply] from comment #10)

> How did you feel about this patch so far?

At the time I was unconvinced by the approach because I didn't see a way to encourage (or even require) add-on authors to provide their extension ID when adding a search plugin using the search service API.

But in a WebExtension context this now makes a lot more sense.

> Were you OK witht he Task.spawn on
> the cache in order to be able to use getAddonById? Do we even need to mess
> with the cache?

I would need to review carefully the code in the startup path of the search service, but I assume this 2 years old patch has significantly bitrotted, so probably not worth doing an actual review on the current attachment.

Comment 12

[Mike Kaply [:mkaply]]

Attachment: New patch

New patch based on the old.

I've removed all the stuff related to non restartless add-ons because the expectation that those add-on authors will update their code to use this new param is unrealistic.

Because of that, things are much simpler.

I also didn't take the cache code because it seemed unnecessary.

As far as tests go, I'd like to use the Webextension patch for the testcase here since it's much easier to do dynamic extensions in tests.

So once this is in, I'll remove the engine management code from bug 1301315 which will make it simpler and much more reliable.

I also added the extensionID for addEngine() as well because I plan to add support OpenSearch files in WebExtensions.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8850059 [details] [diff] [review]
New patch

Review of attachment 8850059 [details] [diff] [review]:
-----------------------------------------------------------------

Is it possible for an add-on to be removed without this code ever receiving an onUninstalled or onDisabled call?
Eg. what happens if an add-on is blocklisted. I assume the add-on manager will disable it at startup, before the search service is initialized.
I think to handle this correctly we need to check the status of the add-on for each search engine loaded from the user profile that has an extensionID.

Another thing you may need to worry about is what happens when an add-on containing a search engine is updated. I'm afraid with the current patch the engine will be removed (and its user-set metadata lost), and then re-added. By 'user-set metadata' I mostly mean alias and position. Said differently, this patch may cause bug 1305705 to happen much more frequently.

This feature needs to be covered by tests. The tests should verify that the engines installed using this API have the correct loadpath and extension id, that they still do after a restart of the search service (both using the sync and async code paths), and that engines added this way are correctly removed if the add-on manager no longer has the relevant add-on installed.

I don't know if it's easier to implement this test next to the existing webextensions tests, or next to the search service xpcshell tests (the latter already has helper methods to (re)initialize the search service, and look at its file stored in the profile; we could stub the add-on manager for these tests, as the add-on manager APIs we really care about are tiny).

(and... sorry for the delay on this review! :-( )

::: toolkit/components/search/nsSearchService.js
@@ +4070,5 @@
>        FAIL("addEngine: Error adding engine:\n" + ex, Cr.NS_ERROR_FAILURE);
>      }
>      engine._setIcon(aIconURL, false);
>      engine._confirm = aConfirm;
> +    engine._extensionID = aExtensionID;

We need this extension id to somehow be part of the loadpath, right? I think with the code in the current patch the load path will be generated using only aEngineURL.

Comment 14

[Florian Quèze [:florian]]

Mike, what's the status of this? IIRC this is now handled by the webextension code and if so we can close this bug.

Comment 15

[Mike Kaply [:mkaply]]

You are correct. This is handled by the Webextension code now.