Refine IPv6 probe to require that the client has an IPv6 address on an interf...

Refine IPv6 probe to require that the client has an IPv6 address on an interface

This currently only works on Posix, not windows.

Network changes are monitored, and the test is repeated each time interfaces
change (which is a subset of any IP addresses changing).

The test performed is still relatively low latency, and we *may* need
to eventually move to an high latency test, such as a DNS resolution,
or an actual test connection.  If we move in that direction, then we'll
need to post a task to perform the work, rather than immediately returning.

BUG=25680
BUG=12754
r=wtc,eroman

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39996

[eroman, 2010-02-23 00:02:15]

I don't know whether this is a good idea, but here are some comments on the
implementation:

It seems unnecessary to have the helper class IPV6ProbObserver in net_util. How
about simply calling IPv6Supported() directly in
HostResolverImpl::OnIpAddressChanged() ?

Secondly, I think the behavior when calling
HostResolverImpl::SetDefaultAddressFamily() should be more predictable -- if the
client has already set HostResolverImpl::SetDefaultAddressFamily() to something
other than unspecified, it shouldn't get overridden by the probe.

[jar (doing other things), 2010-02-23 19:15:45]

Changes made per eroman's suggestions.

We now reuse the address change notification if the host_resolver_impl, and we
switch off probe support when an explicit setting of the address family is
performed.

[wtc, 2010-02-24 02:37:58]

LGTM.  Please wait for eroman's approval before you check
this in.

http://codereview.chromium.org/652072/diff/2008/2013
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/652072/diff/2008/2013#newcode34
chrome/browser/io_thread.cc:34: // Determine if we should disallow IPv6 support.
Nit: disallow => disable

