API addition: add extensionID parameter to addEngineWithDetails

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mconnor, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

unspecified
Firefox 34
dev-doc-needed
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Points: --- → 3
QA Whiteboard: [qa-]
(Assignee)

Updated

5 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 34.1
OS: Windows 8.1 → All
Hardware: x86_64 → All
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?

(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?
Attachment #8460778 - Flags: review?(mconnor)
Forgot to bump the UUID for nsIBrowserSearchService. Just imagine I did that and the next patch will include it.
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Updated

5 years ago
Blocks: 1042938
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).
Attachment #8460778 - Flags: review?(mconnor) → review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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).
(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.
Iteration: 34.1 → 34.2
If this is just blocked by testing concerns, those can be split into another bug.
Flags: needinfo?(ttaubert)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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: gavin@gavinsharp.com] 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
Flags: needinfo?(ttaubert)
(Assignee)

Updated

5 years ago
Depends on: 1055492
Had to back out unfortunately because I didn't push to try before:

https://hg.mozilla.org/integration/fx-team/rev/f39a5425d95d
Created attachment 8475230 [details] [diff] [review]
0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch

Had to adapt a few addEngineWithDetails() calls.
Attachment #8475230 - Flags: review?(gavin.sharp)
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
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 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.
Attachment #8475230 - Flags: review?(gavin.sharp) → review-
Another push to try with the parameter being optional:

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

Updated

5 years ago
Attachment #8475230 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4ed05a6cb24a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
This was never added to the docs.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.