1040931 - API addition: add extensionID parameter to addEngineWithDetails

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Mike Connor [:mconnor]]

Because packed extensions aren't supported for bundled extensions, it's become common to use addEngineWithDetails as a workaround.  However, this effectively means those engines won't be removed when the add-on is, which isn't the right user experience (and is a potential form of abuse).

To manage this with minimal pain, we should add (and require by policy for now) that add-ons calling this method must also pass in their correct add-on ID.  The search service should then watch for add-on notifications and remove added plugins if/when the plugin is removed or blocked.  In addition, we should validate that the add-on ID is still present whenever we're regenerating search.json.

Nominating for backlog based on discussion with Gavin, and cc-ing Jorge/Kris so they know this is coming.  Probably won't happen in 33, but should ideally be in an early 34 iteration.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1040931-Add-extensionID-parameter-to-addEngineWi.patch

I assume that the observer should be added in a separate bug?

(In reply to Mike Connor [:mconnor] from comment #0)In addition,
> we should validate that the add-on ID is still present whenever we're
> regenerating search.json.

How can we validate that a search engine in the JSON file should have an extension ID? AFAICT we keep all search engines (default and non-default ones) in search.json, no?

Should we compare it to the list of default search engines? Or is search.json the list of engines we ship?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Forgot to bump the UUID for nsIBrowserSearchService. Just imagine I did that and the next patch will include it.

Comment 3

[Mike Connor [:mconnor]]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> Created attachment 8460778 [details] [diff] [review]
> 0001-Bug-1040931-Add-extensionID-parameter-to-addEngineWi.patch
> 
> I assume that the observer should be added in a separate bug?

Yeah, that makes sense to me.  I can file it.

> Should we compare it to the list of default search engines? Or is
> search.json the list of engines we ship?

search.json is a cache of all current search engines (to avoid scanning directories, I assume), it gets regenerated when the buildID changes (maybe other times).  I'll include the behaviour in the followup bug.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8460778 [details] [diff] [review]
0001-Bug-1040931-Add-extensionID-parameter-to-addEngineWi.patch

We should have some test coverage that this attribute gets serialized/deserialized correctly (both in the .xml and in the json cache). r=me with that.

I think the next steps here are:
a) validating the passed-in extension ID (i.e. checking that it corresponds to a currently installed add-on, if specified)
b) bug 1042938
c) code that uninstalls/removes any engines whose associated add-ons are no longer installed when the cache is rebuilt (i.e. on upgrade, in the common case)

I think b)/c) can be in followups, but a) should probably land along side this patch (whether or not it gets a new bug).

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #4)
> We should have some test coverage that this attribute gets
> serialized/deserialized correctly (both in the .xml and in the json cache).
> r=me with that.

I'm not at all familiar with the search code and its tests. I managed to write a test that checks that extensionID ends up in search.json but I have no idea how to convince the search service to read from that file after it has already been initialized. I couldn't find any tests for the .xml files either (I think).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> I'm not at all familiar with the search code and its tests. I managed to
> write a test that checks that extensionID ends up in search.json but I have
> no idea how to convince the search service to read from that file after it
> has already been initialized. I couldn't find any tests for the .xml files
> either (I think).

For search.json, you should be able to just tweak test_json_cache.js (and toolkit/components/search/tests/xpcshell/data/search.json) to test both reading and writing that property from the cache. For testing the XML serialization, you should be able to copy test_serialize_file.js but extend it to use addEngineWithDetails, and then actually check the XML for the attribute, rather than just checking that the file exists.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

If this is just blocked by testing concerns, those can be split into another bug.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #7)
> If this is just blocked by testing concerns, those can be split into another
> bug.

Sorry, this slipped off my radar for too long.

(In reply to :Gavin Sharp [email: [e-mail]] from comment #6)
> For search.json, you should be able to just tweak test_json_cache.js (and
> toolkit/components/search/tests/xpcshell/data/search.json) to test both
> reading and writing that property from the cache.

Thanks for the hints, did that and caught a bug (yay). Will file a follow-up to test XML serialization.

https://hg.mozilla.org/integration/fx-team/rev/836c52cc3c05

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Had to back out unfortunately because I didn't push to try before:

https://hg.mozilla.org/integration/fx-team/rev/f39a5425d95d

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch

Had to adapt a few addEngineWithDetails() calls.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=c44cab480373

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8475230 [details] [diff] [review]
0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch

Hrm, why didn't we actually make this optional in IDL? It's confusing to have a comment that says "optional" but having the parameter not actually be. I could see argument for both ways, actually...

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8475230 [details] [diff] [review]
0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch

I think we should make this an optional parameter for now in IDL.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Another push to try with the parameter being optional:

https://tbpl.mozilla.org/?tree=Try&rev=4f181f92ca70

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/4ed05a6cb24a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ed05a6cb24a

Comment 17

[Mike Kaply [:mkaply]]

This was never added to the docs.