1067168 - Entering "1.2.3.4/" in URL bar shouldn't cause a search.

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Justin Dolske [:Dolske]]

(Bug report from email)

Trying to navigate directly to an IP address via "1.2.3.4" works, but not when there's a trailing slash (eg "1.2.3.4/") -- an undesired search is performed instead.

Further, I'd suggest that a trailing slash may generically be useful as an indicator that the entered string is an URL-like destination, and not a search. This is especially true for single-word entries (which bug 693808 made work as searches, with UI to whitelist for future uses). EG, entering "intranet/" or "secretlair/" should always attempt to use "intranet" or "secretlair" as hostnames, and not attempt searches. This could be particularly useful in environments where there are a lot or constantly changing list of such hosts. Alternatively, a bigger hammer (pref?) to disable bug 693808 could be useful in such cases.

Comment 1

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #0)
> (Bug report from email)

Against what version of Firefox? I can't reproduce in 33, 34, or 35 - it's always connecting to the IP address first. I'm not sure this is a regression of bug 693808, or perhaps it's OS-specific? (we have os-specific fixup logic because of path separators)

As for the trailing slash... that needs more thought though, because e.g. "5/4" should be a search, but "localhost/foo" shouldn't. I'm not sure at what point we should assume diminishing returns and/or if we should prioritize fixing that before just doing the whole thing in JS (which will make fixing it so much easier).

Comment 2

[:Gijs (he/him)]

I can reproduce on an updated nightly, but not on 9/9. I'm fairly sure this is just bug 494092.

Comment 3

[:Gijs (he/him)]

(confirmed with mozregression: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db5b64f6d49&tochange=59d4326311e0 )

Comment 4

[Alex Bardas :alexbardas]

It's from this line: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#1052

For 1.2.3.4/ : host == asciihost == 1.2.3.4 and a search is performed. It goes to that line because it passes the ipv4 check (because of the trailing slash)

I've added lots of tests in bug 494092, but not for this particular case.

Gijs, what approach would you suggest for this bug? Improving the condition for ipv4 detection?

Comment 5

[:Gijs (he/him)]

(In reply to Alex Bardas :alexbardas from comment #4)
> It's from this line:
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> nsDefaultURIFixup.cpp#1052
> 
> For 1.2.3.4/ : host == asciihost == 1.2.3.4 and a search is performed. It
> goes to that line because it passes the ipv4 check (because of the trailing
> slash)
> 
> I've added lots of tests in bug 494092, but not for this particular case.
> 
> Gijs, what approach would you suggest for this bug? Improving the condition
> for ipv4 detection?

Probably. The interesting thing is that 1.2.3.4/foo works already... So I *think* we can get away with just dealing with the trailing slash here?

Comment 6

[Alex Bardas :alexbardas]

Attachment: bug1067168_make_sure_valid_ipv4_with_trailing_slash_does_not_perform_search.diff

Comment 7

[:Gijs (he/him)]

Comment on attachment 8489466 [details] [diff] [review]
bug1067168_make_sure_valid_ipv4_with_trailing_slash_does_not_perform_search.diff

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

See below for comments - but I'm not a docshell peer, so it's probably good to ask bz or smaug for the next iteration of the patch. :-)

::: docshell/base/nsDefaultURIFixup.cpp
@@ +1005,5 @@
>              lastLSBracketLoc = pos;
>          } else if (*iter == ']') {
>              foundRSBrackets++;
> +        } else if (*iter == '/') {
> +            lastSlashLoc = pos;

I wonder if it makes more sense to just check this with a CharAt or EndsWith or whatever in the conditional, rather than setting this every time. Anyway, I will leave that up to another reviewer.

::: docshell/test/unit/test_nsDefaultURIFixup_info.js
@@ +87,5 @@
>      input: "127.0.0.1",
>      fixedURI: "http://127.0.0.1/",
>      protocolChange: true,
>    }, {
> +    input: "1.2.3.4/",

Let's add similar tests for the ipv6 case (which already works before this patch, I think)

Comment 8

[Alex Bardas :alexbardas]

Attachment: Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8489532 [details] [diff] [review]
Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

Please add tests also for cases when
there are < 3 dots and slash.
Something like 
1.2.3
1.2.3/
1.2.3/foo
and also 
http://1.2.3
http://1.2.3/
http://1.2.3/foo

Comment 10

[Alex Bardas :alexbardas]

Attachment: Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

I've added the tests smaug recommended.

Gijs please take look and tell me what you think about 1.2.3/ and 1.2.3/foo . I intentionally left a comment there. 

With the following patch, tests are green, but is this the desired behavior?

Comment 11

[:Gijs (he/him)]

Comment on attachment 8489561 [details] [diff] [review]
Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

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

Personally, I would have expected no keyword lookup for either the

1.2.3/
1.2.3/foo

cases, and AIUI right now one gets a keyword lookup and one doesn't?

It's kind of tricky because there are essentially 3 possibilities here:
1) 1.2.3 is an alias and/or works with the DNS' lookup domain - in this case the keyword lookup is clearly wrong.
1b) someone is using a 3 group IP address (that is, relies on normalization to 1.2.0.3) - basically the same as the previous case.
2) "1.2.3/" (or, say, "1.2.3/4") is a typo for a maths expression - in this case, the keyword lookup would be most appropriate.
3) "1.2.3/" (or, say, "1.2.34/") is a typo for an IP address- in this case, not doing a keyword lookup is probably best.

