1181645 - search service's getSubmission should not treat "purpose" as optional.

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[Mike Connor [:mconnor]]

This is a footgun, based on how many partner code setups work.  bug 1181605 is one example of this being a footgun.

The easy fix is to default to "searchbar" if there's no purpose passed, similar to how we assume text/html if unspecified.  The slightly harder fix is to change the API to obsolete the footgun API with optional args, and deprecate getSubmission.  That'd probably impact mobile more, since that's the only non-HTML/non-suggestion use for responseType, but I think the disconnect between how mobile and desktop handle similar cases is relatively busted and another form of risk.

Not blocking any release, but we should get on top of this ASAP.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Patch to default to searchbar

This patch does the first thing  Mike requested which is default searches without a purpose or with an invalid purpose to searchbar.

It did require some major test changes.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8745598 [details] [diff] [review]
Patch to default to searchbar

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

The patch looks fine (I would like to see green try results for browser-chrome and xpcshell tests before you push it), but it doesn't do what you describe. It'll only fallback to searchbar for purposes not specified in the searchplugin, but keeps a special handling of the empty/unspecified purpose.

If you want to remove this special behavior, you should change "var purpose = aPurpose || "";" to "var purpose = aPurpose || "searchbar";", and adapt http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_purpose.js?rev=2bc25894eced#46

I think the new behavior you implement needs to be covered by test_purpose.js anyway (lack of xpcshell test coverage is the reason for not r+'ing).

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Update purpose xpcshell test

Addressed comments.

Working on the try build now.

Comment 4

[Mike Kaply [:mkaply]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=760858284835

Comment 5

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/884060dc6f21feb4da594d02e63e5ded5700fe87
Bug 1181645 - Default invalid or empty purpose in getSubmission to searchbar, r=florian.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/884060dc6f21

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch, as landed

Approval Request Comment
[Feature/regressing bug #]: Bug 1266462. This patch is the clean/correct fix to replace for 47 and later the workaround that landed in bug 1266462.
[User impact if declined]: No direct user impact, the impact is sending incorrect partner codes for searches from the location bar.
[Describe test coverage new/current, TreeHerder]: The search service changes in the patch are covered by the test_purpose.js changes in the patch. The google.xml changes are covered by patches from bug 1266783.
[Risks and why]: Acceptable for beta in my opinion. The patch slightly changes how we handle purpose parameters in search plugins, but to the best of my knowledge no search plugin ever used the behaviors we removed. It's slightly higher risk than the workaround we landed in bug 1266462, but a much cleaner long term solution.
[String/UUID change made/needed]: none.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8747658 [details] [diff] [review]
Patch, as landed

This is something planned for Fx47, the sooner we uplift the better, also happy to see the patch includes new automated tests, Beta47+, Aurora48+

Comment 9

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2817a27cf29b

Comment 10

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

https://hg.mozilla.org/releases/mozilla-beta/rev/c14e8b868634