net: Add fuzzer for HostResolverImpl.

net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
Committed: https://crrev.com/91c1716731622e720e91dc052aaee9961a009423
Cr-Commit-Position: refs/heads/master@{#397429}

[mmenke, 2016-05-04 15:04:54]

Patchset #2 (id:20001) has been deleted

[mmenke, 2016-05-04 15:08:42]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

This fuzzed it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

BUG=600005
==========

[mmenke, 2016-05-04 15:08:50]

Patchset #2 (id:40001) has been deleted

[mmenke, 2016-05-04 15:12:50]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

[mmenke, 2016-05-05 17:41:32]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes tests that use HostResolverImpl no longer use the
WorkerPool, to avoid leaking objects (Getting rid of the leak was
simpler than updating the suppression for it).

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

[mmenke, 2016-05-11 20:03:26]

Patchset #5 (id:120001) has been deleted

[mmenke, 2016-05-11 20:03:37]

Patchset #8 (id:200001) has been deleted

[mmenke, 2016-05-11 20:03:49]

Patchset #7 (id:180001) has been deleted

[mmenke, 2016-05-11 20:24:25]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes tests that use HostResolverImpl no longer use the
WorkerPool, to avoid leaking objects (Getting rid of the leak was
simpler than updating the suppression for it).

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

[mmenke, 2016-05-11 22:01:28]

Patchset #11 (id:300001) has been deleted

[mmenke, 2016-05-12 17:29:58]

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

[mmenke, 2016-05-12 17:30:00]

Eric:  PTAL.  This CL is a bit simpler than its size suggests. 
fuzzed_host_resolver and host_resolver_impl_fuzzer are where all the fun stuff
is.  fuzzed_host_resolver is also intended for use in the URLRequest fuzzer
(Without the async resolver enabled).  I was get an earlier version to
successfully use the built-in DNS client over mock UDP sockets, but the current
version has enough bells and whistles that I haven't seen it succeed yet.  I'll
see if it needs a corpus after a week or two of running.

Julia:  Think you're probably be person most familiar with the async resolver. 
Any other knob else I should be fuzzing there, or any suggestions for my
dictionary?

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1968: HostResolverImpl::HostResolverImpl(
Note that this method is exactly the same as the the old constructor, except it
sets worker_task_runner_, and calls RunLoopbackProbeJob instead of starting the
probe job directly.

[eroman, 2016-05-12 21:58:31]

just a heads up not sure I will get to this today.

[mmenke, 2016-05-12 22:07:33]

On 2016/05/12 21:58:31, eroman wrote:
> just a heads up not sure I will get to this today.

Not a problem, no rush on this.

[Julia Tuttle, 2016-05-13 17:15:12]

juliatuttle@chromium.org changed reviewers:
+ juliatuttle@chromium.org

[Julia Tuttle, 2016-05-13 17:15:13]

looks reasonable to me; added a couple nits.

https://codereview.chromium.org/1946793002/diff/360001/net/data/dns/dns.dict
File net/data/dns/dns.dict (right):

https://codereview.chromium.org/1946793002/diff/360001/net/data/dns/dns.dict#...
net/data/dns/dns.dict:7: # A couple 16-bit big-endian values. Useful in a number
of fields.
12 is 10 more than a couple.

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:146: switch
(data_provider_->ConsumeValueInRange(0, 3)) {
It bothers me slightly that this construction keeps us from creating, for
example, { 8.8.8.8, 8.8.4.4 } as a nameserver list, and ditto for the others
below, to a lesser degree.

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:195: // Timeouts don't really work for fuzzing.
Even a timeout of 0 milliseconds
I understand that fuzzing the timeout doesn't work, but why did you pick such a
long timeout for the constant value?

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:688: // TODO(mmenke): Chrome code generally
doesn't handle failed PostTasks.
Do we have a way of knowing how often we've been hitting this in release builds?
I'd be okay with you ignoring the failure if we're not hitting except in really
strange cases.

[mmenke, 2016-05-17 19:50:37]

Sorry for the slow response, a lot of reviews.

[eroman]:  PTAL.
[juliatuttle]:  Feel free to take another look, if you want.  Know you're out.

https://codereview.chromium.org/1946793002/diff/360001/net/data/dns/dns.dict
File net/data/dns/dns.dict (right):

https://codereview.chromium.org/1946793002/diff/360001/net/data/dns/dns.dict#...
net/data/dns/dns.dict:7: # A couple 16-bit big-endian values. Useful in a number
of fields.
On 2016/05/13 17:15:13, Julia Tuttle wrote:
> 12 is 10 more than a couple.

Done.

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:146: switch
(data_provider_->ConsumeValueInRange(0, 3)) {
On 2016/05/13 17:15:13, Julia Tuttle wrote:
> It bothers me slightly that this construction keeps us from creating, for
> example, { 8.8.8.8, 8.8.4.4 } as a nameserver list, and ditto for the others
> below, to a lesser degree.

Fuzzed this, and the IPs in the hosts file (And the number / type / order of
those IPs).  Still have hard-coded DNS addresses, though.

https://codereview.chromium.org/1946793002/diff/360001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:195: // Timeouts don't really work for fuzzing.
Even a timeout of 0 milliseconds
On 2016/05/13 17:15:13, Julia Tuttle wrote:
> I understand that fuzzing the timeout doesn't work, but why did you pick such
a
> long timeout for the constant value?

Because if it ever triggers, it causes non-determinism.  If it's 5 milliseconds,
sometimes it may trigger, sometimes it may not, depending on whatever else the
test system is doing.  If a fuzzer does one thing on input 1% of the time, and
another thing on it 99% of the time, it's not a good fuzzer - if that 1% is a
bad failure, then you can basically never repro it, which defeats the point of
recording bad input cases to debug.

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/360001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:688: // TODO(mmenke): Chrome code generally
doesn't handle failed PostTasks.
On 2016/05/13 17:15:13, Julia Tuttle wrote:
> Do we have a way of knowing how often we've been hitting this in release
builds?
> I'd be okay with you ignoring the failure if we're not hitting except in
really
> strange cases.

WorkerPool::PostTask unconditionally returns true on POSIX, 

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor...

Looks like it can fail on Windows, but we aren't collecting metrics:
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor...

[mmenke, 2016-05-17 22:42:08]

eroman:  I discovered that the URLRequest fuzzer can hit a DCHECK in the cookie
code, because the MockHostResolver doesn't have a DNS validity check, so can
return hits for domain names that start with a ".".  Hopefully switching to this
will fix that problem, since HostResolverImpl *does* check for hostname
validity.  Just shows that bad fuzzer mocks can lead to unexpected things
happening that can't, in production code.  Anyhow, I thought it was amusing.

[eroman, 2016-05-17 23:01:34]

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
File net/base/fuzzed_data_provider.h (right):

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:36: // Returns an unsigned number in the range
[min, max] by consuming bytes from
This comment should be generalized now (since it applies to both).

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:40: uint32_t ConsumeValueInRange(uint32_t min,
uint32_t max);
Suggest renaming these to:

ConsumeUint32InRange()
ConsumeInt32InRange()

https://codereview.chromium.org/1946793002/diff/380001/net/dns/dns_client.h
File net/dns/dns_client.h (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/dns_client.h#n...
net/dns/dns_client.h:11: #include "net/base/rand_callback.h"
Hmm, yuck.

I didn't know we had such a typdef/file in //net/base.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/dns_client.h#n...
net/dns/dns_client.h:45: // a deterministic "random" number generator.
|socket_factory| must outlive
optional: Fine to drop the quotes around "random". Or just call it a PRNG.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:103: while (data_provider_->ConsumeBool()) {
Is consuming bools here the best approach? (as opposed to consuming a
single-byte length).

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:111: while (data_provider_->ConsumeBool()) {
same question

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:171: while (data_provider_->ConsumeBool()) {
Same question throughout this file.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:196: if (data_provider_->ConsumeBool()) {
nit: Suggest just calling FuzzIPAddress() instead. Then call
GetAddressFamily(ip_addr) to get the other needed param.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:607: // Calls HostResolverProc on the WorkerPool.
Performs retries if necessary.
Update WorkerPool comment

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:688: // TODO(mmenke): Chrome code generally
doesn't handle failed PostTasks.
Arguably that other code is wrong :)
Also best not to reference Chrome in //net code.
Plus, Chromium not Chrome

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:718: // WARNING: This code runs inside a worker
pool. The shutdown code cannot
These comments all assume the non-joinable worker pool threads.
Which doesn't necessarily hold given the thread pool is dependency-injected in.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:749: // Makes next attempt if DoLookup() has not
finished (runs on task runner
update "task runner" in comment (which is now ambiguous).
Or just delete, the DCHECK() also documents this.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:960: // Task runner for the call to the platform
address resolver.
Not necessarily using the "platform" address resolver.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:963: // Used to post events onto the network
thread.
I wonder if "origin" vs "worker" would be more clear.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:996: // Wraps a call to HaveOnlyLoopbackAddresses
to be executed on the WorkerPool as
Update WorkerPool comment

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:103: for (auto request =
dns_requests_->begin(); request != dns_requests_->end();
for( auto request : dns_requests_) ?

https://codereview.chromium.org/1946793002/diff/380001/net/udp/fuzzed_datagra...
File net/udp/fuzzed_datagram_client_socket.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/udp/fuzzed_datagra...
net/udp/fuzzed_datagram_client_socket.cc:46: return
kConnectErrors[data_provider_->ConsumeValueInRange(
Might be handy to have a templetized helper that selects a value given an array
(having seen this in a few places).

[mmenke, 2016-05-19 19:09:46]

Thanks for the feedback!

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
File net/base/fuzzed_data_provider.h (right):

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:36: // Returns an unsigned number in the range
[min, max] by consuming bytes from
On 2016/05/17 23:01:33, eroman wrote:
> This comment should be generalized now (since it applies to both).

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:40: uint32_t ConsumeValueInRange(uint32_t min,
uint32_t max);
On 2016/05/17 23:01:33, eroman wrote:
> Suggest renaming these to:
> 
> ConsumeUint32InRange()
> ConsumeInt32InRange()

Done.  Like those names much better.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/dns_client.h
File net/dns/dns_client.h (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/dns_client.h#n...
net/dns/dns_client.h:45: // a deterministic "random" number generator.
|socket_factory| must outlive
On 2016/05/17 23:01:33, eroman wrote:
> optional: Fine to drop the quotes around "random". Or just call it a PRNG.

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:103: while (data_provider_->ConsumeBool()) {
On 2016/05/17 23:01:34, eroman wrote:
> Is consuming bools here the best approach? (as opposed to consuming a
> single-byte length).

I went with this pattern for three reasons:

1)  More likely to return 0-size and 1-size sets of values, which are
particularly interesting, and more likely to result in the fuzzer finding valid
stuff.
2)  No arbitrary length limit (Though suppose limit on fuzz data length does
implicitly impose one).
3)  Most importantly, it saves one whole line of code!  I could count down
instead, but that seems less natural than counting up, to me.

I've switched all instances of this pattern.  I don't feel too strongly about
it.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:111: while (data_provider_->ConsumeBool()) {
On 2016/05/17 23:01:33, eroman wrote:
> same question

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:171: while (data_provider_->ConsumeBool()) {
On 2016/05/17 23:01:34, eroman wrote:
> Same question throughout this file.

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:196: if (data_provider_->ConsumeBool()) {
On 2016/05/17 23:01:34, eroman wrote:
> nit: Suggest just calling FuzzIPAddress() instead. Then call
> GetAddressFamily(ip_addr) to get the other needed param.

Done.  I hadn't realized there was such a method - the fact that it's not a
method in IPAddress makes is not very discoverable.  I seem to remember some
code explicitly checking the length of an IPAddress to do this somewhere, which
just seemed ugly.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:607: // Calls HostResolverProc on the WorkerPool.
Performs retries if necessary.
On 2016/05/17 23:01:34, eroman wrote:
> Update WorkerPool comment

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:688: // TODO(mmenke): Chrome code generally
doesn't handle failed PostTasks.
On 2016/05/17 23:01:34, eroman wrote:
> Arguably that other code is wrong :)
> Also best not to reference Chrome in //net code.
> Plus, Chromium not Chrome

Just removed the TODO.  In Chrome, the preference is not to handle things that
can't happen (And it seems that in practice, this can't happen).  If you feel we
should be handling all possible PostTask failures, I'm more than happy to file a
bug and assigned it to you.  :)

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:718: // WARNING: This code runs inside a worker
pool. The shutdown code cannot
On 2016/05/17 23:01:34, eroman wrote:
> These comments all assume the non-joinable worker pool threads.
> Which doesn't necessarily hold given the thread pool is dependency-injected
in.

I've searched for every instance of "worker", and updated the comments.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:749: // Makes next attempt if DoLookup() has not
finished (runs on task runner
On 2016/05/17 23:01:34, eroman wrote:
> update "task runner" in comment (which is now ambiguous).
> Or just delete, the DCHECK() also documents this.

Removed the comment, agree the DCHECK is sufficient.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:960: // Task runner for the call to the platform
address resolver.
On 2016/05/17 23:01:34, eroman wrote:
> Not necessarily using the "platform" address resolver.

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:963: // Used to post events onto the network
thread.
On 2016/05/17 23:01:34, eroman wrote:
> I wonder if "origin" vs "worker" would be more clear.

I prefer network thread.  "network thread" sounds like a specific thread. 
"Origin thread" could imply any thread the HostResolver is called on.

(I agree that "network" isn't great or relevant here, though - open to other
ideas)

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:996: // Wraps a call to HaveOnlyLoopbackAddresses
to be executed on the WorkerPool as
On 2016/05/17 23:01:34, eroman wrote:
> Update WorkerPool comment

Done.

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:103: for (auto request =
dns_requests_->begin(); request != dns_requests_->end();
On 2016/05/17 23:01:34, eroman wrote:
> for( auto request : dns_requests_) ?

I believe that doesn't work, for two reasons:  "auto request" is a
unique_ptr<>&, and non-const refs are forbidden.  Also, you can't use erase when
you do that.

https://codereview.chromium.org/1946793002/diff/380001/net/udp/fuzzed_datagra...
File net/udp/fuzzed_datagram_client_socket.cc (right):

https://codereview.chromium.org/1946793002/diff/380001/net/udp/fuzzed_datagra...
net/udp/fuzzed_datagram_client_socket.cc:46: return
kConnectErrors[data_provider_->ConsumeValueInRange(
On 2016/05/17 23:01:34, eroman wrote:
> Might be handy to have a templetized helper that selects a value given an
array
> (having seen this in a few places).

Done.  I wasn't sure how to do this, or I would have done it earlier.

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
File net/base/fuzzed_data_provider.h (right):

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:59: Type PickArrayEntry(Type (&array)[size]) {
Open to other names here.  "ConsumeArrayEntry" or "ConsumeValueFromArray" both
sound like the array is being modified.  Also, is this what you had in mind, or
were you thinking of the (simpler, more reedable) "PickArrayEntry(Type* array,
size_t size)"

[mmenke, 2016-05-19 19:10:37]

Note that there were failures in the tryruns, but both failing browser tests
have been identified as flaky by the bots, and have had bugs filed against them
in the last 2 days.

[eroman, 2016-05-27 01:04:15]

Sorry, totally forgot this was still in my queue!

On Thu, May 19, 2016 at 12:10 PM, <mmenke@chromium.org> wrote:

> Note that there were failures in the tryruns, but both failing browser
> tests
> have been identified as flaky by the bots, and have had bugs filed against
> them
> in the last 2 days.
>
> https://codereview.chromium.org/1946793002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-05-27 01:12:35]

No worries, I'm in no hurry - not working on more fuzzers at the moment.

On 2016/05/27 01:04:15, eroman wrote:
> Sorry, totally forgot this was still in my queue!
> 
> On Thu, May 19, 2016 at 12:10 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > Note that there were failures in the tryruns, but both failing browser
> > tests
> > have been identified as flaky by the bots, and have had bugs filed against
> > them
> > in the last 2 days.
> >
> > https://codereview.chromium.org/1946793002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[eroman, 2016-06-01 01:47:21]

I was initially concerned by the HostResolverImpl refactor, that adds the worker
task runner as an injectable dependency.

After reading this again though, I don't find that as offensive, and maybe even
like the abstraction...

I suppose my remaining concerns are:

  * It makes it less clear in HostResolverImpl that things are running in a
worker pool (which has some extra considerations for code running on it). Meh,
good enough documentation.

  * Would it be stronger for the fuzzer to be exercising the same code as
production? (i.e. worker pool). Yes there are going to be more difficult
ordering concerns, which I appreciate you tried to make the runs deterministic
based on fuzz input. I think we could achieve something similar by instrumenting
the resolveproc and serializing completion order  at that level -- although to
avoid racing which resolve proc posts back to network thread we would
additionally need to wait for request completions are observed. Which is both
ugly and also misses some coverage of threading interactions.

I dunno, thinking out loud, but am curious on your thoughts.

At any rate LGTM as is once comments are addressed.

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
File net/base/fuzzed_data_provider.h (right):

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:58: template <typename Type, int size>
nit: size_t -- Although I can't imagine it making a difference in practice.

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:59: Type PickArrayEntry(Type (&array)[size]) {
On 2016/05/19 19:09:46, mmenke wrote:
> Open to other names here.  "ConsumeArrayEntry" or "ConsumeValueFromArray" both
> sound like the array is being modified.  Also, is this what you had in mind,
or
> were you thinking of the (simpler, more reedable) "PickArrayEntry(Type* array,
> size_t size)"

Yep this is what I was thinking.

Current name is fine by me.

Another idea is PickValueFromArray()

https://codereview.chromium.org/1946793002/diff/420001/net/dns/dns_socket_poo...
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/dns_socket_poo...
net/dns/dns_socket_pool.cc:196: unsigned socket_index =
rand_int_callback().Run(0, pool.size() - 1);
Rather than exposing the callback , how about exposing a function that does
this.

Say, GetRandomInt(.., ...)

(the callback is more of an implementation detail of the base class).

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:34: // Port 0 is magic.
but shouldn't be

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:103: uint16_t num_ipv6_addresses =
data_provider_->ConsumeUint8();
nit: any reason to use uint16 here (rather than uint8, or a size_t) ?

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:112: uint16_t num_ipv4_addresses =
data_provider_->ConsumeUint8();
ditto

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (left):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1885: PrioritizedDispatcher::Limits job_limits =
options.GetDispatcherLimits();
This would be easier to review if it had not moved in the file. Will skim over
the moved code but otherwise assume it hasn't been changed in any meaningful
way...

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:718: // cannot wait for it to finish, so this code
must ve very careful about using
ve --> be

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:119: done = true;
is fallthrough intentional?

[mmenke, 2016-06-01 18:08:08]

On 2016/06/01 01:47:21, eroman wrote:
> I was initially concerned by the HostResolverImpl refactor, that adds the
worker
> task runner as an injectable dependency.
> 
> After reading this again though, I don't find that as offensive, and maybe
even
> like the abstraction...
> 
> I suppose my remaining concerns are:
> 
>   * It makes it less clear in HostResolverImpl that things are running in a
> worker pool (which has some extra considerations for code running on it). Meh,
> good enough documentation.
> 
>   * Would it be stronger for the fuzzer to be exercising the same code as
> production? (i.e. worker pool). Yes there are going to be more difficult
> ordering concerns, which I appreciate you tried to make the runs deterministic
> based on fuzz input. I think we could achieve something similar by
instrumenting
> the resolveproc and serializing completion order  at that level -- although to
> avoid racing which resolve proc posts back to network thread we would
> additionally need to wait for request completions are observed. Which is both
> ugly and also misses some coverage of threading interactions.

I think this just gets a little too ugly.  When we start a job, we'd also have
to wait on it calling the HostResolverProc.  Perhaps I've been working on Chrome
too long, but all that locking and event handling makes me nervous.  We'd also
have to make sure we complete all jobs before the test completes.  I think
having to spin the message loop each time is about as bad as not using a
WorkerPool, in terms of lost test coverage, and I'm reluctant to adding yet more
complexity to these tests, particularly CL that is already 1,000 lines long.  It
may be worth doing in a followup, but I'm moving away from fuzzers now.  I'd
really like to get HTTP2 working in the URLRequest fuzzer, so may still do that,
but given how few issues we've unearthed so far, I don't expect to do much else
there for a while, unless our current ones start turning up more bugs.

I had another idea - we could make our own MessageLoop class.  Is has a normal
message queue, but also has a bogus worker pool.  Whenever it runs a task, it
would randomly pick between the next task on the normal message queue, or one of
the tasks posted to the worker pool.  While we wouldn't be using a real worker
pool, it would get us pretty much everything else, I believe.  We could even
make it mock out time passing for calls to PostDelayedTask, if we really wanted,
though we'd need to be careful about eating too many random bits.

> I dunno, thinking out loud, but am curious on your thoughts.

[mmenke, 2016-06-01 18:08:40]

On 2016/06/01 18:08:08, mmenke wrote:
> On 2016/06/01 01:47:21, eroman wrote:
> > I was initially concerned by the HostResolverImpl refactor, that adds the
> worker
> > task runner as an injectable dependency.
> > 
> > After reading this again though, I don't find that as offensive, and maybe
> even
> > like the abstraction...
> > 
> > I suppose my remaining concerns are:
> > 
> >   * It makes it less clear in HostResolverImpl that things are running in a
> > worker pool (which has some extra considerations for code running on it).
Meh,
> > good enough documentation.
> > 
> >   * Would it be stronger for the fuzzer to be exercising the same code as
> > production? (i.e. worker pool). Yes there are going to be more difficult
> > ordering concerns, which I appreciate you tried to make the runs
deterministic
> > based on fuzz input. I think we could achieve something similar by
> instrumenting
> > the resolveproc and serializing completion order  at that level -- although
to
> > avoid racing which resolve proc posts back to network thread we would
> > additionally need to wait for request completions are observed. Which is
both
> > ugly and also misses some coverage of threading interactions.
> 
> I think this just gets a little too ugly.  When we start a job, we'd also have
> to wait on it calling the HostResolverProc.  Perhaps I've been working on
Chrome
> too long, but all that locking and event handling makes me nervous.  We'd also
> have to make sure we complete all jobs before the test completes.  I think
> having to spin the message loop each time is about as bad as not using a
> WorkerPool, in terms of lost test coverage, and I'm reluctant to adding yet
more
> complexity to these tests, particularly CL that is already 1,000 lines long. 
It
> may be worth doing in a followup, but I'm moving away from fuzzers now.  I'd
> really like to get HTTP2 working in the URLRequest fuzzer, so may still do
that,
> but given how few issues we've unearthed so far, I don't expect to do much
else
> there for a while, unless our current ones start turning up more bugs.
> 
> I had another idea - we could make our own MessageLoop class.  Is has a normal
> message queue, but also has a bogus worker pool.  Whenever it runs a task, it
> would randomly pick between the next task on the normal message queue, or one
of
> the tasks posted to the worker pool.  While we wouldn't be using a real worker
> pool, it would get us pretty much everything else, I believe.  We could even
> make it mock out time passing for calls to PostDelayedTask, if we really
wanted,
> though we'd need to be careful about eating too many random bits.
> 
> > I dunno, thinking out loud, but am curious on your thoughts.

And I'll plan to respond to your comments and upload + land later this week.

[mmenke, 2016-06-01 21:21:51]

Thanks!

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
File net/base/fuzzed_data_provider.h (right):

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:58: template <typename Type, int size>
On 2016/06/01 01:47:21, eroman wrote:
> nit: size_t -- Although I can't imagine it making a difference in practice.

Done.

https://codereview.chromium.org/1946793002/diff/420001/net/base/fuzzed_data_p...
net/base/fuzzed_data_provider.h:59: Type PickArrayEntry(Type (&array)[size]) {
On 2016/06/01 01:47:21, eroman wrote:
> On 2016/05/19 19:09:46, mmenke wrote:
> > Open to other names here.  "ConsumeArrayEntry" or "ConsumeValueFromArray"
both
> > sound like the array is being modified.  Also, is this what you had in mind,
> or
> > were you thinking of the (simpler, more reedable) "PickArrayEntry(Type*
array,
> > size_t size)"
> 
> Yep this is what I was thinking.
> 
> Current name is fine by me.
> 
> Another idea is PickValueFromArray()

I like the name much better...  Actually, though, I like PickValueInArray even
better, so I went with that.  It matches the "ConsumeBlahInRange" methods a bit
more closely.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/dns_socket_poo...
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/dns_socket_poo...
net/dns/dns_socket_pool.cc:196: unsigned socket_index =
rand_int_callback().Run(0, pool.size() - 1);
On 2016/06/01 01:47:21, eroman wrote:
> Rather than exposing the callback , how about exposing a function that does
> this.
> 
> Say, GetRandomInt(.., ...)
> 
> (the callback is more of an implementation detail of the base class).

Done.  Agree it's a bit nicer, though since we're passing it to the base class
in our constructor, not exactly a hidden implementation detail.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:34: // Port 0 is magic.
On 2016/06/01 01:47:21, eroman wrote:
> but shouldn't be

It's a reserved port, so presumably it should be?  This test may work if I
allowed port 0, but connecting to port 0 seemed too weird, hence this line.

INADDR_ANY is also generally defined as 0, I believe - admittedly, it's only for
creating listen sockets, but still makes "port 0" freak me out a bit.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:103: uint16_t num_ipv6_addresses =
data_provider_->ConsumeUint8();
On 2016/06/01 01:47:21, eroman wrote:
> nit: any reason to use uint16 here (rather than uint8, or a size_t) ?

I initially used uint16_t for number of addresses, and forgot to update it when
I switched the range.  I've gone ahead and switch to size_t, but I think uint8_t
works just as well.  size_t just indicates it's related to a memory allocation a
bit more clearly, though that's not exactly ambiguous here.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:112: uint16_t num_ipv4_addresses =
data_provider_->ConsumeUint8();
On 2016/06/01 01:47:21, eroman wrote:
> ditto

Done.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:119: done = true;
On 2016/06/01 01:47:21, eroman wrote:
> is fallthrough intentional?

Eeee...No, it wasn't.  Thanks for catching that!

[mmenke, 2016-06-01 21:28:47]

Oops, missed the most important one.

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:718: // cannot wait for it to finish, so this code
must ve very careful about using
On 2016/06/01 01:47:21, eroman wrote:
> ve --> be

Done.

[eroman, 2016-06-01 21:46:06]

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:34: // Port 0 is magic.
On 2016/06/01 21:21:51, mmenke wrote:
> On 2016/06/01 01:47:21, eroman wrote:
> > but shouldn't be
> 
> It's a reserved port, so presumably it should be?  This test may work if I
> allowed port 0, but connecting to port 0 seemed too weird, hence this line.
> 
> INADDR_ANY is also generally defined as 0, I believe - admittedly, it's only
for
> creating listen sockets, but still makes "port 0" freak me out a bit.

Sorry I should have been clearer.

My comment was about layering -- from the perspective of DnsConfig, is it
expected that port cannot be 0 for a nameserver? If it doesn't care, then should
probably just feed in 0 as a possibility and let it do its magic.

Looking at the windows implementation of dnsconfigservice at least, doesn't look
like it will allow port 0.

Yes, I agree port 0 can be treated in magic ways by APIs, which is why testing
for it can be useful.

That said, I don't think fuzzing or not fuzzing the port for this input is going
to be that interesting, since it won't be doing much with it anyway.

[mmenke, 2016-06-01 21:53:29]

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:34: // Port 0 is magic.
On 2016/06/01 21:46:06, eroman wrote:
> On 2016/06/01 21:21:51, mmenke wrote:
> > On 2016/06/01 01:47:21, eroman wrote:
> > > but shouldn't be
> > 
> > It's a reserved port, so presumably it should be?  This test may work if I
> > allowed port 0, but connecting to port 0 seemed too weird, hence this line.
> > 
> > INADDR_ANY is also generally defined as 0, I believe - admittedly, it's only
> for
> > creating listen sockets, but still makes "port 0" freak me out a bit.
> 
> Sorry I should have been clearer.
> 
> My comment was about layering -- from the perspective of DnsConfig, is it
> expected that port cannot be 0 for a nameserver? If it doesn't care, then
should
> probably just feed in 0 as a possibility and let it do its magic.
> 
> Looking at the windows implementation of dnsconfigservice at least, doesn't
look
> like it will allow port 0.
> 
> Yes, I agree port 0 can be treated in magic ways by APIs, which is why testing
> for it can be useful.
> 
> That said, I don't think fuzzing or not fuzzing the port for this input is
going
> to be that interesting, since it won't be doing much with it anyway.

I've removed the check - I think it was inspired by the belief that
DnsConfigService can't return ports of 0, though perhaps I was wrong - and even
if not, we probably shouldn't fail badly if that happens.  If the bots run into
issues with port 0, can reconsider it.

[mmenke, 2016-06-01 21:54:26]

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
File net/dns/fuzzed_host_resolver.cc (right):

https://codereview.chromium.org/1946793002/diff/420001/net/dns/fuzzed_host_re...
net/dns/fuzzed_host_resolver.cc:34: // Port 0 is magic.
On 2016/06/01 21:53:28, mmenke wrote:
> On 2016/06/01 21:46:06, eroman wrote:
> > On 2016/06/01 21:21:51, mmenke wrote:
> > > On 2016/06/01 01:47:21, eroman wrote:
> > > > but shouldn't be
> > > 
> > > It's a reserved port, so presumably it should be?  This test may work if I
> > > allowed port 0, but connecting to port 0 seemed too weird, hence this
line.
> > > 
> > > INADDR_ANY is also generally defined as 0, I believe - admittedly, it's
only
> > for
> > > creating listen sockets, but still makes "port 0" freak me out a bit.
> > 
> > Sorry I should have been clearer.
> > 
> > My comment was about layering -- from the perspective of DnsConfig, is it
> > expected that port cannot be 0 for a nameserver? If it doesn't care, then
> should
> > probably just feed in 0 as a possibility and let it do its magic.
> > 
> > Looking at the windows implementation of dnsconfigservice at least, doesn't
> look
> > like it will allow port 0.
> > 
> > Yes, I agree port 0 can be treated in magic ways by APIs, which is why
testing
> > for it can be useful.
> > 
> > That said, I don't think fuzzing or not fuzzing the port for this input is
> going
> > to be that interesting, since it won't be doing much with it anyway.
> 
> I've removed the check - I think it was inspired by the belief that
> DnsConfigService can't return ports of 0, though perhaps I was wrong - and
even
> if not, we probably shouldn't fail badly if that happens.  If the bots run
into
> issues with port 0, can reconsider it.

Actually, it was inspired by the belief that *connections* to port 0 should
fail, so it seemed weird that our mock classes would allow them.  Anyhow, I'll
still remove the check.

[mmenke, 2016-06-01 21:55:47]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

This fuzzes it using both the platform resolver (Which isn't too
exciting), and the built-in DNS resolver, complete with fuzzed UDP
sockets.

This introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

[mmenke, 2016-06-01 21:55:58]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-06-01 21:55:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1946793002/#ps480001
(title: "Remove port 0 check")

[commit-bot: I haz the power, 2016-06-01 21:56:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1946793002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1946793002/480001

[commit-bot: I haz the power, 2016-06-02 00:27:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 00:27:51]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-06-02 02:24:53]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-06-02 02:25:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1946793002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1946793002/480001

[commit-bot: I haz the power, 2016-06-02 03:11:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 03:11:54]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-06-02 14:50:27]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-06-02 14:51:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1946793002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1946793002/480001

[commit-bot: I haz the power, 2016-06-02 16:03:36]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

[commit-bot: I haz the power, 2016-06-02 16:03:37]

Committed patchset #19 (id:480001)

[commit-bot: I haz the power, 2016-06-02 16:07:48]

Description was changed from

==========
net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005
==========

to

==========
net: Add fuzzer for HostResolverImpl.

The fuzzer tests it using both a mocked out platform resolver (Which
isn't too exciting), and the built-in DNS resolver, complete with
fuzzed UDP sockets.

This CL introduces a FuzzedHostResolver class that can also be included
as a component for other fuzzers.

Also makes dns_socket_pool.cc less chatty on errors.

BUG=600005

Committed: https://crrev.com/91c1716731622e720e91dc052aaee9961a009423
Cr-Commit-Position: refs/heads/master@{#397429}
==========

[commit-bot: I haz the power, 2016-06-02 16:07:49]

Patchset 19 (id:??) landed as
https://crrev.com/91c1716731622e720e91dc052aaee9961a009423
Cr-Commit-Position: refs/heads/master@{#397429}