620565 - Adding a keyword for a search with an URL containing '?' yield invalid quicksearch bookmark

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Thomas Jansen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

When trying to set up a quicksearch bookmark for the search function of http://lyrics.wikia.com/Main_Page the resulting bookmark contains a bogus URL. The relevant part of the website is <form action="index.php?title=Special:Search" method="get">. Note the '?' in the action. Rightclicking the search text field, selecting "add a keyword for this search" from the pop-up menu and saving it yields the following bookmark: "http://lyrics.wikia.com/index.php?title=Special:Search?search=%s&fulltext=0". This fails upon use of the newly created bookmark due to the duplicate '?' in the URL. Manually replacing the second '?' with '&' yields the desired result.

Reproducible: Always

Steps to Reproduce:
1. Go to http://lyrics.wikia.com/Main_Page
2. Locate the search field to the right.
3. Rightclick the text field
4. Select "add a keyword for this search" from the pop-up menu
5. Assign a keyword
6. Click "Save"
7. Inspect the properties (in particular URL) of the newly created bookmark.
8. Use that quicksearch
Actual Results:  
7. http://lyrics.wikia.com/index.php?title=Special:Search?search=%s&fulltext=0
8. Get an error page due to the duplicate '?'.

Expected Results:  
7. http://lyrics.wikia.com/index.php?title=Special:Search&search=%s&fulltext=0
8. Actual search results.

Suggestion: Subsitute the second '?' by '&' when creating the quicksearch bookmark. I'm not familiar enough with the details of URL composition to know if a simple algorithm like "If URL already contains a '?' append '&', otherwise append '?'" is sufficient for this kind of problem or if a more elaborate approach is necessary.

Comment 1

[Kevin Brosnan]

I think this is an issue with Wika. Doing the same on Wikipedia http://en.wikipedia.org/wiki/Special:Search creates a correct bookmark keyword.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The form elemens action attribute already specifies a question mark for the target: "index.php?title=Special:Search". I would assume that we do not check if a question mark is already present in the URL and add it either way. We should check if a '?' is present and use an ampersand in such a case.

Comment 3

[Thomas Jansen]

I agree with Henrik, checking for '?' should be sufficient. In the interest of refactoring/reusing existing code: The code for building the correct URL should already be available, as using the search field manually works as intended. It just needs to be applied when a search bookmark is generated.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The problem is here, when the URL is not encoded:
http://hg.mozilla.org/mozilla-central/annotate/7f2b60765d01/browser/base/content/browser.js#l6154

> spec += "?" + formData.join("&");

We should check the value of spec first, and only appending "?" if it's really necessary. Looks like a trivial patch.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#846 has similar code.

Comment 6

[Matthew McPherrin]

Attachment: Add check for "?" in form URL

First, this is my first patch, sorry if I've done anything incorrectly.

Patch checks for "?" in URL and and creates URL with "&" instead.

Comment 7

