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 :/

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

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

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.

aPostData now optional. Carrying over sr=bz, r=gavin.

How is it optional in attachment 740251 [details] [diff] [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

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.

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?

Mind posting an interdiff from the last thing that was reviewed?

Hah, indeed! Here's the real test.

(with the search engine included this time. sigh)

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.

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.

Fixed issues as per bz's comments

Mixed Gavin's alternate unit test into mine

Comment on attachment 744074 [details] [diff] [review] Make sure nsDefaultURIFixup::KeywordToURI propagates POST data, take 2 sr=me

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!

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.

Comment on attachment 744075 [details] [diff] [review] unit test coverage (also lets add a new line to the end of POSTSearchEngine.xml :)

Thanks for the feedback Gavin and apologies for this taking a tad longer!

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.

Changed unit test expected results handling. Carrying over r+=gavin

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ff0ad5343476

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.

Weird, the test passes for me with that change. I also addressed comment 41, unbitrotted the patch, and folder the test changes into it.

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!

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.

Comment on attachment 750193 [details] [diff] [review] patch r=me

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)