Closed
Bug 1067168
Opened 10 years ago
Closed 10 years ago
Entering "1.2.3.4/" in URL bar shouldn't cause a search.
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox35 | --- | verified |
People
(Reporter: Dolske, Assigned: alexbardas)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
7.21 KB,
patch
|
alexbardas
:
review+
|
Details | Diff | Splinter Review |
(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•10 years ago
|
||
(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).
Flags: needinfo?(dolske)
Comment 2•10 years ago
|
||
I can reproduce on an updated nightly, but not on 9/9. I'm fairly sure this is just bug 494092.
Comment 3•10 years ago
|
||
(confirmed with mozregression: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db5b64f6d49&tochange=59d4326311e0 )
Assignee | ||
Comment 4•10 years ago
|
||
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?
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment 5•10 years ago
|
||
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8489466 -
Flags: review?(gijskruitbosch+bugs)
Comment 7•10 years ago
|
||
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)
Attachment #8489466 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8489466 -
Attachment is obsolete: true
Attachment #8489532 -
Flags: review?(bugs)
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
Attachment #8489532 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•10 years ago
|
||
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?
Attachment #8489561 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 11•10 years ago
|
||
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. :-)
Attachment #8489561 -
Flags: feedback?(gijskruitbosch+bugs)
Attachment #8489561 -
Flags: feedback?(dolske)
Attachment #8489561 -
Flags: feedback?(bugs)
(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
Attachment #8489561 -
Flags: feedback?(dolske)
Attachment #8489561 -
Flags: feedback?(bugs)
Assignee | ||
Comment 14•10 years ago
|
||
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.
Attachment #8489532 -
Attachment is obsolete: true
Attachment #8489561 -
Attachment is obsolete: true
Attachment #8490303 -
Flags: review?(bugs)
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"
Attachment #8490303 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Iteration: --- → 35.2
Points: --- → 5
Flags: qe-verify?
Flags: firefox-backlog+
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8490303 -
Attachment is obsolete: true
Attachment #8490324 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
Assignee | ||
Comment 20•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Updated•10 years ago
|
QA Contact: andrei.vaida
Comment 22•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 23•10 years ago
|
||
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
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Flags: needinfo?(abardas)
Comment 26•10 years ago
|
||
(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.
Assignee | ||
Comment 27•10 years ago
|
||
(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•10 years ago
|
||
(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.
Status: RESOLVED → VERIFIED
status-firefox35:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•