Turn off ipv6 when there is no support at startup....

Turn off ipv6 when there is no support at startup.

IPv6 confusion (attempt to support when it is really
not supported) has been harming performance, and this
may help *some* users that don't have ipv6 support.

BUG=12754
r=wtc

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

[wtc, 2010-02-04 01:39:12]

LGTM, with the following changes.

http://codereview.chromium.org/564052/diff/10/1004
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/564052/diff/10/1004#newcode233
net/base/host_resolver_proc.cc:233: bool HostResolverProc::Ipv6Supported() {
This should be a static function (or a function in the unnamed
namespace) function in host_resolver_impl.cc.  Please move it
to that file.

http://codereview.chromium.org/564052/diff/10/1004#newcode234
net/base/host_resolver_proc.cc:234: SOCKET test_socket = socket(AF_INET6,
SOCK_STREAM, 0);
This code will only compile on Windows.

On OS_POSIX platforms, please make the following changes:
  SOCKET => int
  closesocket => close

http://codereview.chromium.org/564052/diff/10/1002
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/564052/diff/10/1002#newcode36
net/base/host_resolver_proc.h:36: static bool Ipv6Supported();
Nit: capitalize P: IPv6Supported

[wtc, 2010-02-04 01:50:39]

I forgot to note:

This is a sad day for IPv6.  All that hard work in the
past 10 years to add IPv6 support....

[jar (doing other things), 2010-02-04 02:54:30]

Several changes made per our discussion.

http://codereview.chromium.org/564052/diff/10/1004
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/564052/diff/10/1004#newcode233
net/base/host_resolver_proc.cc:233: bool HostResolverProc::Ipv6Supported() {
Per our discussion, I've left it where it is for now.


On 2010/02/04 01:39:12, wtc wrote:
> This should be a static function (or a function in the unnamed
> namespace) function in host_resolver_impl.cc.  Please move it
> to that file.

http://codereview.chromium.org/564052/diff/10/1004#newcode234
net/base/host_resolver_proc.cc:234: SOCKET test_socket = socket(AF_INET6,
SOCK_STREAM, 0);
On 2010/02/04 01:39:12, wtc wrote:
> This code will only compile on Windows.
> 
> On OS_POSIX platforms, please make the following changes:
>   SOCKET => int
>   closesocket => close

Done.

http://codereview.chromium.org/564052/diff/10/1002
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/564052/diff/10/1002#newcode36
net/base/host_resolver_proc.h:36: static bool Ipv6Supported();
On 2010/02/04 01:39:12, wtc wrote:
> Nit: capitalize P: IPv6Supported

Done.

[darin (slow to review), 2010-02-04 05:29:15]

One thing that occurs to me:  What if the user switches between an IPv6 network
configuration and an IPv4 network configuration?  In other words, perhaps we
should make this more dynamic?  One idea is to repeat this check whenever
getaddrinfo fails.

[jar (doing other things), 2010-02-04 08:24:11]

I think Will Chan was putting in a "network change" notifier, and that would
probably be the right time to change this, IF we were gonig to change it
dynamically.  This test is sooo basic, that it is hard to imagine anything
other than a network change that would alter the probe's status.

...and there are probably many inconsequential DNS name-not-found errors.

Although the current CL test is very fast, I'd expect that test to (over
time) be made more precise, and then it would take more time, and not be
desirable to do too often.

Jim

On Wed, Feb 3, 2010 at 9:29 PM, <darin@chromium.org> wrote:

> One thing that occurs to me:  What if the user switches between an IPv6
> network
> configuration and an IPv4 network configuration?  In other words, perhaps
> we
> should make this more dynamic?  One idea is to repeat this check whenever
> getaddrinfo fails.
>
>
> http://codereview.chromium.org/564052
>

[darin (slow to review), 2010-02-04 08:31:24]

On Thu, Feb 4, 2010 at 12:23 AM, Jim Roskind <jar@chromium.org> wrote:

> I think Will Chan was putting in a "network change" notifier, and that
> would probably be the right time to change this, IF we were gonig to change
> it dynamically.  This test is sooo basic, that it is hard to imagine
> anything other than a network change that would alter the probe's status.


Yeah, that's a much better idea.

-Darin




>
> ...and there are probably many inconsequential DNS name-not-found errors.
>
> Although the current CL test is very fast, I'd expect that test to (over
> time) be made more precise, and then it would take more time, and not be
> desirable to do too often.
>
> Jim
>
> On Wed, Feb 3, 2010 at 9:29 PM, <darin@chromium.org> wrote:
>
>> One thing that occurs to me:  What if the user switches between an IPv6
>> network
>> configuration and an IPv4 network configuration?  In other words, perhaps
>> we
>> should make this more dynamic?  One idea is to repeat this check whenever
>> getaddrinfo fails.
>>
>>
>> http://codereview.chromium.org/564052
>>
>
>

