506511 - Web searches in sidebar pass symbols rather than spaces to the search engines

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[_\/_ Luctur _\/_]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.1pre) Gecko/20090717 SeaMonkey/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.1pre) Gecko/20090717 SeaMonkey/2.0b1

SM passes spaces to search engines as symbols % (percentage) and +25 rather than actual spaces.

 

Reproducible: Always

Steps to Reproduce:
1. open SM sidebar;
2. search for a couple of words separated by spaces (i.e. "Name Surname"); 
3. do your web search;
4. click the "Next" button in the sidebar search tab and display more results.
Actual Results:  
After the search, SM displays wrong search engine results.

Expected Results:  
SM correctly displays search engines results either in sidebar and in main page tab.

I have Google as default search engine and in the sidebar input field I've written my name "Gianluca Turconi".

After the search, the input field displays "Gianluca%Turconi", but so far there's no problem because Google finds correctly the first page of results. When I press the "next" button in SM sidebar the input text changes to "gianluca+25turconi" and the search returns wrong results. 

Sometimes (3 out of 10 attempts), the wrong search string was passed to the wrong search engine too (Ask instead of Google).

The web search results are correctly displayed both in sidebar and the Google main html page if I click "next" on Google web page only.

More details from Jens Hatlak:

The first field value change is already wrong, it shouldn't replace the space by a percent sign. I tracked that part down to search-panel.js function rememberSearchText(). In line 128 'target' is already wrong, containing %25 instead of a space. %25 gets decoded to % by decodeURIComponent() in line 142. There's even a plus-to-space conversion in line 130 but obviously it doesn't apply here.

rememberSearchText() is called from RDF_observer. Maybe this regression is a side-effect of some RDF-related changes?

BTW: The second part of the steps above can also be produced by simply repeating the search instead of pressing Next.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Further analysis shows that RDF_observer's onChange() (in combination with prop == textArc which is LastText) is only triggered by RememberLastSearchText() (see nsInternetSearchService.cpp). That function is only called on three occasions:

1. by search-panel.js's OpenSearch(), line 841, with the correct value, but with sidebarInitiatedSearch == true so rememberSearchText() does nothing,

2. by nsInternetSearchService.cpp's FindInternetSearchResults(), line 2719, with nsnull, which should clear the search text but is not called in this case,

3. by nsInternetSearchService.cpp's FindInternetSearchResults(), line 2701, obviously with the wrong value since the two other cases above have been eliminated.

Maybe a regression from bug 374862? Don't expect me to understand any of those C++ changes... Unfortunately I have to give up here. Hopefully someone else can take over.

Comment 2

[Robert Kaiser]

I would very much hope for us to switch to the toolkit search service for 2.1, but I guess someone would need to rewrite that sidebar in that case...

Comment 3

[Philip Chee]

Attachment: Patch v1.0 wallpaper over the problem.

The real problem is somewhere in the CPP implementation of nsIInternetSearchService::FindInternetSearchResults(). There is a lot of escape/unescape/escape/unescape going on there and I guess something got lost in between.

This wallpapers over the problem in the front end until the search service is rewritten to use the toolkit openSearch implementation.

Comment 4

[Philip Chee]

Comment on attachment 435091 [details] [diff] [review]
Patch v1.0 wallpaper over the problem.

Oops. Spotted an obvious problem.

Comment 5

[Philip Chee]

Attachment: Patch v1.1 only escape

Comment 6

[Philip Chee]

Attachment: Patch v1.1 only escape '+' in the path

Only convert + to space in the path.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 435179 [details] [diff] [review]
Patch v1.1 only escape '+' in the path

Works fine. :-) Tried:
1. entered search string with spaces, pressed Search, Next, Previous.
2. entered search string with spaces and +s, pressed Search, Next, Previous.
In both cases the text field (Sidebar) and Google's text field (browsing area) matched what I had entered before and after each button press.

Comment 8

[[e-mail]]

Attachment: %2B or not %2B?

OK, so the bug was with the external string API conversion.

Historically the code converted %25 to %2B25 and + to %25 before converting them from the search engine's character set to UTF8, and then converted them back.

Unfortunately the external string API conversion suffered from too much copy and paste and the wrong string was converted back.

Additionally (and this applied to the original code) the strings %25 and %2B were confused. %2B is the encoding of +, and %252B is the encoding of %2B.

So of course I couldn't choose any other patch description ;-)

Comment 9

[[e-mail]]

Attachment: Revised patch

Although %2B25 didn't work with the old code %252B doesn't work either (searching for +25 %25 25 +2B %2B 2B fails differently depending on the code).

Comment 10

[Ian Neal]

Comment on attachment 436203 [details] [diff] [review]
Revised patch

r=me but, unless there is a reason that currently escapes me (pun intended), can you be consistent with the single and double quotes used.

Comment 11

[[e-mail]]

(In reply to comment #10)
> (From update of attachment 436203 [details] [diff] [review])
> r=me but, unless there is a reason that currently escapes me (pun intended),
> can you be consistent with the single and double quotes used.
This is C++, so you use single quotes for a single character and double quotes for an arbitrary-length string. Now I guess I could have used Find to find a single-character string, but it's more efficient to use FindChar. Replace is overloaded and will accept both, but I stuck to matching the strings I found.

Comment 12

[[e-mail]]

Pushed changeset eada20ef2a0a to comm-central.

Comment 13

[[e-mail]]

Pushed changeset 2bed1d09c66a to releases/comm-1.9.1