Fix crash when trying to connect to an IPv6 IP via a SOCKS4 proxy.

Fix crash when trying to connect to an IPv6 IP via a SOCKS4 proxy.

The socks client socket correctly requests the HostResolver only
give it IPv4 addresses, but the HostResolver ignored that flag when
passed IPs directly.  This fixes that behavior.

BUG=379031
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288143

[mmenke, 2014-08-01 21:03:55]

PTAL, thanks!

[Deprecated (see juliatuttle), 2014-08-04 20:32:20]

https://codereview.chromium.org/435603008/diff/20001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/435603008/diff/20001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2042: // Don't return IPv6 addresses if default
address family is not set to IPv4,
This comment doesn't read right to me.

"Don't return IPv6 addresses if default address family is set to IPv4 and IPv6
probes are disabled." sounds correct to me.

https://codereview.chromium.org/435603008/diff/20001/net/socket/socks_client_...
File net/socket/socks_client_socket_unittest.cc (right):

https://codereview.chromium.org/435603008/diff/20001/net/socket/socks_client_...
net/socket/socks_client_socket_unittest.cc:423: // Use a real HostResolver for
this test, as the MockHostResolver has its own
Perhaps test the behavior of both of them, to ensure they stay in sync?

[eroman, 2014-08-05 22:33:41]

lgtm

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2040: default_address_family_ ==
ADDRESS_FAMILY_IPV4 &&
As an aside: I am starting to think that "default address family" may not be the
best naming. More like "restrict to address family"

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2041: !probe_ipv6_support_) {
Can you test for !probe_ipv6_support_ before default_address_family_ ? (Since it
only makes sense in the context where that is false, wouldn't want this to
become an uninitialized memory read in the future).

[mmenke, 2014-08-05 22:50:38]

Thanks for the feedback.

tuttle:  I'll wait for your signoff before landing.

https://codereview.chromium.org/435603008/diff/20001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/435603008/diff/20001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2042: // Don't return IPv6 addresses if default
address family is not set to IPv4,
On 2014/08/04 20:32:20, ttuttle wrote:
> This comment doesn't read right to me.
> 
> "Don't return IPv6 addresses if default address family is set to IPv4 and IPv6
> probes are disabled." sounds correct to me.

Done.

https://codereview.chromium.org/435603008/diff/20001/net/socket/socks_client_...
File net/socket/socks_client_socket_unittest.cc (right):

https://codereview.chromium.org/435603008/diff/20001/net/socket/socks_client_...
net/socket/socks_client_socket_unittest.cc:423: // Use a real HostResolver for
this test, as the MockHostResolver has its own
On 2014/08/04 20:32:20, ttuttle wrote:
> Perhaps test the behavior of both of them, to ensure they stay in sync?

Done.

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2040: default_address_family_ ==
ADDRESS_FAMILY_IPV4 &&
On 2014/08/05 22:33:41, eroman wrote:
> As an aside: I am starting to think that "default address family" may not be
the
> best naming. More like "restrict to address family"

I agree that this would be much clearer.

https://codereview.chromium.org/435603008/diff/60001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:2041: !probe_ipv6_support_) {
On 2014/08/05 22:33:41, eroman wrote:
> Can you test for !probe_ipv6_support_ before default_address_family_ ? (Since
it
> only makes sense in the context where that is false, wouldn't want this to
> become an uninitialized memory read in the future).

Done.

[Deprecated (see juliatuttle), 2014-08-07 17:56:35]

lgtm

[mmenke, 2014-08-07 17:58:15]

On 2014/08/07 17:56:35, ttuttle wrote:
> lgtm

Thanks!

[mmenke, 2014-08-07 17:58:26]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:09:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/435603008/80001

[commit-bot: I haz the power, 2014-08-07 21:34:20]

Change committed as 288143