http://codereview.chromium.org/652072/diff/2008/2013#newcode52
chrome/browser/io_thread.cc:52: if (use_ipv6_probe) {
Nit: we don't use curly braces for a one-line "if" body.

http://codereview.chromium.org/652072/diff/2008/2009
File net/base/host_resolver.h (right):

http://codereview.chromium.org/652072/diff/2008/2009#newcode170
net/base/host_resolver.h:170: // If we have a network_change_notifier,
continuously observe it and adapt to
There is no member or argument by the name network_change_notifier in this
class.
Perhaps use NetworkChangeNotifier instead?

http://codereview.chromium.org/652072/diff/2008/2009#newcode172
net/base/host_resolver.h:172: virtual void ProbeIPv6Support() { return; }
The return statement is not necessary.  Just {} is enough.

http://codereview.chromium.org/652072/diff/2008/2011
File net/base/net_util.cc (right):

http://codereview.chromium.org/652072/diff/2008/2011#newcode1567
net/base/net_util.cc:1567: int test_socket = socket(AF_INET6, SOCK_STREAM, 0);
I would delete the socket(AF_INET6) test for OS_POSIX now.
Up to you.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1575
net/base/net_util.cc:1575: struct ifaddrs *interface_addr = NULL;
Nit: throughout this function, put * next to the type
instead of the variable.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1576
net/base/net_util.cc:1576: int ret_val = getifaddrs(&interface_addr);
Nit: In the net module, this variable is named 'rv' by
convention.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1586
net/base/net_util.cc:1586: struct sockaddr *addr = interface->ifa_addr;
You should also check interface->ifa_flags and make sure
it contains IFF_UP but not IFF_LOOPBACK.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1587
net/base/net_util.cc:1587: if (!addr)
Nit: this null check is not necessary.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1595
net/base/net_util.cc:1595: if (IN6_IS_ADDR_LOOPBACK(sin6_addr) ||
IN6_IS_ADDR_SITELOCAL(sin6_addr))
Change IN6_IS_ADDR_SITELOCAL to
IN6_IS_ADDR_LINKLOCAL.

[jar (doing other things), 2010-02-24 08:13:09]

Changes and corrections made per Wan Teh's comments.

Waiting for review by eroman per request by Wan Teh.

http://codereview.chromium.org/652072/diff/2008/2013
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/652072/diff/2008/2013#newcode34
chrome/browser/io_thread.cc:34: // Determine if we should disallow IPv6 support.
On 2010/02/24 02:37:58, wtc wrote:
> Nit: disallow => disable

Done.

http://codereview.chromium.org/652072/diff/2008/2013#newcode52
chrome/browser/io_thread.cc:52: if (use_ipv6_probe) {
On 2010/02/24 02:37:58, wtc wrote:
> Nit: we don't use curly braces for a one-line "if" body.

Done.

http://codereview.chromium.org/652072/diff/2008/2009
File net/base/host_resolver.h (right):

http://codereview.chromium.org/652072/diff/2008/2009#newcode170
net/base/host_resolver.h:170: // If we have a network_change_notifier,
continuously observe it and adapt to
I updated the comment.

On 2010/02/24 02:37:58, wtc wrote:
> There is no member or argument by the name network_change_notifier in this
> class.
> Perhaps use NetworkChangeNotifier instead?

http://codereview.chromium.org/652072/diff/2008/2009#newcode172
net/base/host_resolver.h:172: virtual void ProbeIPv6Support() { return; }
On 2010/02/24 02:37:58, wtc wrote:
> The return statement is not necessary.  Just {} is enough.

Done.

http://codereview.chromium.org/652072/diff/2008/2011
File net/base/net_util.cc (right):

http://codereview.chromium.org/652072/diff/2008/2011#newcode1567
net/base/net_util.cc:1567: int test_socket = socket(AF_INET6, SOCK_STREAM, 0);
A reasonable percentage of Linux users failed at this point, so it seems nice to
leave the test in. (although Mac users seemed to always pass this test).

On 2010/02/24 02:37:58, wtc wrote:
> I would delete the socket(AF_INET6) test for OS_POSIX now.
> Up to you.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1575
net/base/net_util.cc:1575: struct ifaddrs *interface_addr = NULL;
On 2010/02/24 02:37:58, wtc wrote:
> Nit: throughout this function, put * next to the type
> instead of the variable.

Done, including another place in the file.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1576
net/base/net_util.cc:1576: int ret_val = getifaddrs(&interface_addr);
On 2010/02/24 02:37:58, wtc wrote:
> Nit: In the net module, this variable is named 'rv' by
> convention.

Done.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1586
net/base/net_util.cc:1586: struct sockaddr *addr = interface->ifa_addr;
On 2010/02/24 02:37:58, wtc wrote:
> You should also check interface->ifa_flags and make sure
> it contains IFF_UP but not IFF_LOOPBACK.

Done.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1587
net/base/net_util.cc:1587: if (!addr)
On 2010/02/24 02:37:58, wtc wrote:
> Nit: this null check is not necessary.

Based on my reading of:
http://www.kernel.org/doc/man-pages/online/pages/man3/getifaddrs.3.html

"ifa_addr and other fields can actually be NULL if the interface has no address"

So I threw in the null check to be safe.

http://codereview.chromium.org/652072/diff/2008/2011#newcode1595
net/base/net_util.cc:1595: if (IN6_IS_ADDR_LOOPBACK(sin6_addr) ||
IN6_IS_ADDR_SITELOCAL(sin6_addr))
On 2010/02/24 02:37:58, wtc wrote:
> Change IN6_IS_ADDR_SITELOCAL to
> IN6_IS_ADDR_LINKLOCAL.

Done.

[eroman, 2010-02-24 20:40:22]

lgtm with the following comments:

http://codereview.chromium.org/652072/diff/33/34
File net/base/host_resolver.h (right):

http://codereview.chromium.org/652072/diff/33/34#newcode172
net/base/host_resolver.h:172: virtual void ProbeIPv6Support() { }
I don't think this should be added to the base interface -- just make it a
non-virtual method of HostResolverImpl.

(The only usage of this method is from io_thread.cc where we know it is a
HostResolverImpl).

http://codereview.chromium.org/652072/diff/33/35
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/652072/diff/33/35#newcode989
net/base/host_resolver_impl.cc:989: default_address_family_ = IPv6Supported() ?
ADDRESS_FAMILY_UNSPECIFIED
I suggest a LOG(INFO) indicating when the change is made.

http://codereview.chromium.org/652072/diff/33/36
File net/base/net_util.cc (right):

http://codereview.chromium.org/652072/diff/33/36#newcode1578
net/base/net_util.cc:1578: if (rv) {
nit: i suggest writing this as rv != 0

[jar (doing other things), 2010-02-25 00:00:01]

Changes made per eroman's suggestions.

http://codereview.chromium.org/652072/diff/33/34
File net/base/host_resolver.h (right):

http://codereview.chromium.org/652072/diff/33/34#newcode172
net/base/host_resolver.h:172: virtual void ProbeIPv6Support() { }
Per discussion, I pushed it down into the impl, and then used GetAs...Impl to
support this.

On 2010/02/24 20:40:22, eroman wrote:
> I don't think this should be added to the base interface -- just make it a
> non-virtual method of HostResolverImpl.
> 
> (The only usage of this method is from io_thread.cc where we know it is a
> HostResolverImpl).

http://codereview.chromium.org/652072/diff/33/35
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/652072/diff/33/35#newcode989
net/base/host_resolver_impl.cc:989: default_address_family_ = IPv6Supported() ?
ADDRESS_FAMILY_UNSPECIFIED
On 2010/02/24 20:40:22, eroman wrote:
> I suggest a LOG(INFO) indicating when the change is made.

Done.

http://codereview.chromium.org/652072/diff/33/36
File net/base/net_util.cc (right):

http://codereview.chromium.org/652072/diff/33/36#newcode1578
net/base/net_util.cc:1578: if (rv) {
On 2010/02/24 20:40:22, eroman wrote:
> nit: i suggest writing this as rv != 0

Done.

[eroman, 2010-02-25 01:09:34]

> Per discussion, I pushed it down into the impl, and then used
> GetAs...Impl to support this.

Thanks!

LGTM.

http://codereview.chromium.org/652072/diff/4002/4006
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/652072/diff/4002/4006#newcode96
net/base/host_resolver_impl.h:96: virtual void ProbeIPv6Support();
This no longer needs to be virtual.

[jar (doing other things), 2010-02-25 01:57:15]

Change made per eroman's comment.

http://codereview.chromium.org/652072/diff/4002/4006
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/652072/diff/4002/4006#newcode96
net/base/host_resolver_impl.h:96: virtual void ProbeIPv6Support();
On 2010/02/25 01:09:34, eroman wrote:
> This no longer needs to be virtual.

Done.