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 performed on a worker thread, so latency should not
be an issue (even if we created much slower tests).

The current test appears to takes in the raneg of 50-100ms, and probably
(under the covers) does some reading from files).

BUG=25680
BUG=12754
r=wtc,eroman

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

[eroman, 2010-03-16 01:06:14]

LGTM, just some nits.

(Note I didn't review net_util thoroughly, since I assumed it was a relanding of
the earlier reviewed CL).

http://codereview.chromium.org/1006001/diff/5002/12001
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/1006001/diff/5002/12001#newcode425
net/base/host_resolver_impl.cc:425: IPv6ProbeJob(HostResolverImpl* resolver)
nit: mark as "explicit".

http://codereview.chromium.org/1006001/diff/5002/12001#newcode433
net/base/host_resolver_impl.cc:433: const bool IS_SLOW = false;
why consider this not slow?

http://codereview.chromium.org/1006001/diff/5002/12001#newcode463
net/base/host_resolver_impl.cc:463: address_family_ = IPv6Supported() ?
ADDRESS_FAMILY_UNSPECIFIED
suggestion: instead of making this a member variable, how about making it an
argument to OnProbeComplete(), and passing it in the PostTask ?

http://codereview.chromium.org/1006001/diff/5002/12001#newcode490
net/base/host_resolver_impl.cc:490: return !MessageLoop::current() ||
origin_loop_ == MessageLoop::current();
under what circumstance is this going to hit MessageLoop::current() == NULL ?