[willchan no longer on Chromium, 2010-02-04 15:59:52]

On Thu, Feb 4, 2010 at 12:31 AM, Darin Fisher <darin@chromium.org> wrote:
> On Thu, Feb 4, 2010 at 12:23 AM, Jim Roskind <jar@chromium.org> wrote:
>>
>> I think Will Chan was putting in a "network change" notifier, and that
>> would probably be the right time to change this, IF we were gonig to change
>> it dynamically.  This test is sooo basic, that it is hard to imagine
>> anything other than a network change that would alter the probe's status.

I've added it in already, and HostResolver is an observer already, so
you can add your IPv6 detection code in
HostResolver::OnIPAddressChange() (something like that, I forget) and
it'll get called whenever the address of the primary interface changes
(roughly speaking).

>
> Yeah, that's a much better idea.
> -Darin
>
>
>>
>> ...and there are probably many inconsequential DNS name-not-found errors.
>> Although the current CL test is very fast, I'd expect that test to (over
>> time) be made more precise, and then it would take more time, and not be
>> desirable to do too often.
>> Jim
>> On Wed, Feb 3, 2010 at 9:29 PM, <darin@chromium.org> wrote:
>>>
>>> One thing that occurs to me:  What if the user switches between an IPv6
>>> network
>>> configuration and an IPv4 network configuration?  In other words, perhaps
>>> we
>>> should make this more dynamic?  One idea is to repeat this check whenever
>>> getaddrinfo fails.
>>>
>>> http://codereview.chromium.org/564052
>>
>
>

[eroman, 2010-02-04 19:52:54]

I am concerned by this change.

(1)
This isn't a reliable way to detect IPv6 support.

I've tried this sort of detection in the past in order to be able to enable IPv6
tests on the buildbots. See:

    http://codereview.chromium.org/114061/diff/1003/6

What I found is there is a wide range of platform differences (even just on our
buildbots). On most linux systems, this test will pretty much always pass, even
though the system does not support IPv6. For example, while that would pass,
doing a getaddrinfo with an IPv6 literal would fail.

(2)
Users and bug investigators need some visibility into this disabling.

How about adding some bolded red text on chrome://net-internals/hostresolver
when IPv6 was automatically disabled (or simply disabled).

(3)
Have you considered adding histograms to measure what percentage of users we
auto-disable IPv6 for?

(4) I agree with previous comments that this detection must be done as part of
OnIPAddressChanged().

[eroman, 2010-02-04 21:40:08]

I spoke with Jim in person, and I am at ease with this now.

The plan is to turn this on as an A/B experiment, and measure the sort of impact
it has.

And regarding the IPv6 "support" detection, while it doesn't capture all the
cases, it shouldn't have false-positives at least, and therefore has very low
chance of breaking someone's internet connectivity.

A display of whether IPv6 is disabled in the about:net-intenals is easy, and I
can do this as an orthogonal change.

[darin (slow to review), 2010-02-04 21:59:47]

On 2/4/10, eroman@chromium.org <eroman@chromium.org> wrote:
> I am concerned by this change.
>
>  (1)
>  This isn't a reliable way to detect IPv6 support.
>
>  I've tried this sort of detection in the past in order to be able to enable
> IPv6
>  tests on the buildbots. See:
>
>     http://codereview.chromium.org/114061/diff/1003/6
>
>  What I found is there is a wide range of platform differences (even just on
> our
>  buildbots). On most linux systems, this test will pretty much always pass,
> even
>  though the system does not support IPv6. For example, while that would
> pass,
>  doing a getaddrinfo with an IPv6 literal would fail.

This change was inspired by what NSPR does to probe for IPv6.  Note:
NSPR only does this probing on Windows and OSX (from my reading of the
source).

It seems reasonable to match Firefox's behavior as a starting point.
I'm sure we could do better, but this has to be better than nothing,
and at least it moves us to something that is known to be reasonable
for Firefox.

-Darin


>
>  http://codereview.chromium.org/564052
>