Unfortunately, (2) seems like the most farfetched in terms of how easily you typo like this, but also, I guess it's a case most likely to happen to most of our users. That is, I imagine that the frequency with which users type full IP addresses into the URL bar is somewhere <1% of our users and/or their uses of the browser, so cases 1 and 3 are much less likely than case 2...

Chrome picks case (1b) here and normalizes to 1.2.0.3 and tries to load that. I think that searching might be more appropriate? Not 100% sure, though, so I'm going to chicken out of making a decision and needinfo Olli and Justin to give their opinions. :-)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gijs Kruitbosch from comment #11)
> 1.2.3/
> 1.2.3/foo
> 
> cases, and AIUI right now one gets a keyword lookup and one doesn't?

This is indeed odd inconsistency.


1b or 3 look the best options to me.
2 is really odd

Comment 14

[Alex Bardas :alexbardas]

Attachment: Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

I've fixed 1.2.3/, 1.2.3/foo and 1.2.3.4:8000 (no keyword lookup). I've added tests to cover all these cases.

I'm open to new test suggestions if I've missed something.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490303 [details] [diff] [review]
Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

This is getting unreadable :(
Waiting for this all to move outside docshell/*

Please test also "1.2.3:8000/foo"

Comment 16

[Alex Bardas :alexbardas]

Attachment: Ipv4 with trailing slash input in locationbar should open the address, not perform a search.

Comment 20

[Alex Bardas :alexbardas]

Try looks good:

https://tbpl.mozilla.org/?tree=Try&rev=c0a1b87e9deb

I'd like the QA team to also test the duplicates, not only the trailing slash.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/797ec8950536

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/797ec8950536

Comment 23

[Andrei Vaida [:avaida]]

Testing performed on Nightly 35.0a1 (2014-09-23) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Potential issues:
1. [Windows only] XML parsing error encountered while accessing http://1.2.3.4/foo
 * screenshot: http://i.imgur.com/EmvtvKt.png
 * note: I was only able to reproduce this issue once, on a clean profile

This issue might be something isolated to my testing environment. Alex, what do you think?

(In reply to Alex Bardas :alexbardas from comment #20)
> I'd like the QA team to also test the duplicates, not only the trailing
> slash.
I'm not sure what you by 'duplicates', but here's a list [1] of scenarios I verified across platforms. If there's anything else I should be looking at, please let me know.


[1] https://mozqa.etherpad.mozilla.org/Bug1067168

Comment 24

[Andrei Vaida [:avaida]]

Just making sure everyone's aware of Comment 23.

Comment 25

[Alex Bardas :alexbardas]

(In reply to Andrei Vaida, QA [:avaida] from comment #24)
> Just making sure everyone's aware of Comment 23.

Hi Andrei. I've tried to access that mozqa etherpad link but I need an account (my mozilla account doesn't work there). I'm still waiting to get one, but maybe you can paste them only on a etherpad.mozilla.org link.

Regarding the screenshot, I don't see how it is related to this bug. I usually get that error when the string is not defined as a l10n resource.

Comment 26

[Andrei Vaida [:avaida]]

(In reply to Alex Bardas :alexbardas from comment #25)
> Hi Andrei. I've tried to access that mozqa etherpad link but I need an
> account (my mozilla account doesn't work there). I'm still waiting to get
> one, but maybe you can paste them only on a etherpad.mozilla.org link.
Yeah, sorry about that, here's a new link: https://etherpad.mozilla.org/Bug1067168.

Comment 27

[Alex Bardas :alexbardas]

(In reply to Andrei Vaida, QA [:avaida] from comment #26)
> Yeah, sorry about that, here's a new link:
> https://etherpad.mozilla.org/Bug1067168.

Thank you for the list. Those results are expected by the current implementation. 

Can you still reproduce the error from the screenshot? If not, then this bug was verified. If yes, I'd still say it's not related to this bug but I would investigate more and maybe fill a new bug to fix it.

Comment 28

[Andrei Vaida [:avaida]]

(In reply to Alex Bardas :alexbardas from comment #27)
> Thank you for the list. Those results are expected by the current
> implementation. 
> 
> Can you still reproduce the error from the screenshot? If not, then this bug
> was verified. If yes, I'd still say it's not related to this bug but I would
> investigate more and maybe fill a new bug to fix it.

I was only able to reproduce that error once, so I'm thinking we can safely mark this as verified. I'll keep an eye on it though, if I manage to isolate STR for it, I'll file a separate bug for it. Thanks.