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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Search
P3
major
Rank:
35
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.

Updated

3 years ago
Priority: -- → P3
Whiteboard: [fxsearch]

Updated

3 years ago
Rank: 35
Severity: normal → major

Comment 1

2 years ago
Created attachment 8745598 [details] [diff] [review]
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.
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+

Comment 3

2 years ago
Created attachment 8746632 [details] [diff] [review]
Update purpose xpcshell test

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.

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/884060dc6f21
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Created attachment 8747658 [details] [diff] [review]
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.
Attachment #8746632 - Attachment is obsolete: true
Attachment #8747658 - Flags: review+
Attachment #8747658 - Flags: approval-mozilla-beta?
Attachment #8747658 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox47: --- → affected
status-firefox48: --- → affected

Comment 8

2 years ago
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+

Comment 9

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2817a27cf29b
status-firefox48: affected → fixed

Comment 10

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c14e8b868634
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.