Closed Bug 1299391 Opened 5 years ago Closed 5 years ago

Location bar search for a number suggests navigating to an IP address (e.g. 123 and "0.0.0.123")

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: cpeterson, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
1. Search for a number in the location bar, such as 123

RESULT:
Firefox will show an info bar asking "Did you mean to go to 0.0.0.123?"

Gijs, my understanding is that searching for numbers should no longer ask about visiting IP addresses (bug 1053245). Is that correct?

This is a recent regression, which I bisected to bug 1288049 ("Canonicalize IPv4 for nsStandardURL"). Even if we did want to suggest visiting IP addresses, we shouldn't suggest invalid IP addresses such as those that begin with "0."
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: General → Location Bar
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P1
Comment on attachment 8786700 [details]
Bug 1299391 - fix decimal IP detection for 'did you mean this host instead' when we fix up URIs for search,

https://reviewboard.mozilla.org/r/75616/#review73636

We need a test for this so it doesn't regress again.
Attachment #8786700 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8786700 [details]
> Bug 1299391 - fix decimal IP detection for 'did you mean this host instead'
> when we fix up URIs for search,
> 
> https://reviewboard.mozilla.org/r/75616/#review73636
> 
> We need a test for this so it doesn't regress again.

There is already a test, and bug 1288049 changed the testcase so that the test kept passing after their changes. :'-(

I'll update the test.
Comment on attachment 8786700 [details]
Bug 1299391 - fix decimal IP detection for 'did you mean this host instead' when we fix up URIs for search,

https://reviewboard.mozilla.org/r/75616/#review74048

::: browser/base/content/test/urlbar/browser_urlbarSearchSingleWordNotification.js:72
(Diff revision 2)
>  });
>  
> -add_task(function* test_navigate_valid_numbers() {
> +add_task(function* test_navigate_decimal_ip() {
>    let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
>    yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> -  yield* runURLBarSearchTest("1234", true, true);
> +  yield* runURLBarSearchTest("1234", true, false);

I think it is easy for a reviewer to not look up what these boolean arguments mean, and thus reviewers might blindly think that these changes are OK to rubberstamp.

While we're here, can you change this to an options Object argument so these can be named parameters?
Attachment #8786700 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8786700 [details]
> Bug 1299391 - fix decimal IP detection for 'did you mean this host instead'
> when we fix up URIs for search,
> 
> https://reviewboard.mozilla.org/r/75616/#review74048
> 
> :::
> browser/base/content/test/urlbar/browser_urlbarSearchSingleWordNotification.
> js:72
> (Diff revision 2)
> >  });
> >  
> > -add_task(function* test_navigate_valid_numbers() {
> > +add_task(function* test_navigate_decimal_ip() {
> >    let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
> >    yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> > -  yield* runURLBarSearchTest("1234", true, true);
> > +  yield* runURLBarSearchTest("1234", true, false);
> 
> I think it is easy for a reviewer to not look up what these boolean
> arguments mean, and thus reviewers might blindly think that these changes
> are OK to rubberstamp.

FWIW, I don't think that the comments in bug 1288049 bear that out. They explicitly decided to change behaviour, with no real justification:

(In reply to Junior [:junior] from bug 1288049 comment #20)
> This patch breaks bug 1053245.
> But I think a number which can be transformed to IPv4 deserves a
> notification.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> While we're here, can you change this to an options Object argument so these
> can be named parameters?

I don't know that it'll help, but sure.
> FWIW, I don't think that the comments in bug 1288049 bear that out. They
> explicitly decided to change behaviour, with no real justification:
> 
> (In reply to Junior [:junior] from bug 1288049 comment #20)
> > This patch breaks bug 1053245.
> > But I think a number which can be transformed to IPv4 deserves a
> > notification.
> 

The notification change is not on purpose. We try to canonicalize IPv4 to standard.
And we did notice this issue during the test.

It seems not a super bad idea for this explicit notifying behavior to me, so I didn't
change the behavior. Feel free to remove the IP hint suggestion and I'm sorry to not
let you know this change in advance.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c470a53900
fix decimal IP detection for 'did you mean this host instead' when we fix up URIs for search, r=jaws
https://hg.mozilla.org/mozilla-central/rev/73c470a53900
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1302004
No longer depends on: 1302004
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.