[net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous DNS ready for experiments.

[net] Asynchronous DNS ready for experiments.

If started with --enable-async-dns, HostResolverImpl will use
DnsConfigService to determine system DNS configuration and
DnsTransaction to resolve host names. It will fallback to
HostResolverProc on failure.

BUG=90881, 107880, 113829
TEST=./net_unittests --gtest_filter=HostResolverImpl*:Dns*


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

[szym, 2012-02-09 22:52:56]

Everything put together. This is the first draft. 

Job limits on ProcTask (after DnsTask fails) are not enforced right now.

[cbentzel, 2012-02-10 01:26:14]

Cool. By "first draft" are you interested in getting some design feedback,
a full review, or is this just an FYI?

On Thu, Feb 9, 2012 at 5:52 PM, <szym@chromium.org> wrote:

> Reviewers: cbentzel, Matt Menke,
>
> Message:
> Everything put together. This is the first draft.
>
> Job limits on ProcTask (after DnsTask fails) are not enforced right now.
>
>
> Description:
> [net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous
> DNS
> ready for experiments.
>
> BUG=90881, 107880
> TEST=
>
>
> Please review this at
http://codereview.chromium.**org/9369045/<http://codereview.chromium.org/9369...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/io_thread.cc
>  M chrome/common/chrome_switches.**cc
>  M net/base/address_list.h
>  M net/base/address_list.cc
>  M net/base/address_list_**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/dns/async_host_resolver.cc
>  M net/dns/dns_config_service.h
>  M net/dns/dns_config_service_**posix.cc
>  M net/dns/dns_config_service_**win.cc
>  M net/dns/dns_response.h
>  M net/dns/dns_response.cc
>  M net/dns/dns_response_unittest.**cc
>  M net/dns/dns_transaction.cc
>
>
>

[szym, 2012-02-10 02:04:18]

So far it compiles and passes the old unit tests, but I have not yet
tested the code, so there might be bugs in it.

Still missing is the code that will dispatch ProcTask using the inner
PrioritizedDispatcher, but more importantly the
DnsConfigService::Observer interface is candidate for redesign. (If
HostResolverImpl owns the service, perhaps a Callback would be more
appropriate.)

Otherwise, I'll be going with this general design, so if you have any
high level comments, they'll be much appreciated.

On Thu, Feb 9, 2012 at 8:26 PM, Chris Bentzel <cbentzel@chromium.org> wrote:
> Cool. By "first draft" are you interested in getting some design feedback, a
> full review, or is this just an FYI?
>
>
> On Thu, Feb 9, 2012 at 5:52 PM, <szym@chromium.org> wrote:
>>
>> Reviewers: cbentzel, Matt Menke,
>>
>> Message:
>> Everything put together. This is the first draft.
>>
>> Job limits on ProcTask (after DnsTask fails) are not enforced right now.
>>
>>
>> Description:
>> [net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous
>> DNS
>> ready for experiments.
>>
>> BUG=90881, 107880
>> TEST=
>>
>>
>> Please review this at http://codereview.chromium.org/9369045/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M chrome/browser/io_thread.cc
>>  M chrome/common/chrome_switches.cc
>>  M net/base/address_list.h
>>  M net/base/address_list.cc
>>  M net/base/address_list_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/dns/async_host_resolver.cc
>>  M net/dns/dns_config_service.h
>>  M net/dns/dns_config_service_posix.cc
>>  M net/dns/dns_config_service_win.cc
>>  M net/dns/dns_response.h
>>  M net/dns/dns_response.cc
>>  M net/dns/dns_response_unittest.cc
>>  M net/dns/dns_transaction.cc
>>
>>
>

[szym, 2012-02-10 18:11:47]

If you're curious, this is now ready for test-drive. Still need to write the
missing tests and ProcTask limiting.

[cbentzel, 2012-02-10 19:51:08]

A few initial nits, going through the meat of it now.

http://codereview.chromium.org/9369045/diff/4001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/9369045/diff/4001/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:139: if
(command_line.HasSwitch(switches::kDnsServer)) {
I'd be fine with getting rid of this for now.

http://codereview.chromium.org/9369045/diff/10001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/9369045/diff/10001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:423: const char kEnableAsyncDns[]              
 = "enable-async-dns";
Nit: Should be ordered after EnableAeroPeekTabs.

http://codereview.chromium.org/9369045/diff/10001/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/address_list.cc#ne...
net/base/address_list.cc:115: const std::string& canonical_name) {
Why were you able to get rid of port here? Was it only used in the unit test?

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver.h#ne...
net/base/host_resolver.h:195: size_t max_retry_attempts,
Nit: document max_retry_attempts and max_concurrent_resolves like above, or at
least reference the CreateSystemHostResolver documentation.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:470: true /* use_cache */,
I don't like these comments - would rather have a bit flag enum or something
instead of the comments to describe the booleans.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:67: public DnsConfigService::Observer,
NON_EXPORTED_BASE needed.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:205: const AddressList& addrlist,
Should ttl be part of the AddressList?

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:215: // NetworkChangeNotifier::IPAddressObserver
interface.
Nit: the examples on http://dev.chromium.org/developers/coding-style show

// NetworkChangeNotifier::IPAddressObserver:

http://codereview.chromium.org/9369045/diff/10001/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:374: // NOTE: This is here only to compile, but
async_host_resolver* is going
Is this note still correct?

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:244: // TODO(szym): Test
DnsResponseToAddressList
Yes, please do.

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:81: dict->Set("source_dependency",
source_.ToValue());
Is this going to impact backwards compatibility of the JSON format?

[szym, 2012-02-10 21:49:35]

Thanks for the comments. I'll now move some tests to dns_response_unittest.cc
and also switch to the ProcTask dispatcher.

http://codereview.chromium.org/9369045/diff/10001/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/address_list.cc#ne...
net/base/address_list.cc:115: const std::string& canonical_name) {
On 2012/02/10 19:51:08, cbentzel wrote:
> Why were you able to get rid of port here? Was it only used in the unit test?

It was only used in AsyncHostResolver, and the usecase was good. However, in
HostResolverImpl, there's an equally efficient MutableSetPort that was already
used for ProcTask, so I prefer that one for simplicity of code paths.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:470: true /* use_cache */,
On 2012/02/10 19:51:08, cbentzel wrote:
> I don't like these comments - would rather have a bit flag enum or something
> instead of the comments to describe the booleans.

Not a fan of bit flag or enum in this case. If we ever introduce more than 2
choices for a setting, it will blow things up. I'm open to having a params
struct not unlike elsewhere in net/ (HttpNetworkSession::Params, for instance).

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:67: public DnsConfigService::Observer,
On 2012/02/10 19:51:08, cbentzel wrote:
> NON_EXPORTED_BASE needed. 

In the long run, I don't think we should be keeping the Observer pattern given
that HostResolverImpl owns the DnsConfigService.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:205: const AddressList& addrlist,
On 2012/02/10 19:51:08, cbentzel wrote:
> Should ttl be part of the AddressList?

An excellent question. Makes sense for HostCache and DnsTask, but not quite for
HostResolverProc and getaddrinfo. (There's no explicit permission in POSIX to
cache the results of getaddrinfo.)

http://codereview.chromium.org/9369045/diff/10001/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:374: // NOTE: This is here only to compile, but
async_host_resolver* is going
On 2012/02/10 19:51:08, cbentzel wrote:
> Is this note still correct?

I haven't removed async_host_resolver* yet, but that would be part of this CL. 

The tests that are using the data in dns_test_util.h will be moved to
dns_response_unittest.cc

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/9369045/diff/10001/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:81: dict->Set("source_dependency",
source_.ToValue());
On 2012/02/10 19:51:08, cbentzel wrote:
> Is this going to impact backwards compatibility of the JSON format? 

These event parameters have been introduced recently http://crrev.com/119146 (2
weeks ago)
I added the "socket_source" parameter, but later learned that the automatic
linkage in chrome://net-internals will only work if the are named
"source_dependency".

[cbentzel, 2012-02-10 22:18:49]

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:67: public DnsConfigService::Observer,
On 2012/02/10 21:49:36, szym wrote:
> On 2012/02/10 19:51:08, cbentzel wrote:
> > NON_EXPORTED_BASE needed. 
> 
> In the long run, I don't think we should be keeping the Observer pattern given
> that HostResolverImpl owns the DnsConfigService.

Agree. But it doesn't compile without this on a component build.

[szym, 2012-02-11 00:01:37]

Dropped async_host_resolver*, and added tests for DnsResponse::ParseAddressList.

I'll look into the issues on Windows.

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/9369045/diff/10001/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:67: public DnsConfigService::Observer,
On 2012/02/10 22:18:49, cbentzel wrote:
> On 2012/02/10 21:49:36, szym wrote:
> > On 2012/02/10 19:51:08, cbentzel wrote:
> > > NON_EXPORTED_BASE needed. 
> > 
> > In the long run, I don't think we should be keeping the Observer pattern
given
> > that HostResolverImpl owns the DnsConfigService.
> 
> Agree. But it doesn't compile without this on a component build.

Oh yes, absolutely, that is fixed in the next patch.

[szym, 2012-02-13 20:16:26]

I think this is in a decent shape for a thorough review at this stage.

The issues on Win should be fixed now. Also added NetLog to DnsTask, although I
admit that logging is quite inconsistent right now, i.e., what should be its own
NetLog::Source and what should be merely an Event with Begin/End.

I'm holding back on the ProcTask dispatcher for now -- it's not critical until
the ProcTask dispatcher limits are tighter than the Job dispatcher's.

[szym, 2012-02-13 20:18:36]

I should not that the main outstanding issue which will cause performance and
potentially correctness loss with --enable-async-dns:
- HOSTS is ignored
- local computer name is ignored

[cbentzel, 2012-02-14 01:27:24]

On 2012/02/13 20:18:36, szym wrote:
> I should not that the main outstanding issue which will cause performance and
> potentially correctness loss with --enable-async-dns:
> - HOSTS is ignored
> - local computer name is ignored

Do you know why async_host_resolver.cc is labeled "M" instead of "D"? Is there a
single newline or something?

[cbentzel, 2012-02-14 02:01:54]

Haven't gotten to host_resolver_impl.cc, will look at that in the morning.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:451: base::ThreadRestrictions::ScopedAllowIO
allow_io;
Can you add a bug about whether we should try to remove this ScopedAllowIO case?
I think it's OK, but want to spend a little more time thinking about it.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:232: std::string expected_name = GetDottedName();
It looks like both DNSDomainToString and ParseName both omit trailing dot. Might
be worth mentioning since both need to be identical in behavior for this to work
due to the string comparison in the while loop.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:235: size_t expected_size = (qtype() ==
dns_protocol::kTypeAAAA)
Could use expected_type instead of qtype().

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:236: ? kIPv6AddressSize : kIPv4AddressSize;
Do you want to make sure the qtype is only one of AAAA or A? I don't remember if
this is enforced elsewhere.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:245: while (parser.ParseRecord(&record)) {
Looks like this would accept a response for an A query of alpha like

alpha CNAME beta
beta A 18.85.0.1
beta CNAME gamma
gamma A 18.85.0.2

CNAMEs should probably be rejected once we process a non-CNAME entry?

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:252: if (!parser.ParseName(record.rdata.begin(),
&expected_name)) {
Are these guaranteed to always be in order of the chain, or could they be
reordered and still be valid? 

i.e. would a query for alpha and a response like

alpha CNAME beta
gamma CNAME delta
beta CNAME gamma
delta A 18.85.0.1

be valid, if not common?

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
I thought TTL for all RRSet entries needed to be identical according to one of
the RFCs (will have to dig for which one). 

If you're going to be permissive, I'm OK, but may be worth calling out. Doing a
min seems correct.

[cbentzel, 2012-02-14 13:08:25]

My main concerns are some tear down bugs [always the gotcha] and the
proc_dispatcher_.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:326: PriorityTracker(RequestPriority priority)
explicit

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:327: : highest_priority_(priority),
total_count_(0) {
This was a little tricky - I expected total_count_ and counts_[priority] to both
be 1. Once I looked at how the AddRequest was made common for the
new-job/existing-job case it became clear why this was needed, but may benefit
from a note.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1101: // Spawns ProcTask when started.
The comment "Spawns ProcTask when started" is no longer completely accurate.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1130: proc_task_->Cancel();
BUG: This needs to deal with the case where is_running() is true but it's in the
middle of dns_task_

Perhaps share code with Abort().

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1179: // The Job will own |req| and destroy it in
~Job.
Nit: change this to take a scoped_ptr<Request> argument instead?

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1234: DCHECK(is_running());
Should this remove itself from the PrioritizedDispatcher if !handle_.is_null()

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1316: void OnProcTaskComplete(int net_error, int
os_error,
Nit: probably easier to read if this were moved directly under StartProcTask.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1323: base::TimeDelta ttl =
base::TimeDelta::FromSeconds(0);
Should there be a constant for negative cache TTL?

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1363: resolver_->OnJobFinished(this, addr_list,
ttl);
Does this need to be called after deleting self? Will this Job still be in the
PrioritizedDispatcher?

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1438: proc_dispatcher_(proc_limits),
Is proc_dispatcher_ even used? It looks like we just use dispatcher_ for all
scheduling.

It looks like there is a TODO for it. However, we'd need to add some state to
Job to know whether to do a DNS task or a Proc task. Also, we'd probably want to
go back up to HostResolverImpl if the DNS Job completes so it can be removed
from dispatcher_, and then a new Job added to prioritized_dispatcher_. This may
be a bit ugly because we'd need to transfer the requests over.

[cbentzel, 2012-02-14 13:12:06]

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:420: // TODO(szym): Add tighter limits for
proc_dispatcher_
Why do you need this TODO? In this case it will only be using the limits
specified for the regular dispatcher_, and these are the same limits as before.

Also - at one point I thought we had different queues for
speculative/non-speculative DNS queries. Should we do some reserved slots for
non-idle cases?

[szym, 2012-02-14 15:56:24]

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:420: // TODO(szym): Add tighter limits for
proc_dispatcher_
On 2012/02/14 13:12:06, cbentzel wrote:
> Why do you need this TODO? In this case it will only be using the limits
> specified for the regular dispatcher_, and these are the same limits as
before.

I need this TODO to remember that even if I start using proc_dispatcher_, it
will never queue a job unless the its limits are tighter than those of
dispatcher_. Right now proc_dispatcher_ is not used, and I needed to put some
limits in there. I'll change it to PrioritizedDispatcher::Limits(NUM_PRIORITIES,
0);
 
> Also - at one point I thought we had different queues for
> speculative/non-speculative DNS queries. Should we do some reserved slots for
> non-idle cases? 

Even before the big refactor of HostResolverImpl, the is_speculative flag wasn't
used to determine the priority of the Job. There was the JobPool facility, but
with only one instance, there really wasn't any reservation in place.

I think it might be time for an experiment with tiered limits. We'd need to
check the call-sites to Resolve to see if request priority is properly set.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1130: proc_task_->Cancel();
On 2012/02/14 13:08:26, cbentzel wrote:
> BUG: This needs to deal with the case where is_running() is true but it's in
the
> middle of dns_task_
> 
> Perhaps share code with Abort().

Good catch. Needs: "if (proc_task_.get())".

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1234: DCHECK(is_running());
On 2012/02/14 13:08:26, cbentzel wrote:
> Should this remove itself from the PrioritizedDispatcher if !handle_.is_null()

If is_running() then handle_ should be null. (A running Job cannot be queued.) I
will add a DCHECK for that.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1363: resolver_->OnJobFinished(this, addr_list,
ttl);
On 2012/02/14 13:08:26, cbentzel wrote:
> Does this need to be called after deleting self? Will this Job still be in the
> PrioritizedDispatcher?

I'm not clear what you mean. After deleting self I cannot access |resolver_|.
The critical ordering is that the Job is removed from |HostResolverImpl::jobs_|
before running any of the Request callbacks.

On second point: this Job cannot be in the dispatcher. CompleteRequests applies
only if the Job has already been started, which means it is out of the
dispatcher.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1438: proc_dispatcher_(proc_limits),
On 2012/02/14 13:08:26, cbentzel wrote:
> Is proc_dispatcher_ even used? It looks like we just use dispatcher_ for all
> scheduling.

Right now it is not. I'm okay removing it until we determine that we need
tighter limits for ProcTask than for DnsTask (which is expected, but we do not
yet have particular numbers in mind).

> It looks like there is a TODO for it. However, we'd need to add some state to
> Job to know whether to do a DNS task or a Proc task. Also, we'd probably want
to
> go back up to HostResolverImpl if the DNS Job completes so it can be removed
> from dispatcher_, and then a new Job added to prioritized_dispatcher_. 

Not needed. A Job that has a DnsTask cannot be in any dispatcher. I'll add some
comments to make it more apparent.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:245: while (parser.ParseRecord(&record)) {
On 2012/02/14 02:01:54, cbentzel wrote:
> Looks like this would accept a response for an A query of alpha like
> 
> alpha CNAME beta
> beta A 18.85.0.1
> beta CNAME gamma
> gamma A 18.85.0.2
> 
> CNAMEs should probably be rejected once we process a non-CNAME entry?

To answer both this and the next question:

I looked at evdns, glibc and c-ares. 

evdns:
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/libevent/evdns.c
Only checks names in question. Does not check names in answers.

c-ares:
http://code.google.com/codesearch#5jiG3hroW30/trunk/libcurl.mod/src/ares/ares...
Assumes CNAMEs are in an ordered chain.

glibc:
http://code.google.com/codesearch#PfbwIaGvWwI/trunk/glibc-2.14/resolv/nss_dns...
??? I still have not successfully parsed through this.

Either way, if we think that a response like this is invalid, should we reject
it outright (ERR_DNS_SERVER_FAILED?) or accept part of it? I'm leaning towards
the former.

I did notice a BUG. This comparison below should ignore case.

[szym, 2012-02-14 17:44:38]

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
On 2012/02/14 02:01:54, cbentzel wrote:
> I thought TTL for all RRSet entries needed to be identical according to one of
> the RFCs (will have to dig for which one). 
> 
> If you're going to be permissive, I'm OK, but may be worth calling out. Doing
a
> min seems correct.

Not clear on this one. C-ares does not bundle the TTLs together. Instead it
returns one TTL per address fetched.

[cbentzel, 2012-02-14 18:01:27]

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1363: resolver_->OnJobFinished(this, addr_list,
ttl);
On 2012/02/14 15:56:24, szym wrote:
> On 2012/02/14 13:08:26, cbentzel wrote:
> > Does this need to be called after deleting self? Will this Job still be in
the
> > PrioritizedDispatcher?
> 
> I'm not clear what you mean. After deleting self I cannot access |resolver_|.
> The critical ordering is that the Job is removed from
|HostResolverImpl::jobs_|
> before running any of the Request callbacks.
> 
> On second point: this Job cannot be in the dispatcher. CompleteRequests
applies
> only if the Job has already been started, which means it is out of the
> dispatcher.

It was the second point I was concerned about. Agree that it's out of the
dispatcher. If it did do after delete self, it would need to copy resolver_ to a
stack variable to prevent the use-after-free.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1438: proc_dispatcher_(proc_limits),
On 2012/02/14 15:56:24, szym wrote:
> On 2012/02/14 13:08:26, cbentzel wrote:
> > Is proc_dispatcher_ even used? It looks like we just use dispatcher_ for all
> > scheduling.
> 
> Right now it is not. I'm okay removing it until we determine that we need
> tighter limits for ProcTask than for DnsTask (which is expected, but we do not
> yet have particular numbers in mind).

I'd recommend removing it for now, as well as the additional set of limits
passed into HostResolverImpl.

> 
> > It looks like there is a TODO for it. However, we'd need to add some state
to
> > Job to know whether to do a DNS task or a Proc task. Also, we'd probably
want
> to
> > go back up to HostResolverImpl if the DNS Job completes so it can be removed
> > from dispatcher_, and then a new Job added to prioritized_dispatcher_. 
> 
> Not needed. A Job that has a DnsTask cannot be in any dispatcher. I'll add
some
> comments to make it more apparent.
>

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:245: while (parser.ParseRecord(&record)) {
On 2012/02/14 15:56:24, szym wrote:
> On 2012/02/14 02:01:54, cbentzel wrote:
> > Looks like this would accept a response for an A query of alpha like
> > 
> > alpha CNAME beta
> > beta A 18.85.0.1
> > beta CNAME gamma
> > gamma A 18.85.0.2
> > 
> > CNAMEs should probably be rejected once we process a non-CNAME entry?
> 
> To answer both this and the next question:
> 
> I looked at evdns, glibc and c-ares. 
> 
> evdns:
> http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/libevent/evdns.c
> Only checks names in question. Does not check names in answers.
> 
> c-ares:
>
http://code.google.com/codesearch#5jiG3hroW30/trunk/libcurl.mod/src/ares/ares...
> Assumes CNAMEs are in an ordered chain.
> 
> glibc:
>
http://code.google.com/codesearch#PfbwIaGvWwI/trunk/glibc-2.14/resolv/nss_dns...
> ??? I still have not successfully parsed through this.
> 
> Either way, if we think that a response like this is invalid, should we reject
> it outright (ERR_DNS_SERVER_FAILED?) or accept part of it? I'm leaning towards
> the former.

I think that makes sense - this is unlikely to be something done intentionally.
You could even add something like malformed DNS response for an error.
 
> 
> I did notice a BUG. This comparison below should ignore case.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
On 2012/02/14 17:44:39, szym wrote:
> On 2012/02/14 02:01:54, cbentzel wrote:
> > I thought TTL for all RRSet entries needed to be identical according to one
of
> > the RFCs (will have to dig for which one). 
> > 
> > If you're going to be permissive, I'm OK, but may be worth calling out.
Doing
> a
> > min seems correct.
> 
> Not clear on this one. C-ares does not bundle the TTLs together. Instead it
> returns one TTL per address fetched.

See http://tools.ietf.org/html/rfc2181#section-5 5.2

"Should a client receive a response containing RRs from an RRSet with differing
TTLs, it should treat this as an error."

It might be the case that this isn't actually obeyed in practice.

[szym, 2012-02-14 18:07:19]

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
On 2012/02/14 18:01:27, cbentzel wrote:
> On 2012/02/14 17:44:39, szym wrote:
> > On 2012/02/14 02:01:54, cbentzel wrote:
> > > I thought TTL for all RRSet entries needed to be identical according to
one
> of
> > > the RFCs (will have to dig for which one). 
> > > 
> > > If you're going to be permissive, I'm OK, but may be worth calling out.
> Doing
> > a
> > > min seems correct.
> > 
> > Not clear on this one. C-ares does not bundle the TTLs together. Instead it
> > returns one TTL per address fetched.
> 
> See http://tools.ietf.org/html/rfc2181#section-5 5.2
> 
> "Should a client receive a response containing RRs from an RRSet with
differing
> TTLs, it should treat this as an error."
> 
> It might be the case that this isn't actually obeyed in practice. 
> 

A good strategy could be to be strict initially, but have fine-grained
net_error. Then if the ProcTask succeeds when the DnsTask failed, we would
histogram the net_error returned from DnsTask. This would help us quantify how
often a presumably invalid response is accepted by the OS anyway.

[cbentzel, 2012-02-14 18:21:34]

On Tue, Feb 14, 2012 at 1:07 PM, <szym@chromium.org> wrote:

>
> http://codereview.chromium.**org/9369045/diff/17002/net/**
>
dns/dns_response.cc<http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc>
> File net/dns/dns_response.cc (right):
>
> http://codereview.chromium.**org/9369045/diff/17002/net/**
>
dns/dns_response.cc#newcode259<http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#newcode259>
> net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
> On 2012/02/14 18:01:27, cbentzel wrote:
>
>> On 2012/02/14 17:44:39, szym wrote:
>> > On 2012/02/14 02:01:54, cbentzel wrote:
>> > > I thought TTL for all RRSet entries needed to be identical
>>
> according to one
>
>> of
>> > > the RFCs (will have to dig for which one).
>> > >
>> > > If you're going to be permissive, I'm OK, but may be worth calling
>>
> out.
>
>> Doing
>> > a
>> > > min seems correct.
>> >
>> > Not clear on this one. C-ares does not bundle the TTLs together.
>>
> Instead it
>
>> > returns one TTL per address fetched.
>>
>
>  See
http://tools.ietf.org/html/**rfc2181#section-5<http://tools.ietf.org/html/rfc...
>>
>
>  "Should a client receive a response containing RRs from an RRSet with
>>
> differing
>
>> TTLs, it should treat this as an error."
>>
>
>  It might be the case that this isn't actually obeyed in practice.
>>
>
>
> A good strategy could be to be strict initially, but have fine-grained
> net_error. Then if the ProcTask succeeds when the DnsTask failed, we
> would histogram the net_error returned from DnsTask. This would help us
> quantify how often a presumably invalid response is accepted by the OS
> anyway.
>
> That is a great strategy.


>
http://codereview.chromium.**org/9369045/<http://codereview.chromium.org/9369...
>

[mmenke, 2012-02-14 18:46:00]

http://codereview.chromium.org/9369045/diff/17002/net/base/address_list_unitt...
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/address_list_unitt...
net/base/address_list_unittest.cc:314:
ParseIPLiteralToNumber(tests[i].ip_address, &ip_number);
nit:  While you're here, might want to ASSERT_TRUE the above line.

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:463: = DnsConfigService::CreateSystemService();
nit:  Put the equals sign on the previous line (Can't find a single instance
where we don't do that in net/, and only one in chrome/)

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
On 2012/02/14 18:07:20, szym wrote:
> On 2012/02/14 18:01:27, cbentzel wrote:
> > On 2012/02/14 17:44:39, szym wrote:
> > > On 2012/02/14 02:01:54, cbentzel wrote:
> > > > I thought TTL for all RRSet entries needed to be identical according to
> one
> > of
> > > > the RFCs (will have to dig for which one). 
> > > > 
> > > > If you're going to be permissive, I'm OK, but may be worth calling out.
> > Doing
> > > a
> > > > min seems correct.
> > > 
> > > Not clear on this one. C-ares does not bundle the TTLs together. Instead
it
> > > returns one TTL per address fetched.
> > 
> > See http://tools.ietf.org/html/rfc2181#section-5 5.2
> > 
> > "Should a client receive a response containing RRs from an RRSet with
> differing
> > TTLs, it should treat this as an error."
> > 
> > It might be the case that this isn't actually obeyed in practice. 
> > 
> 
> A good strategy could be to be strict initially, but have fine-grained
> net_error. Then if the ProcTask succeeds when the DnsTask failed, we would
> histogram the net_error returned from DnsTask. This would help us quantify how
> often a presumably invalid response is accepted by the OS anyway.

Agree that it would be great to have a number of network error codes for
different failures.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl);
On 2012/02/14 18:01:27, cbentzel wrote:
> On 2012/02/14 17:44:39, szym wrote:
> > On 2012/02/14 02:01:54, cbentzel wrote:
> > > I thought TTL for all RRSet entries needed to be identical according to
one
> of
> > > the RFCs (will have to dig for which one). 
> > > 
> > > If you're going to be permissive, I'm OK, but may be worth calling out.
> Doing
> > a
> > > min seems correct.
> > 
> > Not clear on this one. C-ares does not bundle the TTLs together. Instead it
> > returns one TTL per address fetched.
> 
> See http://tools.ietf.org/html/rfc2181#section-5 5.2
> 
> "Should a client receive a response containing RRs from an RRSet with
differing
> TTLs, it should treat this as an error."
> 
> It might be the case that this isn't actually obeyed in practice. 
> 

I believe I saw a discussion somewhere (Maybe in a later RFC) athat had
suggestions on how to handle differing TTLs.  I believe it was either pick the
shortest or pick the first, can't remember which.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_response.h#newc...
net/dns/dns_response.h:122: bool ParseAddressList(AddressList* addr_list,
base::TimeDelta* ttl) const;
nit:  Suggest you rename this function to ParseAddresses or AsAddressList or
CreateAddressList or something.  Want to make it clearer from its name that it
is not parsing the AddressList that's an argument, but it's parsing its contents
as an address.

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:308: virtual const BoundNetLog& net_log() const {
nit:  OVERRIDE

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_transaction.h
File net/dns/dns_transaction.h (right):

http://codereview.chromium.org/9369045/diff/17002/net/dns/dns_transaction.h#n...
net/dns/dns_transaction.h:42: // Returns the net_log for this transaction.
nit:  "BoundNetLog".  |net_log| is a private variable name not defined in this
file.

[szym, 2012-02-14 23:53:51]

I have flattened the NetLog a bit by merging HOST_RESOLVER_IMPL_PROC_TASK into
HOST_RESOLVER_IMPL_JOB. I was planning to do the same for DNS_TRANSACTION.
However, I noticed that I often get:

(P) t=1329262677630 [st=0] +HOST_RESOLVER_IMPL_JOB  [dt=1]
                            --> host = "www.gstatic.com"
                            --> source_dependency = 119
(HOST_RESOLVER_IMPL_REQUEST)
(P) t=1329262677630 [st=0]    HOST_RESOLVER_IMPL_JOB_STARTED
(P) t=1329262677630 [st=0]   +HOST_RESOLVER_IMPL_DNS_TASK  [dt=1]
                              --> source_dependency = 121 (DNS_TRANSACTION)
(P) t=1329262677630 [st=0]      HOST_RESOLVER_IMPL_JOB_REQUEST_ATTACH
                                --> priority = 3
                                --> source_dependency = 119
(HOST_RESOLVER_IMPL_REQUEST)
(P) t=1329262677631 [st=1]   -HOST_RESOLVER_IMPL_DNS_TASK
                              --> address_list = ["74.125.226.191:80"]
(P) t=1329262677631 [st=1] -HOST_RESOLVER_IMPL_JOB

This is a bit misleading because it suggests that REQUEST_ATTACH happened to
DNS_TASK when in reality it happened to JOB. For this reason I removed the
CANCELLED event from ProcTask::Cancel() so that it is not confused with the Job
being cancelled.

[szym, 2012-02-15 00:07:29]

On 2012/02/14 23:53:51, szym wrote:
> I have flattened the NetLog a bit by merging HOST_RESOLVER_IMPL_PROC_TASK into
> HOST_RESOLVER_IMPL_JOB. I was planning to do the same for DNS_TRANSACTION.
> However, I noticed that I often get:
> 
> (P) t=1329262677630 [st=0] +HOST_RESOLVER_IMPL_JOB  [dt=1]
>                             --> host = "www.gstatic.com"
>                             --> source_dependency = 119
> (HOST_RESOLVER_IMPL_REQUEST)
> (P) t=1329262677630 [st=0]    HOST_RESOLVER_IMPL_JOB_STARTED
> (P) t=1329262677630 [st=0]   +HOST_RESOLVER_IMPL_DNS_TASK  [dt=1]
>                               --> source_dependency = 121 (DNS_TRANSACTION)
> (P) t=1329262677630 [st=0]      HOST_RESOLVER_IMPL_JOB_REQUEST_ATTACH
>                                 --> priority = 3
>                                 --> source_dependency = 119
> (HOST_RESOLVER_IMPL_REQUEST)
> (P) t=1329262677631 [st=1]   -HOST_RESOLVER_IMPL_DNS_TASK
>                               --> address_list = ["74.125.226.191:80"]
> (P) t=1329262677631 [st=1] -HOST_RESOLVER_IMPL_JOB
> 
> This is a bit misleading because it suggests that REQUEST_ATTACH happened to
> DNS_TASK when in reality it happened to JOB. For this reason I removed the
> CANCELLED event from ProcTask::Cancel() so that it is not confused with the
Job
> being cancelled.

I also think that the benefit of having an independent
HOST_RESOLVER_IMPL_PROC_TASK source was that it stood out in the NetLog (clearly
suggesting that the DnsTransaction failed). Is there a way to search for the
PROC_TASK event even if it does not have its own source?

[mmenke, 2012-02-15 00:17:19]

On 2012/02/15 00:07:29, szym wrote:
> On 2012/02/14 23:53:51, szym wrote:
> > I have flattened the NetLog a bit by merging HOST_RESOLVER_IMPL_PROC_TASK
into
> > HOST_RESOLVER_IMPL_JOB. I was planning to do the same for DNS_TRANSACTION.
> > However, I noticed that I often get:
> > 
> > (P) t=1329262677630 [st=0] +HOST_RESOLVER_IMPL_JOB  [dt=1]
> >                             --> host = "www.gstatic.com"
> >                             --> source_dependency = 119
> > (HOST_RESOLVER_IMPL_REQUEST)
> > (P) t=1329262677630 [st=0]    HOST_RESOLVER_IMPL_JOB_STARTED
> > (P) t=1329262677630 [st=0]   +HOST_RESOLVER_IMPL_DNS_TASK  [dt=1]
> >                               --> source_dependency = 121 (DNS_TRANSACTION)
> > (P) t=1329262677630 [st=0]      HOST_RESOLVER_IMPL_JOB_REQUEST_ATTACH
> >                                 --> priority = 3
> >                                 --> source_dependency = 119
> > (HOST_RESOLVER_IMPL_REQUEST)
> > (P) t=1329262677631 [st=1]   -HOST_RESOLVER_IMPL_DNS_TASK
> >                               --> address_list = ["74.125.226.191:80"]
> > (P) t=1329262677631 [st=1] -HOST_RESOLVER_IMPL_JOB
> > 
> > This is a bit misleading because it suggests that REQUEST_ATTACH happened to
> > DNS_TASK when in reality it happened to JOB. For this reason I removed the
> > CANCELLED event from ProcTask::Cancel() so that it is not confused with the
> Job
> > being cancelled.
> 
> I also think that the benefit of having an independent
> HOST_RESOLVER_IMPL_PROC_TASK source was that it stood out in the NetLog
(clearly
> suggesting that the DnsTransaction failed). Is there a way to search for the
> PROC_TASK event even if it does not have its own source?

Unfortunately, I had to remove that ability when I added the source_dependency
links.  We used to convert the entire thing to the same string for display and
for search.  With the links, we now have to render them as HTML, and to avoid
having 3 different versions of the code, I removed the old search code, so now
it just searches the JSON representation of the log.

May look into restoring this ability at some point, but it's not a high
priority.

That having been said, it wouldn't be hard to give them a different background
color based on the event, or something along those lines.  WDYT?

[szym, 2012-02-15 16:15:31]

> > I also think that the benefit of having an independent
> > HOST_RESOLVER_IMPL_PROC_TASK source was that it stood out in the NetLog
> (clearly
> > suggesting that the DnsTransaction failed). Is there a way to search for the
> > PROC_TASK event even if it does not have its own source?
> 
> That having been said, it wouldn't be hard to give them a different background
> color based on the event, or something along those lines.  WDYT?

Of the two, I think search would be more useful. Either way, do you suggest I
revert back to separate source for PROC_TASK, or should I merge DNS_TRANSACTION
in as well?

[mmenke1, 2012-02-15 16:31:18]

On 2012/02/15 16:15:31, szym wrote:
> > > I also think that the benefit of having an independent
> > > HOST_RESOLVER_IMPL_PROC_TASK source was that it stood out in the NetLog
> > (clearly
> > > suggesting that the DnsTransaction failed). Is there a way to search for
the
> > > PROC_TASK event even if it does not have its own source?
> > 
> > That having been said, it wouldn't be hard to give them a different
background
> > color based on the event, or something along those lines.  WDYT?
> 
> Of the two, I think search would be more useful. Either way, do you suggest I
> revert back to separate source for PROC_TASK, or should I merge
DNS_TRANSACTION
> in as well?

Think it would be easier to see just what is going on with respect to a single
Job if we merged the DNS_TRANSACTION in, too.  Searching for
"type:HOST_RESOLVER_IMPL_JOB attempt_number" will give you the ones that used a
PROC_TASK, though it would be great to have something more intuitive.  Full text
search would help, but even that's not great, given how much junk there is to
sort through.

I think we should merge them.  Searching for "attempt_number" can locate the

[mmenke1, 2012-02-15 16:32:10]

On 2012/02/15 16:31:18, mmenke wrote:
> On 2012/02/15 16:15:31, szym wrote:
> > > > I also think that the benefit of having an independent
> > > > HOST_RESOLVER_IMPL_PROC_TASK source was that it stood out in the NetLog
> > > (clearly
> > > > suggesting that the DnsTransaction failed). Is there a way to search for
> the
> > > > PROC_TASK event even if it does not have its own source?
> > > 
> > > That having been said, it wouldn't be hard to give them a different
> background
> > > color based on the event, or something along those lines.  WDYT?
> > 
> > Of the two, I think search would be more useful. Either way, do you suggest
I
> > revert back to separate source for PROC_TASK, or should I merge
> DNS_TRANSACTION
> > in as well?
> 
> Think it would be easier to see just what is going on with respect to a single
> Job if we merged the DNS_TRANSACTION in, too.  Searching for
> "type:HOST_RESOLVER_IMPL_JOB attempt_number" will give you the ones that used
a
> PROC_TASK, though it would be great to have something more intuitive.  Full
text
> search would help, but even that's not great, given how much junk there is to
> sort through.
> 
> I think we should merge them.  Searching for "attempt_number" can locate the

Err...oops.  Ignore that last fragment.  Forgot to delete it.

[szym, 2012-02-15 17:49:01]

Ready to land as is.

[szym, 2012-02-15 18:13:38]

On 2012/02/15 17:49:01, szym wrote:
> Ready to land as is.

Argh, I need to re-do the last patch. It didn't merge in.

[cbentzel, 2012-02-15 19:48:35]

Most of this is nits/style. There are two points I want addressed before giving
an L G T M:

  - Make sure the OnDnsChanged case is handled correctly.
  - More tests for ParseAddressList.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:393: class DispatcherHandle {
Is this needed as much anymore, since you're restricted to only one dispatcher
in the current implementation? I'm not totally opposed to keeping it, but it
feels a bit extraneous.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:482: config_service->Watch();
Should Watch be called by HostResolverImpl, so it can attach itself as an
observer first?

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1359: net_error_ = OK;
Are net_error_ and os_error_ needed as members, or could they be passed in as
arguments to CompleteRequests?

There's one little bit of funkiness where ERR_IO_PENDING is used both to
indicate that the Job hasn't been dispatched yet, and that it is actively doing
a DnsTask/ProcTask. Perhaps that could be better served with an explicit state.

If you'd like to table that to another cleanup CL, that would also be fine -
this one already has a lot of stuff.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1484: if (dns_config_service_.get())
Maybe want a note here that |this| is never removed as an observer, since it
destroys dns_config_service_ in the destructor. Or perhaps this is obvious.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1772: for (JobMap::iterator it = jobs_.begin();
it != jobs_.end(); ) {
BUG: I don't think this works correctly. The problem is that it will potentially
activate start a new Job running that doesn't needed to be aborted - for
example, it might have the new DnsTransactionFactory after OnDnsChanged. I think
you want to accumulate the jobs to abort in one loop in a different data
structure, and then abort them.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1833: if (had_factory) {
Nit: braces not needed.

Also - would it be easier to just call OnDNSChanged each time? It seems like it
would be a no-op if there wasn't a factory before, so would simplify the logic a
tiny bit.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:253: if (record.type == expected_type)
Are there any cases where DNS servers will append RRs of related domains to
prevent a round trip? I suppose we'll see the DNS_NAME_MISMATCH then, but might
be something to support in the future.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:255: continue;
Why do you want to continue in this case?

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:257: if ((record.type == dns_protocol::kTypeCNAME)) {
Nit: double parents not needed.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:259: if (!ip_addresses.empty()) {
Nit: extra braces

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:279: canon_name = record.name;
Shouldn't the canon_name always be the same? Should we check for that?

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:309: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(cases); ++i) {
You should add tests for each of the unsuccessful ParseToAddressList results.

[mmenke, 2012-02-15 19:54:16]

Just nits from me.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:391: // to update it when cancelling or changing
the priority.
nit: canceling.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1080: DnsResponse::Result result =
DnsResponse::DNS_SUCCESS;
Fine with this as-is, but does seem a little weird to explicitly log DNS_SUCCESS
when net_error isn't OK.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1088: callback_.Run(net_error, addr_list, ttl);
Suggest you add a comment that we've been deleted here.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1094:
net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
Suggest you move this line up, and delete |this| in the callback, just like the
other case.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1339: if (rv != ERR_IO_PENDING) {
Maybe reset |dns_task| here, and maybe DCHECK it's NULL when ProcTask starts?

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1629: // request. So it is safe to Abort it if it
has no more active requests.
I find this comment confusing.

Maybe something like:

If this were called from Request's callback in  Job::CompleteRequests(), that
Request would not have been cancelled, so job->num_active_requests() would not
be 0.  Therefore, we weren't called from Job::CompleteRequests().

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1710: const AddressList& addrlist,
At some point (Not necessarily in this CL), you might want to switch all of
these to addr_list, for consistency.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:312: virtual const BoundNetLog& net_log() const {
nit:  OVERRIDE

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_transaction.h
File net/dns/dns_transaction.h (right):

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_transaction.h#n...
net/dns/dns_transaction.h:43: virtual const BoundNetLog& net_log() const = 0;
Since this is virtual, should be called something like GetNetLog.

[szym, 2012-02-15 20:28:05]

I'll remove the DispatcherHandle, but I'd rather keep the logic of updating the
Handle within Job as it is now. This will make the coupling between Job and
HostResolverImpl tighter (need ->dispatcher_) but I think that'll make the core
of HostResolverImpl a bit cleaner.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:391: // to update it when cancelling or changing
the priority.
On 2012/02/15 19:54:16, Matt Menke wrote:
> nit: canceling.

I think the consensus was that it's okay to use non-US English in comments :P

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:393: class DispatcherHandle {
On 2012/02/15 19:48:35, cbentzel wrote:
> Is this needed as much anymore, since you're restricted to only one dispatcher
> in the current implementation? I'm not totally opposed to keeping it, but it
> feels a bit extraneous.

I prefer this over having to remember to set handle_ to Handle(). I'll probably
just move it to net/base/prioritized_dispatcher.h. I'm actually thinking about
replacing PrioritizedDispatcher::Handle with this, although storing the pointer
to |dispatcher_| with each handle is extraneous in case there's only one
dispatcher.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:482: config_service->Watch();
On 2012/02/15 19:48:35, cbentzel wrote:
> Should Watch be called by HostResolverImpl, so it can attach itself as an
> observer first?

I'll move it although it doesn't matter: DnsConfigService::AddObserver will call
the callback if config is already available. Also, the plan is to replace
DnsConfigService::Observer with NetworkChangeNotifier.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1080: DnsResponse::Result result =
DnsResponse::DNS_SUCCESS;
On 2012/02/15 19:54:16, Matt Menke wrote:
> Fine with this as-is, but does seem a little weird to explicitly log
DNS_SUCCESS
> when net_error isn't OK.

It is equivalent to setting os_error = 0 if we don't have an os_error.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1094:
net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
On 2012/02/15 19:54:16, Matt Menke wrote:
> Suggest you move this line up, and delete |this| in the callback, just like
the
> other case.

Good catch. This is a bug.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1359: net_error_ = OK;
On 2012/02/15 19:48:35, cbentzel wrote:
> Are net_error_ and os_error_ needed as members, or could they be passed in as
> arguments to CompleteRequests?
> 
> There's one little bit of funkiness where ERR_IO_PENDING is used both to
> indicate that the Job hasn't been dispatched yet, and that it is actively
doing
> a DnsTask/ProcTask. Perhaps that could be better served with an explicit
state.
> 
> If you'd like to table that to another cleanup CL, that would also be fine -
> this one already has a lot of stuff.

Agreed that the state indication is all over place. I'll clean it up.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1772: for (JobMap::iterator it = jobs_.begin();
it != jobs_.end(); ) {
On 2012/02/15 19:48:35, cbentzel wrote:
> BUG: I don't think this works correctly. The problem is that it will
potentially
> activate start a new Job running that doesn't needed to be aborted - for
> example, it might have the new DnsTransactionFactory after OnDnsChanged. I
think
> you want to accumulate the jobs to abort in one loop in a different data
> structure, and then abort them.

Right. That's a bug.
HostResolverImplTest.OnlyAbortExistingRequestsOnIPAddressChange should have
caught that. I'll investigate then fix.

http://codereview.chromium.org/9369045/diff/37003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1833: if (had_factory) {
On 2012/02/15 19:48:35, cbentzel wrote:
> Nit: braces not needed.
> 
> Also - would it be easier to just call OnDNSChanged each time? It seems like
it
> would be a no-op if there wasn't a factory before, so would simplify the logic
a
> tiny bit.

OnDNSChanged aborts all in-flight Jobs. Calling it after we got the DnsConfig
for the first time might hamper startup. Also, if we have a bad DnsConfigService
that always returns invalid DnsConfig, that would make us dump all the Jobs on
the floor every now and then.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:253: if (record.type == expected_type)
On 2012/02/15 19:48:35, cbentzel wrote:
> Are there any cases where DNS servers will append RRs of related domains to
> prevent a round trip? I suppose we'll see the DNS_NAME_MISMATCH then, but
might
> be something to support in the future. 

It might. I'll add a comment clarifying that for now we err on the cautious side
with the assumption that getaddrinfo will save our butt if we go too far. We'll
need a more flexible API that both records what kind of weirdness we've seen in
the DNS responses and allows to proceed despite such weirdness.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:255: continue;
On 2012/02/15 19:48:35, cbentzel wrote:
> Why do you want to continue in this case? 

Hmmm. I'll make it unconditionally reject.

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:257: if ((record.type == dns_protocol::kTypeCNAME)) {
On 2012/02/15 19:48:35, cbentzel wrote:
> Nit: double parents not needed.

clang barfed on it :-/

http://codereview.chromium.org/9369045/diff/37003/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:279: canon_name = record.name;
On 2012/02/15 19:48:35, cbentzel wrote:
> Shouldn't the canon_name always be the same? Should we check for that?

All the other checks will already catch that. This logic might be more clear if
we simply used |expected_name| as |canon_name| at the end.

[szym, 2012-02-16 22:50:40]

I'm currently working on DnsResponseTest.ParseToAddressListFail but you might
want to take a look at the Patch Set 11 which changes the AbortAllInProgressJobs
logic.

=======
There was a free-after-use bug where we'd access a |const AddressList&| passed
through callbacks after we freed the owner of that list (proc_task_ or
dns_task_).

There was also the issue of aborting the jobs started in request callback during
AbortAllInProgressJobs. The test that was supposed to test it was insufficient.
It created a new instance of HostResolverImpl. (This is how it was originally
written.) I wrote a new test. It was tricky because std::map preserves the order
and validity of iterators, so it's perfectly legal and well-defined to insert
new items into the map while iterating over it. If the new request is for the
same hostname, then it will lie behind the iterator in the map and won't be
aborted. To expose the bug, I create two requests for a low and high hostname
key to ensure that one of them will lie ahead in the map and thus would be
aborted.

The test failed on the old HostResolverImpl::AbortAllInProgressJobs.

Aborting jobs turns out to be rather tricky because we are completing a bunch of
Jobs in a bunch. Any of those completions can spawn new Jobs which should be
unaffected. The old HostResolverImpl (before the refactoring) would simply swap
out |jobs_|, because all Jobs in |jobs_| were running. The current code is
similar in that it first isolates the jobs to be Aborted and then it aborts
them.

[szym, 2012-02-17 06:35:32]

The test is also in place now. PTAL.

On 2012/02/16 22:50:40, szym wrote:
> I'm currently working on DnsResponseTest.ParseToAddressListFail but you might
> want to take a look at the Patch Set 11 which changes the
AbortAllInProgressJobs
> logic.
> 
> =======
> There was a free-after-use bug where we'd access a |const AddressList&| passed
> through callbacks after we freed the owner of that list (proc_task_ or
> dns_task_).
> 
> There was also the issue of aborting the jobs started in request callback
during
> AbortAllInProgressJobs. The test that was supposed to test it was
insufficient.
> It created a new instance of HostResolverImpl. (This is how it was originally
> written.) I wrote a new test. It was tricky because std::map preserves the
order
> and validity of iterators, so it's perfectly legal and well-defined to insert
> new items into the map while iterating over it. If the new request is for the
> same hostname, then it will lie behind the iterator in the map and won't be
> aborted. To expose the bug, I create two requests for a low and high hostname
> key to ensure that one of them will lie ahead in the map and thus would be
> aborted.
> 
> The test failed on the old HostResolverImpl::AbortAllInProgressJobs.
> 
> Aborting jobs turns out to be rather tricky because we are completing a bunch
of
> Jobs in a bunch. Any of those completions can spawn new Jobs which should be
> unaffected. The old HostResolverImpl (before the refactoring) would simply
swap
> out |jobs_|, because all Jobs in |jobs_| were running. The current code is
> similar in that it first isolates the jobs to be Aborted and then it aborts
> them.

[mmenke, 2012-02-21 16:34:24]

LGTM, just a couple nits.

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:437: config_service->Watch();
This was going to be moved to HostResolverImpl, right?

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1097: // TODO(szym): is there's any benefit in
having this distinction?
I'd say no, but think it's fine as-is for now.

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1578: // Job::CompleteRequests().
nit:  Might want to move this comment up to "if (job->num_active_requests() ==
0) {", since the calls to "delete job" and "RemoveJob" both also depend on this.

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl_unittest.cc:479: // proceed.
nit:  Remove "to"

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl_unittest.cc:483: is_signaled_(manual_reset, false)
{}
nit:  Fix indent

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl_unittest.cc:1159: virtual void
OnCompleted(ResolveRequest* resolve) {
nit:  OVERRIDE

[szym, 2012-02-21 17:18:19]

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:437: config_service->Watch();
On 2012/02/21 16:34:24, Matt Menke wrote:
> This was going to be moved to HostResolverImpl, right?

The order of AddObserver and Watch doesn't matter. It's also not obvious that
the service passed to HostResolverImpl ctor is not already watching in which
case Watch() would be a no-op call. I think it makes more sense if the service
is told to Watch at the time it is created.

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl_unittest.cc:1159: virtual void
OnCompleted(ResolveRequest* resolve) {
On 2012/02/21 16:34:24, Matt Menke wrote:
> nit:  OVERRIDE

Done. Also on the other 10 cases.

[cbentzel, 2012-02-21 18:41:59]

LGTM

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9369045/diff/44004/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1381: // A handle used
|HostResolverImpl::dispatcher_|.
Nit: used by

[commit-bot: I haz the power, 2012-02-21 19:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9369045/43074

[commit-bot: I haz the power, 2012-02-21 21:23:24]

Change committed as 122878