Closed Bug 1181645 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: Search, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mconnor, Assigned: mconnor)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P3
Whiteboard: [fxsearch]
Rank: 35
Severity: normal → major
Attached patch Patch to default to searchbar (obsolete) — Splinter Review
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.
Attachment #8745598 - Flags: review?(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).
Attachment #8745598 - Flags: review?(florian) → feedback+
Attached patch Update purpose xpcshell test (obsolete) — Splinter Review
Addressed comments.

Working on the try build now.
Attachment #8746632 - Flags: review?(florian)
Attachment #8745598 - Attachment is obsolete: true
Attachment #8746632 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/884060dc6f21feb4da594d02e63e5ded5700fe87
Bug 1181645 - Default invalid or empty purpose in getSubmission to searchbar, r=florian.
https://hg.mozilla.org/mozilla-central/rev/884060dc6f21
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Attached patch Patch, as landedSplinter Review
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.
Attachment #8746632 - Attachment is obsolete: true
Attachment #8747658 - Flags: review+
Attachment #8747658 - Flags: approval-mozilla-beta?
Attachment #8747658 - Flags: approval-mozilla-aurora?
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+
Attachment #8747658 - Flags: approval-mozilla-beta?
Attachment #8747658 - Flags: approval-mozilla-beta+
Attachment #8747658 - Flags: approval-mozilla-aurora?
Attachment #8747658 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.