Closed Bug 862401 Opened 7 years ago Closed 6 years ago

keyword search is broken for search engines using POST

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 + fixed

People

(Reporter: FeuerFliege, Assigned: mikedeboer)

References

()

Details

Attachments

(1 file, 18 obsolete files)

25.14 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 738818 landed and removed keyword.URL. The locationbar should use the search engine which is selected in the searchbar.

This requires the firefox specific <SearchForm> element to be present in the OpenSearch plugins, but since it is not part of the OpenSearch specification it is missing for many search engines like DuckDuckGo, Wolfram|Alpha, Flickr, yasni, ixquick, Startpage, etc.

Without keyword.URL it is not easy to restore this ability and the locationbar search is broken for user of those search engines.
Gavin, is there anything obvious we can do to ameliorate here or is this going to be something that requires updated search plug-ins?
FYI, I'm not seeing this problem with Duck Duck Go.

SearchForm is used for clicking on search when no search term is specified

It's not used for keyword search.
(In reply to Michael Kaply (mkaply) from comment #2)
> FYI, I'm not seeing this problem with Duck Duck Go.

I'm not either.

> SearchForm is used for clicking on search when no search term is specified

By "clicking on search", we mean clicking on the magnifying glass icon on the right of the search bar.

One problem that I noticed is that immediately after adding a third-party search engine, e.g. DuckDuckGo, even though it is shown as selected in the search bar, it doesn't work in the URL bar. This is a bug in our code. I think we're not listening to serach-engine-added/removed when we should be. Mike, could you investigate that?
(In reply to Frank Yan (:fryn) from comment #3)
> One problem that I noticed is that immediately after adding a third-party
> search engine, e.g. DuckDuckGo, even though it is shown as selected in the
> search bar, it doesn't work in the URL bar. This is a bug in our code. I
> think we're not listening to serach-engine-added/removed when we should be.
> Mike, could you investigate that?

bug 860560 should address this.
For DuckDuckGo, which doesn't have a <SearchForm/>, when I click the magnifying glass icon, it sends me to https://duckduckgo.com/, because we just use the search URL template's prePath:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2455

I'm pretty sure this bug is actually INVALID.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)

Great! :)
(In reply to Florian J. [:FeuerFliege] from comment #0)
> Bug 738818 landed and removed keyword.URL. The locationbar should use the
> search engine which is selected in the searchbar.
> 
> This requires the firefox specific <SearchForm> element to be present in the
> OpenSearch plugins

As Mike mentioned, this isn't true. So you're likely seeing some other issue. What are the values of your browser.search.defaultenginename and browser.search.selectedEngine?
I was wrong, I am very sorry.

I thought I only added SearchForm element but I also changed the method parameter. Later I looked for plugins with missing SearchForm elements assuming those wouldn’t work, too :(

Actually it is broken if the plug uses the POST instead of the GET method which is quite rare (Startpage/ixquick are using this as default)
(In reply to Florian J. [:FeuerFliege] from comment #7)
> Actually it is broken if the plug uses the POST instead of the GET method
> which is quite rare (Startpage/ixquick are using this as default)

That's good info. I'll check that and update the bug.
Good catch! indeed this is a difficult issue, not sure how to best fix at the moment :/
Blocks: 738818
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Keyword search is broken for many search engines → keyword search is broken for search engines using POST
Probably the best way forward is adjusting the nsIURIFixup::keywordToURI API to allow passing back post data to submit, in addition to just returning a URI.
here's a page for the iquick
Assignee: nobody → mdeboer
Attachment #738811 - Attachment is obsolete: true
Attachment #738811 - Flags: review?(gavin.sharp)
Attachment #738836 - Flags: superreview?(bzbarsky)
Attachment #738836 - Flags: review?(gavin.sharp)
Comment on attachment 738836 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data

Should the new outparam be optional to avoid breaking existing JS consumers?

sr=me with that
Attachment #738836 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 738836 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data

r=me with the argument optional, as bz suggests.

We should probably file a followup on whether the other callers should better handle the received postData.
Attachment #738836 - Flags: review?(gavin.sharp) → review+
Status: NEW → ASSIGNED
aPostData now optional. Carrying over sr=bz, r=gavin.
Attachment #738836 - Attachment is obsolete: true
Attachment #740251 - Flags: superreview+
Attachment #740251 - Flags: review+
Comment on attachment 740251 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data

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

::: docshell/base/nsIURIFixup.idl
@@ +63,5 @@
>       * caller's responsibility to check whether keywords are enabled and
>       * whether aKeyword is a sensible keyword.
> +     *
> +     * @param aKeyword The keyword string to convert into a URI
> +     * @param aPostData The POST data that might come paired with the returned 

trailing whitespace
thanks to tips from bz: aPostData now truly optional
Attachment #740251 - Attachment is obsolete: true
Attachment #740740 - Flags: superreview+
Attachment #740740 - Flags: review+
Attached patch WIP: unit test (obsolete) — Splinter Review
Gavin, it would be awesome if you could figure this one out... and afterwards explain to me how this works! :) Kidding, but any input would be much appreciated.
Attached patch alternate browser chrome test (obsolete) — Splinter Review
I was playing around with this and decided to write a test using a bit of a different approach (just use the sjs test server, simplify the invocation of the search, some other changes).

Debugging the failure, I think the root cause is that in the common case, URL bar searches actually goes through nsDefaultURIFixup::CreateFixupURI, and that's one of the cases the patch doesn't handle.

I think the only way to address that is to add an optional postData argument to createFixupURI as well.
Gavin: good news! I'll get to that! However, I think you re-uploaded the implementation patch and not your newly created sjs unit test... could you upload that one instead?
Attachment #740740 - Attachment is obsolete: true
Attachment #742352 - Flags: superreview?(bzbarsky)
Attachment #742352 - Flags: review?(gavin.sharp)
Attached patch unit test coverage (obsolete) — Splinter Review
Attachment #741015 - Attachment is obsolete: true
Attachment #742354 - Flags: review?(gavin.sharp)
Mind posting an interdiff from the last thing that was reviewed?
Attached patch alternate browser chrome test (obsolete) — Splinter Review
Hah, indeed! Here's the real test.
Attachment #742193 - Attachment is obsolete: true
Attached patch alternate browser chrome test (obsolete) — Splinter Review
(with the search engine included this time. sigh)
Attachment #742411 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Boris: this patch shows the things I changed to allow propagation of POST data in more scenarios, compared to the one scenario I implemented before.

I created the diff manually, so I hope it's clear enough.
Flags: needinfo?(mdeboer)
What does "created manually" mean?  Was it just done by doing a diff between the file with the one patch applied and the file with the other patch applied?
"created manually" means that I took the previous patch and the new one, and created a new patch file that can be applied when the previous has been applied already. In other words: you can apply the patch I just attached on top of previous one and you'll end up with the state as defined in attachment 742352 [details] [diff] [review].
Ah, ok.  That's exactly what I was looking for, yes.  ;)

Digging out from review request pileup over the weekend, but should get to this soon.
Comment on attachment 742352 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data, take 2

>+            aPostStream = static_cast<nsIInputStream*>(postData);

Why is that ok?  Seems like that will leave aPostStream dangling once postData goes out of scope...

You probably want to have an on-stack nsCOMPtr that gets initialized to aPostStream and changed here as needed and used elsewhere in this function.
Attachment #742352 - Flags: superreview?(bzbarsky) → superreview-
Fixed issues as per bz's comments
Attachment #742352 - Attachment is obsolete: true
Attachment #742352 - Flags: review?(gavin.sharp)
Attachment #744074 - Flags: superreview?(bzbarsky)
Attachment #744074 - Flags: review?(gavin.sharp)
Attached patch unit test coverage (obsolete) — Splinter Review
Mixed Gavin's alternate unit test into mine
Attachment #744075 - Flags: review?(gavin.sharp)
Attachment #742354 - Attachment is obsolete: true
Attachment #742354 - Flags: review?(gavin.sharp)
Attachment #743008 - Attachment is obsolete: true
Comment on attachment 744074 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data, take 2

sr=me
Attachment #744074 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 744074 [details] [diff] [review]
Make sure nsDefaultURIFixup::KeywordToURI propagates POST data, take 2

Hmm, overridding aPostStream in the keyword case seems a bit weird, but I guess you can already control that with LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP, and it certainly wouldn't make sense to send the original postData to the keyword URI.

Am I right that the previous behavior was to send the passed-in postData to the search engine URL? That seems broken, so I guess it's good that we're fixing this!
Attachment #744074 - Flags: review?(gavin.sharp) → review+
The new behavior is the same _unless_ the search engine provides a post stream....
Oh, true. Seems like we should instead pass "postStream" to CreateFixupURI unconditionally?
Well, we should at least clear it whenever we end up using the URI returned by CreateFixupURI...
Right, CreateFixupURI would do that for us (at least in the cases where it ends up calling KeywordToURI, with the current patch). Arguably, there are cases where it might make sense to continue sending the original postData to the URI returned by CreateFixupURI (e.g. when it's only slightly modifying the passed-in URI). But trying to reason about that is complicated, so the simple and more explicit behavior of just always replacing postData when we're (potentially) replacing the URI seems better.
Comment on attachment 744075 [details] [diff] [review]
unit test coverage

I would prefer sticking with the SJS rather than rolling your own HTTP server. But I would welcome a followup bug to add the ability to register simple handlers dynamically on the existing mochitest server, without having to use an SJS.

The windowObserver only exists in the old test to handle the failure case from bug 595509. Since that isn't the expected failure case for this test, you shouldn't need to add it here.

The old test uses an nsIWebProgressListener because it doesn't install its own engine and just uses the default (Google). We don't want the test to rely on the load of google.com firing (not a local resource, internet connectivity on test slaves can be choppy), so I was using an onStateChange to check for STATE_START (start of the request). Since this test uses a custom engine with a local URL, there's no need for that, you can just use a simpler normal load listener like in my test.

We really need to stop cargo-culting (and remove other instances of) that "// Cause service initialization" comment, it's not accurate after bug 722332. "Services.search" is short enough that there's no need for a local variable, too.

My test removed the added search engine in the cleanup function, it looks like this version lost that bit.

Adding coverage for both the "? foo" and "foo search" cases is better than my test, which only covered one of those.
Attachment #744075 - Flags: review?(gavin.sharp) → review-
Comment on attachment 744075 [details] [diff] [review]
unit test coverage

(also lets add a new line to the end of POSTSearchEngine.xml :)
Attached patch unit test coverage (obsolete) — Splinter Review
Thanks for the feedback Gavin and apologies for this taking a tad longer!
Attachment #742418 - Attachment is obsolete: true
Attachment #744075 - Attachment is obsolete: true
Attachment #745072 - Flags: review?(gavin.sharp)
Comment on attachment 745072 [details] [diff] [review]
unit test coverage

>diff --git a/browser/base/content/test/browser_keywordSearch.js b/browser/base/content/test/browser_keywordSearch_postData.js

> function doTest() {

>+  waitForLoad(function () {
>+    let loadedText = gBrowser.contentDocument.body.innerHTML.replace(/<\/?[^>]+>/gi, "");
>+    ok(loadedText, "search page loaded");
>+    info("Loaded text: " + loadedText + ", " + gCurrTest.testText);
>+    // Trim and sanitize needle to match the query string as passed to the engine
>+    let needle = "searchterms=" + gCurrTest.testText.replace(/(^[\s]+|[\s]+$)/, "")
>+                                              .replace(/\?[\s]*/, "")
>+                                              .replace(/[\s]+/g, "+");

You can use .trim() instead of a regexp. But I would simplify and put the "expected encoded output" as a property next to testText in gTests, and avoid the need to jump through these hoops.

r=me with that.
Attachment #745072 - Flags: review?(gavin.sharp) → review+
Attached patch unit test coverage (obsolete) — Splinter Review
Changed unit test expected results handling. Carrying over r+=gavin
Attachment #745072 - Attachment is obsolete: true
Attachment #745807 - Flags: review+
Comment on attachment 745807 [details] [diff] [review]
unit test coverage

>diff --git a/browser/base/content/test/browser_keywordSearch.js b/browser/base/content/test/browser_keywordSearch_postData.js

> function doTest() {
>   info("Running test: " + gCurrTest.name);
> 
>+  waitForLoad(function () {
>+    let loadedText = gBrowser.contentDocument.body.innerHTML.replace(/<\/?[^>]+>/gi, "");

Using body.textContent should avoid the need for the regexp.
Gavin: I tried, but didn't work. I saw you used it in your tests as well, but after copying the lines and running the test, they failed for me. I didn't take the time to investigate why.
Attached patch unbitrotted patch, tweaked patch (obsolete) — Splinter Review
Weird, the test passes for me with that change. I also addressed comment 41, unbitrotted the patch, and folder the test changes into it.
Attachment #744074 - Attachment is obsolete: true
Attachment #744078 - Attachment is obsolete: true
Attachment #745807 - Attachment is obsolete: true
Attachment #746601 - Flags: feedback?(mdeboer)
Comment on attachment 746601 [details] [diff] [review]
unbitrotted patch, tweaked patch

Works for me too! Ran the tests and in-browser manual test. Yay and thanks!
Attachment #746601 - Flags: feedback?(mdeboer) → feedback+
Attached patch patchSplinter Review
Here's an updated patch that takes a bit of a different approach to fixing the issue from comment 37.

The main changes:
- nsIURIFixup::keywordToURI now unconditionally clobbers its outparams (both aURI and aPostData). It was already doing this for aURI, so I figure it makes sense to do it for aPostData too (when specified).
- changed the callers of that method in nsDocShell to depend on that behavior.
- made nsDefaultURIFixup::KeywordURIFixup return void since I kept confusing it with keywordToURI and it only has one caller (who doesn't care about its useless nsresult return value)
- adjusted some comments in nsIURIFixup and in nsDefaultURIFixup::KeywordToURI

This does not fix the pre-existing issue that in LoadURI, we can end up sending the passed-in aPostStream to a a URI that's been munged by CreateFixupURI. However, it does somewhat reduce the likelihood of that happening, because one of the more primary ways that CreateFixupURI can change the URI (by calling keywordToURI) now does clobber the passed-in post data.

bz, I'll attach an interdiff that covers just these changes.
Attachment #746601 - Attachment is obsolete: true
Attachment #750193 - Flags: review?(bzbarsky)
Comment on attachment 750193 [details] [diff] [review]
patch

r=me
Attachment #750193 - Flags: review?(bzbarsky) → review+
Flags: in-testsuite+
Target Milestone: --- → Firefox 24
Comment on attachment 750193 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): exposed by bug 738818
User impact if declined: some searches with custom search engines from the URL bar won't work
Testing completed (on m-c, etc.): manually, automated test coverage
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: changes nsIURIFixup.idl in a JS-compatible way, bumps the IID accordingly (should be fine for aurora)
Attachment #750193 - Flags: approval-mozilla-aurora?
Attachment #750194 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8b36d359f889
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #750193 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 873755
Depends on: 873857
Depends on: 875223
Depends on: 874870
No longer depends on: 874870
No longer depends on: 875223
You need to log in before you can comment on or make changes to this bug.