http://codereview.chromium.org/1006001/diff/5002/12001#newcode670
net/base/host_resolver_impl.cc:670: ipv6_probe_job_(NULL) {
nit: no need to init to explicitly init to NULL.

http://codereview.chromium.org/1006001/diff/5002/12001#newcode687
net/base/host_resolver_impl.cc:687: DiscardIPv6ProbeJob();
nit: I suggest naming this "cancel" instead of "discard".

http://codereview.chromium.org/1006001/diff/5002/12001#newcode1100
net/base/host_resolver_impl.cc:1100: void
HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(AddressFamily address_family)
{
nit: long line.

[jar (doing other things), 2010-03-16 04:15:25]

Almost all changes made per eroman's suggestions.

I can change the name of the method to "Cancel...," but I had it that way
originally, and (see note below) it bothered me since the semantics included
dropping the reference, and it was called in at least one place where the drop
was much more significant than the Cancel.

Please indicate if you'd really rather see it called CancelIPv6Probe and I'll
change it

http://codereview.chromium.org/1006001/diff/5002/12001
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/1006001/diff/5002/12001#newcode425
net/base/host_resolver_impl.cc:425: IPv6ProbeJob(HostResolverImpl* resolver)
On 2010/03/16 01:06:14, eroman wrote:
> nit: mark as "explicit".

Done.

http://codereview.chromium.org/1006001/diff/5002/12001#newcode433
net/base/host_resolver_impl.cc:433: const bool IS_SLOW = false;
On 2010/03/16 01:06:14, eroman wrote:
> why consider this not slow?

It wasn't clear what "slow" meant.  I assumed "slow" was a large file read, or a
long URL get with significant back-end response time.  

My guess is this will take about 40-100ms.

As a comparison, DNS will probably take over 400ms on average to resolve
(assuming it is not in the OS cache).

Based on our discussion,  I went ahead and just marked this as "true".  Perhaps
the right thing is to have an API which supplies some estimated times, or
probable bounds on times, and then let the underlying code make a decision.  For
now, I'll just leave it as true (and as you indicated, this will probably have
very little impact).

http://codereview.chromium.org/1006001/diff/5002/12001#newcode463
net/base/host_resolver_impl.cc:463: address_family_ = IPv6Supported() ?
ADDRESS_FAMILY_UNSPECIFIED
On 2010/03/16 01:06:14, eroman wrote:
> suggestion: instead of making this a member variable, how about making it an
> argument to OnProbeComplete(), and passing it in the PostTask ?

Done.

http://codereview.chromium.org/1006001/diff/5002/12001#newcode490
net/base/host_resolver_impl.cc:490: return !MessageLoop::current() ||
origin_loop_ == MessageLoop::current();
On 2010/03/16 01:06:14, eroman wrote:
> under what circumstance is this going to hit MessageLoop::current() == NULL ?

I was concerned that during shutdowns, that the message loop could be nulled out
before all tasks were run.  Am I mistaken about thread shutdown ordering?

This seemed to be a conservative DCHECK argument (that was much better than a
comment).

http://codereview.chromium.org/1006001/diff/5002/12001#newcode670
net/base/host_resolver_impl.cc:670: ipv6_probe_job_(NULL) {
On 2010/03/16 01:06:14, eroman wrote:
> nit: no need to init to explicitly init to NULL.

Done.

http://codereview.chromium.org/1006001/diff/5002/12001#newcode687
net/base/host_resolver_impl.cc:687: DiscardIPv6ProbeJob();
On 2010/03/16 01:06:14, eroman wrote:
> nit: I suggest naming this "cancel" instead of "discard".

I had it named that originally... but decided that the fact that this call would
commonly instigate a delete meant it has more significant semantics.  This was
especially visible when, after the job had done its callback, I still
"cancelled" it.  That was what made be give it the more aggressive "discard"
moniker.  If you find this to out-of-teh-norm and less readable, I can change it
back... but the "cancel after callback" seemed strange to me (but then again...
that call-site was perchance a tiny optimization... and not necessary).

http://codereview.chromium.org/1006001/diff/5002/12001#newcode1100
net/base/host_resolver_impl.cc:1100: void
HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(AddressFamily address_family)
{
On 2010/03/16 01:06:14, eroman wrote:
> nit: long line.

Done.

[eroman, 2010-03-16 04:32:31]

LGTM

http://codereview.chromium.org/1006001/diff/5002/12001
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/1006001/diff/5002/12001#newcode490
net/base/host_resolver_impl.cc:490: return !MessageLoop::current() ||
origin_loop_ == MessageLoop::current();
On 2010/03/16 04:15:25, jar wrote:
> On 2010/03/16 01:06:14, eroman wrote:
> > under what circumstance is this going to hit MessageLoop::current() == NULL
?
> 
> I was concerned that during shutdowns, that the message loop could be nulled
out
> before all tasks were run.  Am I mistaken about thread shutdown ordering?
> 
> This seemed to be a conservative DCHECK argument (that was much better than a
> comment).
> 

This shouldn't be an issue. (All tasks on the IO thread are drained during
shutdown.)

I suggest just having DCHECK_EQ(MessageLoop::current(), origin_loop_), can
always correct if it gets triggered (although realistically all it has to do is
pass unittests, since almost noone runs debug builds).

http://codereview.chromium.org/1006001/diff/5002/12001#newcode687
net/base/host_resolver_impl.cc:687: DiscardIPv6ProbeJob();
On 2010/03/16 04:15:25, jar wrote:
> On 2010/03/16 01:06:14, eroman wrote:
> > nit: I suggest naming this "cancel" instead of "discard".
> 
> I had it named that originally... but decided that the fact that this call
would
> commonly instigate a delete meant it has more significant semantics.  This was
> especially visible when, after the job had done its callback, I still
> "cancelled" it.  That was what made be give it the more aggressive "discard"
> moniker.  If you find this to out-of-teh-norm and less readable, I can change
it
> back... but the "cancel after callback" seemed strange to me (but then
again...
> that call-site was perchance a tiny optimization... and not necessary).
> 

Your call, I am fine with either.

[wtc, 2010-03-16 19:13:15]

LGTM.  Just some nits below.

http://codereview.chromium.org/1006001/diff/11002/23001
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/1006001/diff/11002/23001#newcode432
net/base/host_resolver_impl.cc:432: const bool IS_SLOW = true;
Nit: please follow the Style Guide's naming convention for
constants.  This should be named kIsSlow.

http://codereview.chromium.org/1006001/diff/11002/23001#newcode1101
net/base/host_resolver_impl.cc:1101: LOG(INFO) << "IPv6Probe forced
AddressFamily setting to "
Fix the indentation of this LOG(INFO) statement.

http://codereview.chromium.org/1006001/diff/11002/23001#newcode1106
net/base/host_resolver_impl.cc:1106: // Drop reference since the job has called
us back.
You just need to set ipv6_probe_job_ to NULL here.

http://codereview.chromium.org/1006001/diff/11002/23003
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/1006001/diff/11002/23003#newcode186
net/base/host_resolver_impl.h:186: void DiscardIPv6ProbeJob();
Perhaps "Cancel" instead of "Discard"?

http://codereview.chromium.org/1006001/diff/11002/23002
File net/base/net_util.cc (right):

http://codereview.chromium.org/1006001/diff/11002/23002#newcode1581
net/base/net_util.cc:1581: IPv6SupportResults(IPV6_CANNOT_CREATE_SOCKETS);
It's a little confusing that you're not using
IPV6_CAN_CREATE_SOCKETS on OS_POSIX now.

http://codereview.chromium.org/1006001/diff/11002/23002#newcode1609
net/base/net_util.cc:1609: reinterpret_cast<struct sockaddr_in6*>(addr);
Nit: indent by 4.

[jar (doing other things), 2010-03-16 22:22:47]

Changes made per wtc's comment (responded to below) will be checked in with CL
http://codereview.chromium.org/1057001

http://codereview.chromium.org/1006001/diff/11002/23001
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/1006001/diff/11002/23001#newcode432
net/base/host_resolver_impl.cc:432: const bool IS_SLOW = true;
On 2010/03/16 19:13:15, wtc wrote:
> Nit: please follow the Style Guide's naming convention for
> constants.  This should be named kIsSlow.

Done.

http://codereview.chromium.org/1006001/diff/11002/23001#newcode1101
net/base/host_resolver_impl.cc:1101: LOG(INFO) << "IPv6Probe forced
AddressFamily setting to "
On 2010/03/16 19:13:15, wtc wrote:
> Fix the indentation of this LOG(INFO) statement.

Done.

http://codereview.chromium.org/1006001/diff/11002/23001#newcode1106
net/base/host_resolver_impl.cc:1106: // Drop reference since the job has called
us back.
On 2010/03/16 19:13:15, wtc wrote:
> You just need to set ipv6_probe_job_ to NULL here.

If you or Eroman insist, I'd change this (and/or the method name).  IMO, it is
better to centralize the dancel and decref functionality, and only write it once
(correctly), rather than doing subtle items like assigning to null to achieve
the decref.  If you wanted, I could also drop this optimization, as all that
would happen is the ProbeTask would be held until shutdown, but it is not a big
object anyway.

http://codereview.chromium.org/1006001/diff/11002/23002
File net/base/net_util.cc (right):

http://codereview.chromium.org/1006001/diff/11002/23002#newcode1581
net/base/net_util.cc:1581: IPv6SupportResults(IPV6_CANNOT_CREATE_SOCKETS);
On 2010/03/16 19:13:15, wtc wrote:
> It's a little confusing that you're not using
> IPV6_CAN_CREATE_SOCKETS on OS_POSIX now.

On posix, we have a more refined set of error messages (courtesy of the
additional probing).  I think it is common to see different error messages
appear on different platforms, especially when different OS support is present.

http://codereview.chromium.org/1006001/diff/11002/23002#newcode1609
net/base/net_util.cc:1609: reinterpret_cast<struct sockaddr_in6*>(addr);
On 2010/03/16 19:13:15, wtc wrote:
> Nit: indent by 4.

Done.