Reduce frequency of IPv6 probes in HostResolverImpl.

Reduce frequency of IPv6 probes in HostResolverImpl.

Currently HostResolverImpl tries to probe IPv6 support for every
Resolve() request, i.e. very often. The probe is implemented using
a dummy UDP socket. This doesn't look efficient because IPv6 state
doesn't change often.

Also this change will decrease frequency of the crash in the linked bug.
In that bug the root cause is that on OSX system certificate library may
close recently created file descriptors it doesn't own. Not creating
dummy UDP sockets very often will help to avoid hitting this bug in the
OS very often.

BUG=481163
Committed: https://crrev.com/b8cdc219c478bde98b36acebcc859f1785a90119
Cr-Commit-Position: refs/heads/master@{#329885}

[Sergey Ulanov, 2015-05-11 23:02:58]

sergeyu@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-05-12 14:39:58]

mmenke@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mmenke, 2015-05-12 14:39:59]

Paul:  Mind taking this one?

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2197: NetLog::BoolCallback("ipv6_available",
ipv6_available));
Think this makes more sense in ProbeIpv6SupportCached - maybe also log if the
result was cached or not?

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2212: bool
HostResolverImpl::ProbeIpv6SupportCached(const BoundNetLog& net_log) {
Think this method name is a little weird.  Maybe IsIpv6Reachable?

[Sergey Ulanov, 2015-05-12 19:22:24]

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2197: NetLog::BoolCallback("ipv6_available",
ipv6_available));
On 2015/05/12 14:39:59, mmenke wrote:
> Think this makes more sense in ProbeIpv6SupportCached - maybe also log if the
> result was cached or not?

Done.

