724841 - Searching from the address bar lower cases search phrase sent to search engines

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) search from the address bar with text that includes capital letters.

Example "TTDeleteEmbeddedFont"

result: the phrase will be sent to your default search engine as lower case, e.g. "ttdeleteembeddedfont" - 

"http://www.google.com/search?q=ttdeleteembeddedfont&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:official&client=firefox-a"

I wasn't able to find another bug on this, although I'm surprised it hasn't been filed.

Note, both IE and Chrome send the original, properly cased phrase.

Comment 1

[Eric Wong (:ekw)]

Attachment: Retain case when sending location bar text to search engine service.

Bug only occurs with one word searches from location bar due to attempt to load as URL first:

"TTDeleteEmbeddedFont" --> Try to load http://ttdeleteembeddedfont --> Search for "ttdeleteembeddedfont"

Two or more word searches from location bar are fine (there's no attempt to load as URL first):

"TTDeleteEmbeddedFont Other Stuff" --> search for "TTDeleteEmbeddedFont Other Stuff"

This patch saves the original text when nsStandardURL object is created (via nsIURI interface) and uses it later when load fails and text is sent to search engine service.

Comment 2

[Eric Wong (:ekw)]

I haven't run any tests yet.  This being my first attempt at fixing a mozilla bug, I'm trying to figure out which tests I should run.  Any help here appreciated.

Comment 3

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

I think it's probably a little too invasive to be making an API addition to nsIURI just to cover this use case (nsIURI is a "core" interface, and this doesn't seem like something relevant to most implementations of it). Can nsDocShell keep a reference to the source string through some other mechanism?

Comment 4

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

Oh, and great work looking into the issue, Eric - impressive effort for your first patch :)

Comment 5

[Eric Wong (:ekw)]

Thanks, I will look into keeping things within nsDocShell.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 602816 [details] [diff] [review]
Retain case when sending location bar text to search engine service.

Yeah, I don't think we want to put this on nsIURI....

Comment 7

[Eric Wong (:ekw)]

Attachment: Keep changes within nsDocShell

Is this too simple?  Am I missing anything?

Comment 8

[Eric Wong (:ekw)]

Attachment: mochitest results for docshell/test

543 INFO Passed: 430
544 INFO Failed: 0
545 INFO Todo:   0

Comment 9

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

Comment on attachment 604230 [details] [diff] [review]
Keep changes within nsDocShell