[Mehdi Mulani [:mmm] (I don't check this)]

Comment on attachment 523763 [details] [diff] [review]
Add check for "?" in form URL

Hey Matt, you probably want Gavin to review this patch instead of Henrik.
(You can see module owners and peers here: https://wiki.mozilla.org/Modules/Firefox )

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 523763 [details] [diff] [review]
Add check for "?" in form URL

Matthew: thanks for the patch! Looks good.

This method is somewhat buggy/hacky in general and could use a rewrite, but this fixes the immediate problem in the interim.

We should add a test for this - this is testable using the browser chrome test harness. The test just needs to load a page with a form whose action contains a "?", then set document.popupNode to one of that form's inputs, and then call AddKeywordForSearchField directly and check the result (it might be helpful to factor out the code that produces the spec/postData as a helper so that you could just call it and easily test the result). browser_bug423833.js is an example of a test that does similar things.

Matthew, if you wanted to take on writing the test I'd be happy to provide guidance (as I'm sure would Mehdi, or anyone else on #fx-team or #developers on IRC). Otherwise I can try to do it myself and then check this in some time soon.

Comment 9

[Matthew McPherrin]

I'll look into testing later this week. Mehdi has offered to assist me with that, so I'll ping him. Thanks for the pointers on how to get started, Gavin.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Matthew, would you still like to give this a try? It looks like it is super close to done.

Comment 11

[Hussain]

Hi 
Can you please assign thus bug to me?

Comment 12

[Kevin Brosnan]

There is already a patch on this bug. It may still work. Though it is likely to need to be updated to fit in the current code base (unbitrotted). The main bit here is to create a test for the patch.

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Comment 13

[Tristan]

Hello all. I would love to write the test for this patch, but I will need a little help with the process. Anyone willing to mentor a newbie to the project?

Comment 14

[Matthew McPherrin]

I may get around to this eventually but that looks unlikely

Comment 15

[Iaroslav Sheptykin]

Attachment: The fix and a test for handling question marks in the action attribute of a form

Hi, there!
Here is my attempt to fix this issue.

First, I could reproduce the situation described in the comment 0 with firefox from the trunk. However, Wikia Lyrics (http://lyrics.wikia.com/Main_Page) has updated its page and the search for does not include ? any more in its action. Nevertheless, I could reproduce this with a following, manually crafted piece of HTML.

data:text/html,<!DOCTYPE html><html lang="en-US"><head><meta charset="utf-8" /></head><body><div id="question-mark"><form method="get" action="http://www.google.com/search?oe=utf-8"><input type="text" name="q" value="Search google"></form></div><br /><div id="no-question-mark"><form method="get" action="http://www.google.com/search"><input type="text" name="q" value="Search google"></form></div></body></html>

I could not apply the previously offered patch (comment 6), because the code was relocated. I could, however, locate the functionality in browser.js and update it manually just like in the patch. Instead of creating new test as suggested in comment 8, I updated the test file that corresponds to the keyword search functions (browser_addKeywordSearch.js). I extended the test to cover the '?' use case. The tests pass on my local build (Ubuntu 14.04 x86).

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8490680 [details] [diff] [review]
The fix and a test for handling question marks in the action attribute of a form

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

Thanks! This looks good. I just have some style comments below:

::: browser/base/content/browser.js
@@ +6110,5 @@
>  
>    if (isURLEncoded)
>      postData = formData.join("&");
>    else
> +  {

The brace should go on the same line as the else: else {

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +6111,5 @@
>    if (isURLEncoded)
>      postData = formData.join("&");
>    else
> +  {
> +    // determining correct separator (bug 620565)

This comment doesn't add value over version-control blame so please remove it.

@@ +6112,5 @@
>      postData = formData.join("&");
>    else
> +  {
> +    // determining correct separator (bug 620565)
> +    var separator = (spec.indexOf("?") == -1 ? "?" : "&" );

You can now use |spec.contains("?") ? …| but note that you need to reverse the last two operands. The braces are unnecessary and you should use |let| instead of |var| for new code.

::: browser/base/content/test/general/browser_addKeywordSearch.js
@@ +34,5 @@
>        [ 'http://example.com/', 'q', 'http://example.com/?q=%s' ],
>        [ 'http://example.com/new-path-here/', 'q', 'http://example.com/new-path-here/?q=%s' ],
>        [ '', 'q', 'http://example.org/browser/browser/base/content/test/general/dummy_page.html?q=%s' ],
> +      // Tests for proper behaviour when called on a form whose action contains a question mark. (bug620565)
> +      [ 'http://example.com/search?oe=utf-8', 'q', 'http://example.com/search?oe=utf-8&q=%s' ],

Nit: I don't think the bug number adds value.

Comment 17

[Iaroslav Sheptykin]

Attachment: The fix and a test for handling question marks in the action attribute of a form

Thanks for the detailed review, Metthew! Coding Style was a good reading, I wasn't aware of it before. I revised the code according to your feedback and updated the patch. Tests pass on my setup.

Are there guidelines on how to refer code to the corresponding bug numbers?

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8491376 [details] [diff] [review]
The fix and a test for handling question marks in the action attribute of a form

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

(In reply to Iaroslav Sheptykin from comment #17)
> Are there guidelines on how to refer code to the corresponding bug numbers?

Since every bug fix is required to have a bug number in the commit message, you would use something like hg annotate[2] to find out the bug number for a specific line.

Could you update your commit message to fit the conventions? (you would use "r=MattN" for the review portion). You can then attach that, mark review+ by yourself (to carry forward my r+) and then add the checkin-needed keyword.

Thanks

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions
[2] http://www.selenic.com/mercurial/hg.1.html#annotate

::: browser/base/content/browser.js
@@ +6080,5 @@
>  
>    if (isURLEncoded)
>      postData = formData.join("&");
> +  else {
> +    let separator = spec.contains("?")? "&" : "?";

The space before the ? ternary operator is missing.

Comment 19

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/87b3c243b764

I ended up addressing the comments for you. Thanks for the patch. Do you need help finding another that interests you?

Comment 20

[Iaroslav Sheptykin]

(In reply to Matthew N. [:MattN] from comment #19)

> I ended up addressing the comments for you. Thanks for the patch. 
Thanks, Matthew, and sorry for the trouble. I believe we are far apart, I cannot react fast because I sleep when you work. Or you don't sleep.

> Do you need help finding another that interests you?
I would love having a suggestion about the next bug. Maybe one of super-trivial ones assigned to you, which you find boring? I could work under you mentorship then. I have some experience coding in c++, php, js. I made a couple of apps for android. But I am a total noob in Mozilla codebase. I only have contributed to this bug and the bug 407821. If you recently came across anything that fits this profile and is useful for Mozilla I would appreciate a hint.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87b3c243b764