https://codereview.chromium.org/1138833003/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2212: bool
HostResolverImpl::ProbeIpv6SupportCached(const BoundNetLog& net_log) {
On 2015/05/12 14:39:59, mmenke wrote:
> Think this method name is a little weird.  Maybe IsIpv6Reachable?

Done. Thanks for suggesting a better name!

[davidben, 2015-05-12 20:18:43]

(Tiny drive-by description nit: system SSL library -> system certificate
library. We have our own SSL stack, but we still call out to Apple's code for
certificates, presuming that's the actual root cause of the bug.)

[pauljensen, 2015-05-13 13:00:23]

Can you add a test for this?  Like just checking that two successive calls
return the same result and the netlog says the second call is cached?

Also, I'm a little confused by the description of this CL and how it helps
alleviate the described bug.  I don't have access to the linked bug so I'm
probably misguided.  It would seem like creating more short-lived sockets will
give the problematic OSX code more harmless targets to inadvertently close,
rather than have it close more important Chrome sockets, like those used to
fetch resources.

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:79: const int kIpv6ProbePeriodMs = 1000;
kIpv6->kIPv6 like kIPv6Address

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1831: last_ipv6_probe_result_(false),
nit: Even though this initial value should never be used (unless someone sets
their clock prior to unix epoch) I think defaulting to true represents a safer
initial value.  I believe this would just mean connect() fails instantly on the
IPv6 results.

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2201: if (!ipv6_available) {
Can we combine these two lines into "if (!IsIpv6Reachable(net_log))"

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2226: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x88,
0x88 };
Can we either move kIPv6Address up to the beginning of the file with the other
constants, or move kIpv6ProbePeriodMs down to this function?  To be consistent.

[Sergey Ulanov, 2015-05-13 17:16:03]

On 2015/05/13 13:00:23, pauljensen wrote:
> Can you add a test for this?  Like just checking that two successive calls
> return the same result and the netlog says the second call is cached?
> 
> Also, I'm a little confused by the description of this CL and how it helps
> alleviate the described bug.  I don't have access to the linked bug so I'm
> probably misguided.  It would seem like creating more short-lived sockets will
> give the problematic OSX code more harmless targets to inadvertently close,
> rather than have it close more important Chrome sockets, like those used to
> fetch resources.

I've added you in CC so you can access the bug now. The broken library is doing
the following sequence of syscalls:
 1. fd = open(..);
 2. close(fd);
 3. close(fd);

In the best case the second close() just returns EBADF because fd is invalid at
that point. The problem happens when another thread tries to allocate a new file
descriptor (i.e. calls open() or socket()) between (2) and (3) - in that case it
gets the same fd value that was closed on step (2) and so that FD is immediately
closed again on step (3). So the more frequently new file descriptors are
allocated the more likely they are to be closed.

[mmenke, 2015-05-13 17:22:44]

On 2015/05/13 17:16:03, Sergey Ulanov wrote:
> On 2015/05/13 13:00:23, pauljensen wrote:
> > Can you add a test for this?  Like just checking that two successive calls
> > return the same result and the netlog says the second call is cached?
> > 
> > Also, I'm a little confused by the description of this CL and how it helps
> > alleviate the described bug.  I don't have access to the linked bug so I'm
> > probably misguided.  It would seem like creating more short-lived sockets
will
> > give the problematic OSX code more harmless targets to inadvertently close,
> > rather than have it close more important Chrome sockets, like those used to
> > fetch resources.
> 
> I've added you in CC so you can access the bug now. The broken library is
doing
> the following sequence of syscalls:
>  1. fd = open(..);
>  2. close(fd);
>  3. close(fd);
> 
> In the best case the second close() just returns EBADF because fd is invalid
at
> that point. The problem happens when another thread tries to allocate a new
file
> descriptor (i.e. calls open() or socket()) between (2) and (3) - in that case
it
> gets the same fd value that was closed on step (2) and so that FD is
immediately
> closed again on step (3). So the more frequently new file descriptors are
> allocated the more likely they are to be closed.

I can't speak towards that bug, but think it's worth noting that probes is also
a cause of jank - see
https://code.google.com/p/chromium/issues/detail?id=455942.

[Sergey Ulanov, 2015-05-13 18:15:59]

On 2015/05/13 13:00:23, pauljensen wrote:
> Can you add a test for this?  Like just checking that two successive calls
> return the same result and the netlog says the second call is cached?
 
I think test like this would be flaky - it would fail if machine is slow and it
takes more than a second to run the two consecutive calls. It can be resolved by
injecting a fake TickClock, but I don't think it's worth it for simple code like
this.

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:79: const int kIpv6ProbePeriodMs = 1000;
On 2015/05/13 13:00:22, pauljensen wrote:
> kIpv6->kIPv6 like kIPv6Address

Done, though IMO this is not right. In CamelCase names abbreviations should have
only the first character upper-cased. Style guide is not explicit about it, but
it does give example of DeleteUrl() being a proper function name and not
DeleteURL(). If that rule was followed we wouldn't have unreadable names such as
EnsureNSSSSLInit().

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1831: last_ipv6_probe_result_(false),
On 2015/05/13 13:00:22, pauljensen wrote:
> nit: Even though this initial value should never be used (unless someone sets
> their clock prior to unix epoch) I think defaulting to true represents a safer
> initial value.  I believe this would just mean connect() fails instantly on
the
> IPv6 results.

Done.

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2201: if (!ipv6_available) {
On 2015/05/13 13:00:22, pauljensen wrote:
> Can we combine these two lines into "if (!IsIpv6Reachable(net_log))"

Done.

https://codereview.chromium.org/1138833003/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2226: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x88,
0x88 };
On 2015/05/13 13:00:22, pauljensen wrote:
> Can we either move kIPv6Address up to the beginning of the file with the other
> constants, or move kIpv6ProbePeriodMs down to this function?  To be
consistent.

Done.

[pauljensen, 2015-05-13 18:25:26]

On 2015/05/13 18:15:59, Sergey Ulanov wrote:
> On 2015/05/13 13:00:23, pauljensen wrote:
> > Can you add a test for this?  Like just checking that two successive calls
> > return the same result and the netlog says the second call is cached?
>  
> I think test like this would be flaky - it would fail if machine is slow and
it
> takes more than a second to run the two consecutive calls. It can be resolved
by
> injecting a fake TickClock, but I don't think it's worth it for simple code
like
> this.

I think a test is important, even for this simple code.  We had an inadvertent
breakage in this code in the last year, so it is fragile.
I think the chance of a flake is extremely rare, and probably indicative of a
more serious problem as this code should generally complete in <10ms.
Flakes could be avoided by rerunning the test if the total time taken by the
test is >1s.

[Sergey Ulanov, 2015-05-13 18:45:21]

PTAL, added a unittest now

[pauljensen, 2015-05-14 13:32:28]

lgtm

[pauljensen, 2015-05-14 13:36:29]

And further, thanks for doing this!  I think the whole IPv6 probing is
more-or-less an optimization (saves wasting time connect()ing to unreachable
addresses and saves time doing unnecessary AAAA queries)...and we shouldn't have
optimizations that actually slow things down and add jank :)  I've been trying
to reduce these probes (crrev.com/1088513004)

[Sergey Ulanov, 2015-05-14 17:45:44]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-05-14 17:46:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138833003/60001

[commit-bot: I haz the power, 2015-05-14 18:50:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-14 18:51:28]

Patchset 4 (id:??) landed as
https://crrev.com/b8cdc219c478bde98b36acebcc859f1785a90119
Cr-Commit-Position: refs/heads/master@{#329885}

[Sergey Ulanov, 2015-05-14 22:15:26]

BUG= is incorrect on this cl :(
correct bug link: crbug.com/461246