Looks reasonable to me! bz should do the actual review (I haven't verified that all of the possible code paths where mOriginalUriString is set correspond to paths where it is used, and vice-versa).

Comment 10

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 604230
	Branch: mozilla-central => try
Insufficient permissions to push to try.

Comment 11

[Mozilla RelEng Bot]

Try run for d2f40a675b89 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d2f40a675b89
Results (out of 93 total builds):
    success: 84
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d2f40a675b89

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604230 [details] [diff] [review]
Keep changes within nsDocShell

No, this is definitely wrong.  There are loading entrypoints that start with an nsIURI, not a string, but can end up in EndPageLoad.  For example <a href="something"> loads.  With this patch if you do a load from the url bar and then click on an anchor and the anchor fails to resolve we'll do a search for the thing that originally came from the url bar.  Worth adding a testcase.

What should probably happen is that starting a load (InternalLoad) always clears mOriginalUriString.  And you'd want to set mOriginalUriString in the string version of LoadURI only after calling the URI version of LoadURI.

Comment 13

[Eric Wong (:ekw)]

Attachment: Made changes based on Comment 12

Made changes based on Boris Zbarsky's comment 12:
"What should probably happen is that starting a load (InternalLoad) always clears mOriginalUriString.  And you'd want to set mOriginalUriString in the string version of LoadURI only after calling the URI version of LoadURI."

This latest patch also fixes the bug.  If feedback is positive, I will look into adding a testcase for this (and will need pointers on where/how to add a testcase as this is my first bug).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607008 [details]
Made changes based on Comment 12

> +    mOriginalUriString.Assign(NULL, 0);

  mOriginalUriString.Truncate();

Looks fine to me with that changed.

Comment 15

[Eric Wong (:ekw)]

Attachment: Made changes based on Comment 14

Comment 16

[Eric Wong (:ekw)]

Attachment: mochitest-plain test results for docshell

544 INFO Passed: 430
545 INFO Failed: 0
546 INFO Todo:   0

Comment 17

[Jon Buckley]

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=c6ef72f5f196

Comment 18

[Eric Wong (:ekw)]

Attachment: Same as last patch, but redone to apply cleanly to current mozilla-central tip

Comment 19

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec73828b931

Comment 20

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/7ec73828b931

Possible to write a test for this?

Comment 21

[Eric Wong (:ekw)]

I can write a testcase, but will need guidance on how to do so.  Will it be a mochitest?  How can I simulate entering a search in the url bar?  I tried location.href = "TTDeleteEmbeddedFont", but that gives "/tests/docshell/test/TTDeleteEmbeddedFont was not found"

Comment 22

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

You might be able to just modify browser/base/content/test/browser_keywordSearch.js to test this.

Comment 23

[Eric Wong (:ekw)]

Attachment: Browser chrome testcase for this bug

Used browser/base/content/test/browser_keywordSearch.js as template for this testcase.  TC1 fails without the patch, passes with the patch.  TC2-TC4 passes with or without patch, but added for completeness.  

This info line appears in the test output that's not in the output for browser_keywordSearch.js:
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug724841.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must to be declared in the document or in the transfer protocol." {file: "http://example.com/?q=TestSearch" line: 0}]

Not sure why.  Maybe because this test goes all the way through NETWORK/WINDOW STOP state instead of stopping at DOCUMENT START state like browser_keywordSearch.js does.

Comment 24

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

You can mostly ignore that message, it just means that your test triggered a load of a page that triggers that warning (which is a little overly aggressive, but I digress).

Was it necessary to observe STOP instead of START for the test to be effective? I was thinking you could have just added an additional test to browser_keywordSearch.js itself...

Comment 25

[Eric Wong (:ekw)]

I did initially try to just add an additional test to browser_keywordSearch.js, but when "TestSearch" is entered in the URL bar, "http://testsearch" is returned first before returning the expected "http://example.com/?q=TestSearch" (See Comment 1).  

If START is observed, the test will test against "http://testsearch" and fail.  I had to observe STOP to skip the part where it first tries to use "TestSearch" as a host before testing against the desired part where it uses "TestSearch" as a search term.

Comment 26

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

Comment on attachment 621267 [details]
Browser chrome testcase for this bug

Sorry for the horrible delay here Eric - this is awesome work. A couple of comments about the code:

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

>+function test() {

>+  let windowObserver = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic == "domwindowopened") {
>+        ok(false, "Alert window opened");
>+        let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>+        win.addEventListener("load", function() {
>+          win.removeEventListener("load", arguments.callee, false);
>+          win.close();
>+        }, false);
>+        executeSoon(finish);
>+      }
>+    }
>+  };
>+
>+  Services.ww.registerNotification(windowObserver);

You should be able to remove this stuff (and the corresponding unregisterNotification) from the test, since the failure cases for this variant of the test aren't alert dialogs.

>+      // Only care when network and window stop
>+      let netWinStop = Ci.nsIWebProgressListener.STATE_IS_NETWORK |
>+                     Ci.nsIWebProgressListener.STATE_IS_WINDOW |
>+                     Ci.nsIWebProgressListener.STATE_STOP;

Did NETWORK | STOP not work? I wonder now whether we can just change browser_keywordSearch to observe STOP instead of START, so that we can add these tests to it. IIRC I chose to use START for browser_keywordSearch instead of STOP only because it occurs first and was sufficient. If we can change that test to use STOP and maintain its test coverage, it would be simpler to just do that.

I realize you may no longer be available to drive this through - if so I can try to do that at some point. Apologies again for the delayed response.

Comment 27

[Eric Wong (:ekw)]

Attachment: Test for bugfix.

I couldn't change browser_keywordSearch to use STOP instead of START.  The existing tests break if I do that.  What I did instead is to make the flags that trigger the test dynamic, so each test can use different flags to trigger the test.  Problem with this though is the ordering of the tests matter.  I had to put all the tests using STOP first, otherwise if they're at the end, the first test that uses STOP will actually be triggered by the previous test's STOP.  I couldn't figure out how to tell a test to wait until the previous test's progress cycle has completed so, for now, I just put all the tests that uses STOP first.

I also changed the way the progress flags are evaluated to determine if the test should start or not.  Let me know if I'm wrong, but I think it was incorrect the way it was before.  Before, the test would start if any of the specified flags were present.  Now, the test will start only if all of the specified flags are present.  With the original tests, it just happened to work anyway.  With these new tests, I had to make sure both NETWORK and STOP are set (the previous code would start the test if either NETWORK or STOP were set).