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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Chris Peterson [:cpeterson]]

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."

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1299391 - fix decimal IP detection for 'did you mean this host instead' when we fix up URIs for search,

Review commit: https://reviewboard.mozilla.org/r/75616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75616/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[:Gijs (he/him)]

(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 4

[:Gijs (he/him)]

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,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75616/diff/1-2/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 6

[:Gijs (he/him)]

(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.

Comment 7

[Junior [inactive]]

> 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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73c470a53900