Note: this is a follow-up for Bug 693808. Reproducible on Nightly 34.0a1 (2014-07-22), Build ID: 20140722030201, Windows platform only. Steps to reproduce: 1. Launch Nightly 34. 2. Bring the Location Bar into focus and type in 'test\' without the apostrophe. 3. Press the <enter> key. Expected result: A google search is performed for the 'test\' keyword (w/o apostrophe). Actual result: A google search is performed for the 'test/' keyword (w/o apostrophe), with the backslash converted to a forward slash.

Alex, can I interest you in this bug? :-)

(In reply to :Gijs Kruitbosch from comment #1) > Alex, can I interest you in this bug? :-) Yes, I will look into it :). I guess we should have a regression test with a comment that it's only for windows?

(In reply to Alex Bardas :alexbardas from comment #2) > I guess we should have a regression test with a comment that it's only for > windows? You can add a case in the Windows-only if block in the existing test for nsIURIFixup that you've been adding to in the lat/long and ipv6 bug. :-)

Hi Alex, can you assign a point value and if the bug should be marked as qe-verify+ or qe-verify- for verification. Thanks.

Comment on attachment 8477642 [details] [diff] [review] Limit cases when backslashes are converted to slashes for windows machines + regression test Review of attachment 8477642 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, no, I think you should just ensure that if we hit the keywordtouri path, we use the original input. Because we store that in the nsIURIFixupInfo object, we can just use that instead of the url-mangled version of the input.

I also added 2 tests, 1 for windows and 1 for os x + linux.

Comment on attachment 8478566 [details] [diff] [review] Limit cases when backslashes are converted to slashes for windows machines + regression tests Review of attachment 8478566 [details] [diff] [review]: ----------------------------------------------------------------- This is what I meant, yeah. As discussed on IRC, can you add a test for the case that includes spaces to ensure we're trimming the input that goes to the search engine?

Comment on attachment 8478611 [details] [diff] [review] Use the original input url instead of the modified one for KeywordURIFixup + regression tests r=me. Thanks for the patch!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/88a47c075155

Alex, could you request approval for aurora? :-)

Comment on attachment 8478611 [details] [diff] [review] Use the original input url instead of the modified one for KeywordURIFixup + regression tests Review of attachment 8478611 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/unit/test_nsDefaultURIFixup_info.js @@ +76,5 @@ > > function run_test() { > for (let [testInput, expectedFixedURI, alternativeURI, > expectKeywordLookup, expectProtocolChange] of testcases) { > + testInput = testInput.trim(); (In reply to :Gijs Kruitbosch from comment #8) > can you add a test for the > case that includes spaces to ensure we're trimming the input that goes to > the search engine? Er, because the test trims the input before it gets passed to urifixup, it's not actually testing this.

The initial input must be trimmed when it is compared with the result, not before calling urifixup. Blair, if this patch fixes the problem, how can it be pushed before requesting approval for aurora?

This specific test case is verified fixed on Nightly 34.0a1 (2014-08-28) using Windows 7 64-bit, but it seems that 'test \' (containing a space as well) is still converted to 'test /'. Also re-confirmed that this does *not* affect Linux and Mac. Waiting for a new patch for this bug, per my conversation with Alex.

(In reply to Andrei Vaida, QA [:avaida] from comment #16) > This specific test case is verified fixed on Nightly 34.0a1 (2014-08-28) > using Windows 7 64-bit, but it seems that 'test \' (containing a space as > well) is still converted to 'test /'. Also re-confirmed that this does *not* > affect Linux and Mac. > > Waiting for a new patch for this bug, per my conversation with Alex. So I further investigated this. In case there is a " " in the url, then that condition from KeywordURIFixup is not reached anymore, because there is a space, no dots, no colons, no quotes, no question marks and the first condition is reached. I guess we also need to use the originalInput there. I'll post a patch addressing that and waiting for feedback. In this case

Comment on attachment 8481125 [details] [diff] [review] Do not trim the input before calling urifixup Review of attachment 8481125 [details] [diff] [review]: ----------------------------------------------------------------- While we're here, let's also fix: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#396 This part is broken on release as well. Test with something like: "mozilla \ foo.txt" as input.

(also, can we add a test for the \r\n stripping, too? :-) )

(In reply to :Gijs Kruitbosch from comment #19) > Comment on attachment 8481125 [details] [diff] [review] > Do not trim the input before calling urifixup > > Review of attachment 8481125 [details] [diff] [review]: > ----------------------------------------------------------------- > > While we're here, let's also fix: > http://mxr.mozilla.org/mozilla-central/source/docshell/base/ > nsDefaultURIFixup.cpp#396 Err, wait. I misread, I think - this is probably fine? Maybe? > This part is broken on release as well. Test with something like: "mozilla \ > foo.txt" as input. This is still true, but is probably still the same path as comment #16, which is also broken on release, so the existing patch probably fixes this? I'm not sure if the comment above the earlier link is wrong, and/or if that call should be updated anyway, because right now it's passing the untrimmed string, which seems like it'd be wrong. Just not sure offhand what cases hit that path, I'd need to chuck this through a debugger. One more reason that all of this should just move to JS... :-\

I've added more tests and tested on OS X & Windows. I've also reversed the order in which we modify the input in the docshell file. First we need to strip "\r|\n" and then to trim, otherwise "file \r\n" would become "file ". KeywordToURI does another trim though. I can't think of an use case when we would need the trim before, I hope this change is appropriate.

Comment on attachment 8481438 [details] [diff] [review] Do not trim the input before calling urifixup Review of attachment 8481438 [details] [diff] [review]: ----------------------------------------------------------------- This LGTM, but you need a docshell peer's review :-) ::: docshell/test/unit/test_nsDefaultURIFixup_info.js @@ +82,5 @@ > testcases.push(["mozilla\\", "http://mozilla\\/", "http://www.mozilla/", true, true]); > } > > +function sanitize(input) { > + return input.replace(/\r|\n|\r\n/g, "").trim(); Nit: don't need the third 'or'd bit, ie just /\r|\n/g should do

(In reply to :Gijs Kruitbosch from comment #23) > Comment on attachment 8481438 [details] [diff] [review] > Do not trim the input before calling urifixup > > Review of attachment 8481438 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: docshell/test/unit/test_nsDefaultURIFixup_info.js > @@ +82,5 @@ > > > > +function sanitize(input) { > > + return input.replace(/\r|\n|\r\n/g, "").trim(); > > Nit: don't need the third 'or'd bit, ie just /\r|\n/g should do I realized that but forgot to hg qref it in the last patch. I will modify it after bz's review :-). I'm also changing the bug status to incomplete.

Incomplete tends to mean "the report didn't contain enough information for us to address it in any way". That's not the case here - this is just not fixed yet, so let's reopen it. :-)

Comment on attachment 8481438 [details] [diff] [review] Do not trim the input before calling urifixup r=me

The second patch needs checkin. The first one was already applied.

Both scenarios are now verified fixed on Nightly 35.0a1 (2014-09-02) using Windows 7 64-bit. Acceptance criteria for this patch: - Backslashes (e.g. 'test\') are not converted to forward slashes. - Backslashes preceded by a space (e.g. 'test \') are not converted to forward slashes.

Alex, shouldn't we flip the status flag for Firefox 34 back to affected, since the 2nd patch (that verifies this bug completely) is only available on Nightly 35? I'm thinking that, this way, it would be less confusing.

(In reply to Andrei Vaida, QA [:avaida] from comment #32) > Alex, shouldn't we flip the status flag for Firefox 34 back to affected, > since the 2nd patch (that verifies this bug completely) is only available on > Nightly 35? I'm thinking that, this way, it would be less confusing. Yes. Alex, can you request aurora approval for the second patch, and add a combined patch and request beta approval on that? We should ideally uplift all of this to beta to land together with the other search-in-the-urlbar fixes we did.

Comment on attachment 8481467 [details] [diff] [review] Better test coverage + do not trim the input before calling urifixup in tests Approval Request Comment [Feature/regressing bug #]: bug 693808 [User impact if declined]: affects windows users which use "\" in keyword searches [Describe test coverage new/current, TBPL]: has automated tests + was manually tested [Risks and why]: small change of the behavior of the location bar [String/UUID change made/needed]: no

Comment on attachment 8481467 [details] [diff] [review] Better test coverage + do not trim the input before calling urifixup in tests Aurora+ 33 (Beta) is marked as affected as well. Can you please request beta uplift as well so that we can avoid shipping this regression?

Verified fixed on Aurora 34.0a2 (2014-09-17) as well, using Windows 7 64-bit.

Comment on attachment 8481467 [details] [diff] [review] Better test coverage + do not trim the input before calling urifixup in tests [Triage Comment] bz thinks it is safe to take it in beta too. Approving it now to have it in beta 6.

Verified fixed on Firefox 33.0b6 (20140922173023 / d508b53c3dee) using Windows 7 64-bit.