Add a mechanism to disable IPv6....

Add a mechanism to disable IPv6.

(1) Adds the ability to specify the address family on a per-request basis.

(2) Exposes a --disable-ipv6 flag to chrome that changes the default address family from AF_UNSPEC to AF_INET (same sort of thing Firefox does).

(3) Changes the backing datastructure for HostCache:EntryMap and HostResolverImpl::JobMap from a "hash_map" to a "std::map". This was for consistency with other code (when I went to add a custom hash trait, I couldn't find any existing code which was using hashmap for custom keys).

(4) Updates about:net-internals to display an address family for the hostcache dump (since it is now a part of the key).

This change is in anticipation of turning off IPv6 host resolving in the PAC utility functions (see bug 24641). But it is also a feature addition.

BUG=24641
TEST=HostCacheTest.AddressFamilyIsPartOfKey

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

[eroman, 2009-10-19 23:29:43]

[cc-ing Wan-Teh]

[wtc, 2009-10-20 02:39:01]

Eric:

This CL seems larger than necessary.  I believe we only need to add an
option to HostResolver to specify the address family.

http://codereview.chromium.org/302010/diff/1/19
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/302010/diff/1/19#newcode241
Line 241: const char kDisableIPv6[]                   = "disable-ipv6";
Are you sure Firefox has this command-line option?

I know Firefox has a preference in which you can list hosts that are IPv4-only.

[eroman, 2009-10-20 03:23:52]

> This CL seems larger than necessary.

What parts of this change are unnecessary?

While at a high level we only need to change the HostResolver to take an address
family, to do that means you need to also change the caching to key by
(hostname, address_family) otherwise you have a correctness problem. And there
is also alot of files affected by the change in API to HostResolverProc which
now takes an address family too. I don't see how to make it smaller than this...

[eroman, 2009-10-20 03:29:31]

http://codereview.chromium.org/302010/diff/1/19
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/302010/diff/1/19#newcode241
Line 241: const char kDisableIPv6[]                   = "disable-ipv6";
On 2009/10/20 02:39:01, wtc wrote:
> Are you sure Firefox has this command-line option?

Firefox has network.dns.disableipv6 which is the same thing. In fact I believe
Darin added that.

> 
> I know Firefox has a preference in which you can list hosts that are
IPv4-only.

Sure, Firefox also has the option network.dns.ipv4onlydomains to blacklist
domains that should not use IPv6.

[darin (slow to review), 2009-10-20 06:06:12]

Maybe we should be supporting IE's IPv6 extensions for PAC?
http://blogs.msdn.com/wndp/articles/IPV6_PAC_Extensions_v0_9.aspx

On Mon, Oct 19, 2009 at 4:29 PM, <eroman@chromium.org> wrote:

> Reviewers: darin,
>
> Description:
> Add a mechanism to disable IPv6.
>
> (1) Adds the ability to specify the address family on a per-request basis.
>
> (2) Exposes a --disable-ipv6 flag to chrome that changes the default
> address
> family from AF_UNSPEC to AF_INET (same sort of thing Firefox does).
>
> (3) Changes the backing datastructure for HostCache:EntryMap and
> HostResolverImpl::JobMap from a "hash_map" to a "std::map". This was for
> consistency with other code (when I went to add a custom hash trait, I
> couldn't
> find any existing code which was using hashmap for custom keys).
>
> (4) Updates about:net-internals to display an address family for the
> hostcache
> dump (since it is now a part of the key).
>
> This change is in anticipation of turning off IPv6 host resolving in the
> PAC
> utility functions (see bug 24641). But it is also a feature addition.
>
> BUG=24641
>
> TEST=HostCacheTest.AddressFamilyIsPartOfKey
>
> Please review this at http://codereview.chromium.org/302010
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/net/dns_global.cc
>  M     chrome/common/chrome_switches.h
>  M     chrome/common/chrome_switches.cc
>  M     chrome/test/unit/chrome_test_suite.h
>  A     net/base/address_family.h
>  M     net/base/address_list_unittest.cc
>  M     net/base/host_cache.h
>  M     net/base/host_cache.cc
>  M     net/base/host_cache_unittest.cc
>  M     net/base/host_resolver.h
>  M     net/base/host_resolver_impl.h
>  M     net/base/host_resolver_impl.cc
>  M     net/base/host_resolver_impl_unittest.cc
>  M     net/base/host_resolver_proc.h
>  M     net/base/host_resolver_proc.cc
>  M     net/base/mock_host_resolver.h
>  M     net/base/mock_host_resolver.cc
>  M     net/net.gyp
>  M     net/url_request/url_request_view_net_internals_job.cc
>
>
>

[eroman, 2009-10-20 16:07:30]

On Mon, Oct 19, 2009 at 11:05 PM, Darin Fisher <darin@chromium.org> wrote:
> Maybe we should be supporting IE's IPv6 extensions for PAC?
> http://blogs.msdn.com/wndp/articles/IPV6_PAC_Extensions_v0_9.aspx

Yeah, I proposed doing IE's style for myIpAddress() / dnsResolve() in
bug 24641. And we could add the extension methods too (but those are
less crucial for the immediate compatibility problem)

>
> On Mon, Oct 19, 2009 at 4:29 PM, <eroman@chromium.org> wrote:
>>
>> Reviewers: darin,
>>
>> Description:
>> Add a mechanism to disable IPv6.
>>
>> (1) Adds the ability to specify the address family on a per-request basi=
s.
>>
>> (2) Exposes a --disable-ipv6 flag to chrome that changes the default
>> address
>> family from AF_UNSPEC to AF_INET (same sort of thing Firefox does).
>>
>> (3) Changes the backing datastructure for HostCache:EntryMap and
>> HostResolverImpl::JobMap from a "hash_map" to a "std::map". This was for
>> consistency with other code (when I went to add a custom hash trait, I
>> couldn't
>> find any existing code which was using hashmap for custom keys).
>>
>> (4) Updates about:net-internals to display an address family for the
>> hostcache
>> dump (since it is now a part of the key).
>>
>> This change is in anticipation of turning off IPv6 host resolving in the
>> PAC
>> utility functions (see bug 24641). But it is also a feature addition.
>>
>> BUG=3D24641
>>
>> TEST=3DHostCacheTest.AddressFamilyIsPartOfKey
>>
>> Please review this at http://codereview.chromium.org/302010
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>> =A0M =A0 =A0 chrome/browser/net/dns_global.cc
>> =A0M =A0 =A0 chrome/common/chrome_switches.h
>> =A0M =A0 =A0 chrome/common/chrome_switches.cc
>> =A0M =A0 =A0 chrome/test/unit/chrome_test_suite.h
>> =A0A =A0 =A0 net/base/address_family.h
>> =A0M =A0 =A0 net/base/address_list_unittest.cc
>> =A0M =A0 =A0 net/base/host_cache.h
>> =A0M =A0 =A0 net/base/host_cache.cc
>> =A0M =A0 =A0 net/base/host_cache_unittest.cc
>> =A0M =A0 =A0 net/base/host_resolver.h
>> =A0M =A0 =A0 net/base/host_resolver_impl.h
>> =A0M =A0 =A0 net/base/host_resolver_impl.cc
>> =A0M =A0 =A0 net/base/host_resolver_impl_unittest.cc
>> =A0M =A0 =A0 net/base/host_resolver_proc.h
>> =A0M =A0 =A0 net/base/host_resolver_proc.cc
>> =A0M =A0 =A0 net/base/mock_host_resolver.h
>> =A0M =A0 =A0 net/base/mock_host_resolver.cc
>> =A0M =A0 =A0 net/net.gyp
>> =A0M =A0 =A0 net/url_request/url_request_view_net_internals_job.cc
>>
>>
>
>

[darin (slow to review), 2009-10-20 20:49:54]

http://codereview.chromium.org/302010/diff/1/17
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/302010/diff/1/17#newcode485
Line 485: DisableIPv6(true);
perhaps net::HostResolver should just expose a DisableIPv6 method so we can
avoid this casting, or maybe it should be a parameter specified at construction
time.  it could be passed to net::CreateSystemHostResolver.

http://codereview.chromium.org/302010/diff/1/8
File net/base/host_cache.h (right):

http://codereview.chromium.org/302010/diff/1/8#newcode40
Line 40: return other.hostname == hostname && other.address_family ==
address_family;
nit: >80 chars

http://codereview.chromium.org/302010/diff/1/6
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/302010/diff/1/6#newcode19
Line 19: HostCache::Key Key(const std::string& hostname) {
if this were also declared static, then the anonymous namespace would be
unnecessary.  alternatively, since there is an anonymous namespace, the static
modifiers on the constants above are unnecessary.

http://codereview.chromium.org/302010/diff/1/13
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/302010/diff/1/13#newcode73
Line 73: // Prevents returning IPv6 addresses from Resolve().
You might comment that IPv6 is enabled by default.

[eroman, 2009-10-20 21:04:28]

http://codereview.chromium.org/302010/diff/1/17
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/302010/diff/1/17#newcode485
Line 485: DisableIPv6(true);
On 2009/10/20 20:49:54, darin wrote:
> perhaps net::HostResolver should just expose a DisableIPv6 method so we can
> avoid this casting, or maybe it should be a parameter specified at
construction
> time.  it could be passed to net::CreateSystemHostResolver.

Done, I added it to the base class.

Note that initially I tried making it a parameter to CreateSystemHostResolver(),
but I didn't really like this approach for two reasons:
  (1) It affected lots of other files, needing to add a default "false" (and
none of those consumers cared, making it purely an uglification)

  (2) In the future we may want the flexibility to dynamically change these
sorts of settings, in which case having a setter method is more convenient than
having a constructor parameter.

http://codereview.chromium.org/302010/diff/1/8
File net/base/host_cache.h (right):

http://codereview.chromium.org/302010/diff/1/8#newcode40
Line 40: return other.hostname == hostname && other.address_family ==
address_family;
On 2009/10/20 20:49:54, darin wrote:
> nit: >80 chars

Done.
(Ooops!)

http://codereview.chromium.org/302010/diff/1/6
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/302010/diff/1/6#newcode19
Line 19: HostCache::Key Key(const std::string& hostname) {
On 2009/10/20 20:49:54, darin wrote:
> if this were also declared static, then the anonymous namespace would be
> unnecessary.  alternatively, since there is an anonymous namespace, the static
> modifiers on the constants above are unnecessary.

Done.
(Removed the unnecessary static modifiers above; my preference is towards
anonymous blocks).

http://codereview.chromium.org/302010/diff/1/13
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/302010/diff/1/13#newcode73
Line 73: // Prevents returning IPv6 addresses from Resolve().
On 2009/10/20 20:49:54, darin wrote:
> You might comment that IPv6 is enabled by default.

Done.

[wtc, 2009-10-21 03:26:49]

On 2009/10/20 02:39:01, wtc wrote:
> 
> This CL seems larger than necessary.

Eric, I haven't reviewed your CL, so I should have said "This CL is larger than
I expected".
Sorry!

[darin (slow to review), 2009-10-21 06:25:01]

http://codereview.chromium.org/302010/diff/1/17
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/302010/diff/1/17#newcode485
Line 485: DisableIPv6(true);
On 2009/10/20 21:04:28, eroman wrote:
> On 2009/10/20 20:49:54, darin wrote:
> > perhaps net::HostResolver should just expose a DisableIPv6 method so we can
> > avoid this casting, or maybe it should be a parameter specified at
> construction
> > time.  it could be passed to net::CreateSystemHostResolver.
> 
> Done, I added it to the base class.
> 
> Note that initially I tried making it a parameter to
CreateSystemHostResolver(),
> but I didn't really like this approach for two reasons:
>   (1) It affected lots of other files, needing to add a default "false" (and
> none of those consumers cared, making it purely an uglification)
> 
>   (2) In the future we may want the flexibility to dynamically change these
> sorts of settings, in which case having a setter method is more convenient
than
> having a constructor parameter.

OK, sounds good to me.

[darin (slow to review), 2009-10-21 06:26:13]

LGTM

[Lorenzo Colitti, 2009-10-21 19:44:08]

How is this a feature addition?

Darin and I implemented disabling IPv6 on Firefox in 2004, and the reasons for
doing so were that OS X didn't disable AAAA lookups when they didn't have IPv6
connectivity and that some DNS servers timed out on AAAA lookups:

https://bugzilla.mozilla.org/show_bug.cgi?id=68796

The OS X issue was fixed by OS X 10.4. The DNS issue was fixed by Windows Vista,
because when 20% of your user population has IPv6 enabled by default you are
incentivated to fix your DNS server quickly.

I don't think we should be doing this.

[eroman, 2009-10-21 20:00:48]

Having the flag is useful for debugging, and workarounds.

There have been bugs filed in Chrome where having a way to disable
IPv6 host resolving would have helped with diagnosing the problem, and
in offering a workaround.

For examples:

http://crbug.com/24641 -- PAC scripts written for IE don't work in
chrome due to our returning IPv6 results. Having the ability to
disable IPv6 at the command line would let me identify which users
that have problems with PAC are due to this issue. And it also gives
such users a temporary workaround.

http://crbug.com/23364, http://crbug.com/14228 : Resolving slowness
and resolving of localhost problems. Being able to toggle IPv6 support
would give such users a temporary fix, and help investigating such
reports (prior to knowing it is IPv6 related).


It hardly seems that current IPv6 deployments are problem free, and I
am confident that Chrome has more IPv6 problems waiting to be
discovered... In which case this flag will be seeing some action.

What is the harm in having a flag to turn this off and be able to
eliminate such variables when testing?

On Wed, Oct 21, 2009 at 12:44 PM,  <l.colitti@gmail.com> wrote:
> How is this a feature addition?
>
> Darin and I implemented disabling IPv6 on Firefox in 2004, and the reasons
> for
> doing so were that OS X didn't disable AAAA lookups when they didn't have
> IPv6
> connectivity and that some DNS servers timed out on AAAA lookups:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=68796
>
> The OS X issue was fixed by OS X 10.4. The DNS issue was fixed by Windows
> Vista,
> because when 20% of your user population has IPv6 enabled by default you are
> incentivated to fix your DNS server quickly.
>
> I don't think we should be doing this.
>
> http://codereview.chromium.org/302010
>

[Lorenzo Colitti, 2009-10-21 20:23:04]

The harm is that code needs to be added to the browser (this patch touches close
to 20 files) to address problems that are caused either by broken deployments,
broken scripts, or other bugs in the browser.

Providing a way to disable IPv6 means that those things stay broken and make
them seem unimportant compared to what they are: bugs that break connectivity.

A flag to disable IPv6 does not help users, because users don't use command
flags. So instead of adding code to implement an option to disable IPv6, why not
add code to find out if DNS lookups or connections are timing out and take
action? That would give you the same debugging potential, would give you the
ability to detect other classes of failures, and would fix problems for users
too. 

Saying that you want a way to disable IPv6 because IPv6 deployments are not
problem-free is taking the easy way out. IPv6 deployments are not problem-free
for many reasons, and one of the reasons is bugs in applications, including
Chrome (for example, IE does not suffer from any of the three bugs you cited). I
think we should fix the bugs instead of working around them.

Finally, I would argue you don't need a disable IPv6 flag to find IPv6 bugs, all
you need are wget and tcpdump.

[eroman, 2009-10-21 20:32:35]

I disagree.

IMO the correct way to fix http://crbug.com/24641 is exactly to do
IPv4-only host resolving.
We can argue about it on that bug report.

On Wed, Oct 21, 2009 at 1:23 PM,  <l.colitti@gmail.com> wrote:
> The harm is that code needs to be added to the browser (this patch touches
> close
> to 20 files) to address problems that are caused either by broken
> deployments,
> broken scripts, or other bugs in the browser.
>
> Providing a way to disable IPv6 means that those things stay broken and make
> them seem unimportant compared to what they are: bugs that break
> connectivity.
>
> A flag to disable IPv6 does not help users, because users don't use command
> flags. So instead of adding code to implement an option to disable IPv6, why
> not
> add code to find out if DNS lookups or connections are timing out and take
> action? That would give you the same debugging potential, would give you the
> ability to detect other classes of failures, and would fix problems for
> users
> too.
>
> Saying that you want a way to disable IPv6 because IPv6 deployments are not
> problem-free is taking the easy way out. IPv6 deployments are not
> problem-free
> for many reasons, and one of the reasons is bugs in applications, including
> Chrome (for example, IE does not suffer from any of the three bugs you
> cited). I
> think we should fix the bugs instead of working around them.
>
> Finally, I would argue you don't need a disable IPv6 flag to find IPv6 bugs,
> all
> you need are wget and tcpdump.
>
> http://codereview.chromium.org/302010
>

[Lorenzo Colitti, 2009-10-21 20:42:18]

Absolutely! You need to be able to support IPv4-only on a per DNS
resolution request to fix that. What I disagree with is adding code to
make this a command-line flag.

[Lorenzo Colitti, 2009-10-21 23:14:08]

http://codereview.chromium.org/302010/diff/2013/3021
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/302010/diff/2013/3021#newcode143
Line 143: bool disable_ipv6_;
Instead, why not make this be "int desired_address_family" It could default to
AF_UNSPEC and then you could disable IPv6 by setting it to AF_INET.

The advantage is that you can pass this value directly into getaddrinfo(), thus
simplifying the code. Also, it's more flexible, since it allows the caller to
disable IPv6, disable IPv4, or request any address family he wants.

[eroman, 2009-10-21 23:32:07]

http://codereview.chromium.org/302010/diff/2013/3021
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/302010/diff/2013/3021#newcode143
Line 143: bool disable_ipv6_;
On 2009/10/21 23:14:08, l.colitti wrote:
> Instead, why not make this be "int desired_address_family" It could default to
> AF_UNSPEC and then you could disable IPv6 by setting it to AF_INET.
> 
> The advantage is that you can pass this value directly into getaddrinfo(),
thus
> simplifying the code. Also, it's more flexible, since it allows the caller to
> disable IPv6, disable IPv4, or request any address family he wants.

Done.

See proposed change here: http://codereview.chromium.org/303026

(as this codereview was already committed).