DnsClient refactoring + features (timeout, suffix search, server rotation).

DnsClient refactoring + features (timeout, suffix search, server rotation).

DnsClient::Request ==> DnsTransaction
DnsTransaction ==> DnsTransactionImpl
DnsClient ==> DnsTransactionFactory

BUG=109949
TEST=net_unittests --gtest_filter=Dns*


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

[szym, 2012-01-12 00:14:09]

First draft. Probably does not even compile yet. No tests for suffix search and
server fallback yet.

[szym, 2012-01-12 19:09:58]

I'm mostly done with the implementation. I'll work on dns_client_unittests.cc
now to test timeouts, server fallback and suffix search.

It might make sense to move DnsSession::NextTimeout to DnsTransaction or the
server fallback policy to DnsSession. In the future, timeout will be adjusted
per-server using historical performance, so DnsSession might be in charge of
that.

Note: After the realization that DnsClient returns DnsResponse, I decided that
DnsHosts should not be handled by DnsClient. Instead, the (planned)
HostResolverImpl::DnsTask will take care of it, as well as converting
DnsResponse to AddressList.

[cbentzel, 2012-01-13 01:17:20]

This CL is bigger than I want to tackle tonight, will get to it first thing in
the morning.

[cbentzel, 2012-01-13 13:39:54]

I didn't make it all the way through, thought I'd send some initial comments.
The big thing I'd like to change is DnsTransactionImpl's constructor.

http://codereview.chromium.org/9190031/diff/4023/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/async_host_resolver....
net/dns/async_host_resolver.h:67: typedef std::list<const DnsTransaction*>
DnsRequestList;
DnsTransactionList?

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode40
net/dns/dns_client.cc:40: for (size_t i = 0; i < name.size() && name[i]; i+=
name[i] + 1)
Nit: space between i and +=

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:261: successful_attempt_(NULL) {
first_server_index_ should be initialized - yeah, it's done in Start, but I like
to be safe.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:265: 
Should this check if qtype is one of the known types for now?

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:269: int rv = PrepareSearch();
Not really sure if I like having all this in the constructor because of the
synchronous completion case. Could you add a 

int Start();

method to the DnsTransaction interface? 

You'd also want to move the PrepareSearch logic to DnsClient::CreateTransaction
and return a NULL DnsTransaction in that case.

This would also remove the need for weak pointer support.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:307: std::string hostname;
It would be nice to use a better variable name than hostname, since it's
confusing with hostname_. Unfortunately, I don't think the RFCs differentiate
between dotted/colon separated textual representation and a sequence-of-labels
for this.

Perhaps labeled_name?

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:331: if (qname.size() == hostname.size()) {
Interesting - is an empty suffix allowed? Or is this some other case I'm not
thinking about.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:332: if (had_hostname)
Do you want to make this more generic in the loop - don't add another name to
qnames_ if it already exists? This would prevent double lookups if search
suffixes had the same name twice.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:376:
query.reset(attempts_[0]->query()->CloneWithNewId(id));
Note to self: See if this any easier.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:384: int server_index = first_server_index_ +
You should check for 0 length config.nameservers, since modulo operation with
that is undefined.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:460: // Set to NULL once the transaction completes.
Nit: perhaps null instead of NULL - NULL implies pointer, null is used in
callback_internal.h as well.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:466: std::deque<std::string> qnames_;
deque not really needed here, since just doing push'es on the back. I'm not too
strongly against it though.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h#newcode33
net/dns/dns_client.h:33: // Returns the |qtype|.
Nit: extra newline before this comment.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h#newcode41
net/dns/dns_client.h:41: class NET_EXPORT_PRIVATE DnsClient {
I wonder if this should just be called DnsTransactionFactory

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h#newcode54
net/dns/dns_client.h:54: // completion. Uses the source of |source_net_log| as
source dependency.
It would be worth pointing out that |hostname| must not be an IP literal - or
fixing DnsTransactionImpl to work correctly with IP literals.

For example, on an IPv6 literal it will fail early since DNSDomainFromDot will
fail. For IPv4, it may run into issues since DNSDomainFromDot will work, and
then suffixes may be appended to it particularly if ndots are set to a very high
value.

If you don't want DNSTransactionImpl (and this) to work with IP literals, then
please add some DCHECKs.

[szym, 2012-01-13 15:43:40]

Thanks for the review. I'll move back to DnsTransaction::Start. In that case it
does make sense to rename DnsClient to DnsTransactionFactory (since it does not
actually start the transactions), and rename the whole file
dns_transaction.{h,cc}

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:269: int rv = PrepareSearch();
On 2012/01/13 13:39:54, cbentzel wrote:
> Not really sure if I like having all this in the constructor because of the
> synchronous completion case. Could you add a 
> 
> int Start();
> 
> method to the DnsTransaction interface? 
> 
> You'd also want to move the PrepareSearch logic to
DnsClient::CreateTransaction
> and return a NULL DnsTransaction in that case.

Do you mean move PrepareSearch to Start? I'd rather not return NULL, because
then the caller receives no neterror.

I've been going back and forth with "int Start()". I modeled the current
behavior after the old HostResolverImpl::Job (now ProcTask) which posts itself
to the MessageLoop on unexpected synchronous failure. But I agree that Start()
is simpler and more clear.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:307: std::string hostname;
On 2012/01/13 13:39:54, cbentzel wrote:
> It would be nice to use a better variable name than hostname, since it's
> confusing with hostname_. Unfortunately, I don't think the RFCs differentiate
> between dotted/colon separated textual representation and a sequence-of-labels
> for this.
> 
> Perhaps labeled_name?  

I could use dotted_hostname for the other case.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:331: if (qname.size() == hostname.size()) {
On 2012/01/13 13:39:54, cbentzel wrote:
> Interesting - is an empty suffix allowed? Or is this some other case I'm not
> thinking about.

DNSDomainFromDot always appends the root domain at the end. This effectively
checks that config.search[i] == '.'.

I'm a bit concerned that DNSDomainFromDot simply skips empty labels
"www..chromium..org" is converted without question.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:332: if (had_hostname)
On 2012/01/13 13:39:54, cbentzel wrote:
> Do you want to make this more generic in the loop - don't add another name to
> qnames_ if it already exists? This would prevent double lookups if search
> suffixes had the same name twice.

I think it'd make sense to do this filtering at the DnsConfigService level,
since it will be re-done for every transaction.

I was actually thinking about converting DnsConfig::search with
DNSDomainFromDot, but then we'd still need a check for length here to ignore
prefix+suffix combinations that are too long.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:384: int server_index = first_server_index_ +
On 2012/01/13 13:39:54, cbentzel wrote:
> You should check for 0 length config.nameservers, since modulo operation with
> that is undefined.

If |config.nameservers| is empty, the config is invalid, see
DnsConfig::IsValid(), so we would never even create the DnsClient. I'll add a
DCHECK to DnsClient::CreateClient.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:466: std::deque<std::string> qnames_;
On 2012/01/13 13:39:54, cbentzel wrote:
> deque not really needed here, since just doing push'es on the back. I'm not
too
> strongly against it though. 

Need qnames_.pop_front().

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.h#newcode54
net/dns/dns_client.h:54: // completion. Uses the source of |source_net_log| as
source dependency.
On 2012/01/13 13:39:54, cbentzel wrote:
> It would be worth pointing out that |hostname| must not be an IP literal - or
> fixing DnsTransactionImpl to work correctly with IP literals.
> 

I'll add DCHECKs. For requests that can complete synchronously (IP literals,
HOSTS, cache), there is no need to create a HostResolverImpl::Job.

[mmenke, 2012-01-13 16:44:37]

Bunch of fairly minor comments.

Others may disagree, but I find it much easier to follow interactions between
objects when they're in different files, or at least their function bodies are
outside of the class declaration.

http://codereview.chromium.org/9190031/diff/4023/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/9190031/diff/4023/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1242: //     "socket": <Source id of the UDP
socket created for the attempt>,
nit:  Think "socket_source" is a little clearer.

http://codereview.chromium.org/9190031/diff/4023/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1253: //     "socket": <Source id of the UDP
socket that received the response>,
nit:  Think "socket_source" is a little clearer.

http://codereview.chromium.org/9190031/diff/4023/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/async_host_resolver....
net/dns/async_host_resolver.h:67: typedef std::list<const DnsTransaction*>
DnsRequestList;
nit:  DnsTransactionList

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode25
net/dns/dns_client.cc:25: #include "net/dns/dns_client.h"
This should go first, above the other includes, per style guide.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode52
net/dns/dns_client.cc:52: virtual Value* ToValue() const {
nit:  OVERRIDE

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode62
net/dns/dns_client.cc:62: uint16 qtype_;
nit:  Both of these can be const

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode71
net/dns/dns_client.cc:71: virtual Value* ToValue() const {
nit:  OVERRIDE

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcode81
net/dns/dns_client.cc:81: int answer_count_;
nit:  Both of these can be const

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:111: const DnsQuery* query() {
nit:  const

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:235: CompletionCallback callback_;
nit:  DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:380: new NetLogSourceParameter("socket",
socket->NetLog().source())));
Think it would be a little clearer if you also logged attempt_number, and used
attempt_number in the corresponding response event instead of "socket".  It's
easier to match single digit numbers again each other than source ids,
particularly when the browser has been running a while.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:470: // The one of the |attempts_| that succeeded first.
Nit:  This is the member of |attempts_| that succeeded first, if any.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:484: class DnsClientImpl : public DnsClient {
Is there a compelling reason to keep DnsClient and DnsClientImpl separate?  The
only things you seem to get out of it are the ability to forward declare
DnsSession in the header, and not include the scoped_refptr header.

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

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_response.cc#newc...
net/dns/dns_response.cc:185: const size_t qname_size = parser_.offset() - 2 *
sizeof(uint16) - hdr_size;
Could you add a comment that this always works because Parser returns a copy of
|parser_|, so parser_.cur_ is never modified?  Took me a while to realize that.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_response.cc#newc...
net/dns/dns_response.cc:204: 
nit:  Remove extra linebreak.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_response_unittes...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_response_unittes...
net/dns/dns_response_unittest.cc:222: EXPECT_EQ(0x8180, resp.flags());
I assume you planned to do so already, but just in case it didn't occur to you,
should add tests for new qname, qtype, and GetDottedName functions in this file.

http://codereview.chromium.org/9190031/diff/4023/net/net.gyp
File net/net.gyp (left):

http://codereview.chromium.org/9190031/diff/4023/net/net.gyp#oldcode1074
net/net.gyp:1074: 'dns/dns_transaction_unittest.cc',
Looks like you forgot to delete this file from the repository.

[mmenke, 2012-01-13 17:16:24]

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:405: NetLog::TYPE_DNS_TRANSACTION_QUERY,
Where do you log the corresponding end event?

[szym, 2012-01-13 17:17:42]

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:380: new NetLogSourceParameter("socket",
socket->NetLog().source())));
On 2012/01/13 16:44:37, Matt Menke wrote:
> Think it would be a little clearer if you also logged attempt_number, and used
> attempt_number in the corresponding response event instead of "socket".  It's
> easier to match single digit numbers again each other than source ids,
> particularly when the browser has been running a while.

The reason why I don't include attempt_number is because I assumed it would be
apparent from the net-internals view. The third attempt logged within
DNS_TRANSACTION_QUERY will have attempt_number = 2. The reason I'm including a
reference to the socket source is so that we can see the server address (which
is logged in Connect) and the socket data.

I now realized that I never EndEvent(DNS_TRANSACTION_QUERY). I'll fix that.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:484: class DnsClientImpl : public DnsClient {
On 2012/01/13 16:44:37, Matt Menke wrote:
> Is there a compelling reason to keep DnsClient and DnsClientImpl separate? 
The
> only things you seem to get out of it are the ability to forward declare
> DnsSession in the header, and not include the scoped_refptr header.

The reason I'd keep them separate is so that we can provide MockDnsClient for
tests of HostResolverImpl (see async_host_resolver_unittest.cc). I'm also
thinking about creating a similar factory object for HostResolverImpl::ProcTask,
although there would be less benefit in that.

The reasoning behind MockDnsClient is that setting up DnsClient for tests by
providing a MockClientSocketFactory is cumbersome and often leads to effectively
testing a different component than intended.

[mmenke, 2012-01-13 17:22:58]

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:380: new NetLogSourceParameter("socket",
socket->NetLog().source())));
On 2012/01/13 17:17:42, szym wrote:
> On 2012/01/13 16:44:37, Matt Menke wrote:
> > Think it would be a little clearer if you also logged attempt_number, and
used
> > attempt_number in the corresponding response event instead of "socket". 
It's
> > easier to match single digit numbers again each other than source ids,
> > particularly when the browser has been running a while.
> 
> The reason why I don't include attempt_number is because I assumed it would be
> apparent from the net-internals view. The third attempt logged within
> DNS_TRANSACTION_QUERY will have attempt_number = 2. The reason I'm including a
> reference to the socket source is so that we can see the server address (which
> is logged in Connect) and the socket data.
> 
> I now realized that I never EndEvent(DNS_TRANSACTION_QUERY). I'll fix that.

I was just thinking that it would be clearer for the
TYPE_DNS_TRANSACTION_RESPONSE to have a number rather than a source, so it's
clearer which attempt actually succeeded, rather than having to match up 4-6
digit source ids.  If you prefer the current approach, that's fine with me.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:484: class DnsClientImpl : public DnsClient {
On 2012/01/13 17:17:42, szym wrote:
> On 2012/01/13 16:44:37, Matt Menke wrote:
> > Is there a compelling reason to keep DnsClient and DnsClientImpl separate? 
> The
> > only things you seem to get out of it are the ability to forward declare
> > DnsSession in the header, and not include the scoped_refptr header.
> 
> The reason I'd keep them separate is so that we can provide MockDnsClient for
> tests of HostResolverImpl (see async_host_resolver_unittest.cc). I'm also
> thinking about creating a similar factory object for
HostResolverImpl::ProcTask,
> although there would be less benefit in that.
> 
> The reasoning behind MockDnsClient is that setting up DnsClient for tests by
> providing a MockClientSocketFactory is cumbersome and often leads to
effectively
> testing a different component than intended.
> 
> 

Ah, right.  Sounds good to me.

[cbentzel, 2012-01-13 18:12:59]

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:269: int rv = PrepareSearch();
On 2012/01/13 15:43:40, szym wrote:
> On 2012/01/13 13:39:54, cbentzel wrote:
> > Not really sure if I like having all this in the constructor because of the
> > synchronous completion case. Could you add a 
> > 
> > int Start();
> > 
> > method to the DnsTransaction interface? 
> > 
> > You'd also want to move the PrepareSearch logic to
> DnsClient::CreateTransaction
> > and return a NULL DnsTransaction in that case.
> 
> Do you mean move PrepareSearch to Start? I'd rather not return NULL, because
> then the caller receives no neterror.

I did mean do it in DnsClient::CreateTransaction, but am completely happy with
having it done in DnsTransaction::Start

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:384: int server_index = first_server_index_ +
On 2012/01/13 15:43:40, szym wrote:
> On 2012/01/13 13:39:54, cbentzel wrote:
> > You should check for 0 length config.nameservers, since modulo operation
with
> > that is undefined.
> 
> If |config.nameservers| is empty, the config is invalid, see
> DnsConfig::IsValid(), so we would never even create the DnsClient. I'll add a
> DCHECK to DnsClient::CreateClient.
> 
> 

What if the config changed while the DnsTransaction is active to be invalid? I
suppose we won't swap it in - but I want to make sure we don't do an undefined
C++ operation here.

[szym, 2012-01-13 20:06:17]

Renamed DnsClient to DnsTransactionFactory and responded to most of the review.
I'll move dns_client to dns_transaction in the next patch.

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/9190031/diff/4023/net/dns/dns_client.cc#newcod...
net/dns/dns_client.cc:384: int server_index = first_server_index_ +
On 2012/01/13 18:12:59, cbentzel wrote:
> On 2012/01/13 15:43:40, szym wrote:
> > On 2012/01/13 13:39:54, cbentzel wrote:
> > > You should check for 0 length config.nameservers, since modulo operation
> with
> > > that is undefined.
> > 
> > If |config.nameservers| is empty, the config is invalid, see
> > DnsConfig::IsValid(), so we would never even create the DnsClient. I'll add
a
> > DCHECK to DnsClient::CreateClient.
> > 
> > 
> 
> What if the config changed while the DnsTransaction is active to be invalid? I
> suppose we won't swap it in - but I want to make sure we don't do an undefined
> C++ operation here. 

DnsConfig is const in DnsSession, so DnsTransaction will not be affected by
config change unless the transaction is destroyed.

[cbentzel, 2012-01-13 22:17:25]

This is a really nice change. I like how the delayed-response-after-retry is
handled. I have not gone through the unit tests yet.

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

http://codereview.chromium.org/9190031/diff/14003/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:431: scoped_ptr<DnsTransaction> dns_req =
client_->CreateTransaction(
dns_transaction instead of dns_req

http://codereview.chromium.org/9190031/diff/14003/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/9190031/diff/14003/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:112: size_t max_dns_requests_;
Perhaps max_dns_transactions_.

http://codereview.chromium.org/9190031/diff/14003/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:115: DnsTransactionList dns_requests_;
dns_transactions_

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h#newcode6
net/dns/dns_client.h:6: #ifndef NET_DNS_DNS_TRANSACTION_H_
This needs to move file names for these guards to change.

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h#newcode26
net/dns/dns_client.h:26: // the query") to fight poisoning.
I'd move the "It does not do caching" comment up here, instead of from the
factory.

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h#newcode40
net/dns/dns_client.h:40: // ERR_IO_PENDING in which case the result will be
passed via the callback.
"The callback" isn't really defined as part of the interface. 

Should the callback be passed into start instead of CreateTransaction?

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:192: const size_t qname_size = parser_.offset() - 2 *
sizeof(uint16) - hdr_size;
This would benefit from an explanation - particularly why the parser_.offset is
what it is.

Another option is to reconstruct from io_buffer_->data() + hdr_size, which may
be more clear. 

Finally, you could just cache it in InitResponse from the query.

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:207: return DNSDomainToString(qname());
[Note to self: Looks like the expansion is done by DnsResponseParser]

This doesn't handle the label compression case correctly (see 4.1.4 of RFC1035).
DNSDomainToString will return an empty string in that case.

Not sure if the expansion should be done in qname() or here.

It may not be a big issue in practice since it will typically done for response
names, but should be defensive about it.

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.h#newc...
net/dns/dns_response.h:49: bool is_valid() const { return packet_ != NULL; }
Old naming scheme is more appropriate here.

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.h#newc...
net/dns/dns_response.h:98: bool is_valid() const;
Ditto, prefer IsValid().

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.h#newc...
net/dns/dns_response.h:108: base::StringPiece qname() const;
Where are these being used?

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:175: return socket_->Connect(server_);
Why not DoConnectComplete? Is this guaranteed to complete synchronously?

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:186: int DoSendQueryComplete(int rv) {
DCHECK_NE(ERR_IO_PENDING, rv);

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:397: attempt_number));
Nice use of currying here.

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:418: void OnAttemptComplete(int attempt_number, int
rv) {
DCHECK that attempt_number within reasonable range? implementations of vector
may do this range check in debug builds though.

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:447: // TODO(szym): Some nameservers could fail and
we should just ignore
It wouldn't be too hard to handle the ERR_DNS_SERVER_FAILED case in a naive way
which moves to the next server if available - perhaps do that now?

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:480: const DnsUDPAttempt* successful_attempt_;
|successful_attempt_| doesn't need to be a member - could be passed into
DoCallback from OnAttemptComplete instead.

[szym, 2012-01-13 22:32:44]

As usual, commenting only on the things that need commenting. I'll fix the rest.

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/9190031/diff/14003/net/dns/dns_client.h#newcode40
net/dns/dns_client.h:40: // ERR_IO_PENDING in which case the result will be
passed via the callback.
On 2012/01/13 22:17:25, cbentzel wrote:
> "The callback" isn't really defined as part of the interface. 
> 
> Should the callback be passed into start instead of CreateTransaction?

I'm fine either way. The benefit of having it in the constructor is that we
don't need to worry about initialization.

A potential benefit of having it in Start() is that then CallbackType could be
Callback<void(int, const DnsResponse*)>. If any user really needed the
DnsTransaction* they could just curry it, albeit this costs a little bit of
memory for storage. I don't actually expect to need the pointer in
HostResolverImpl, since the callback will already be bound to a specific Job or
a DnsTask.

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:175: return socket_->Connect(server_);
On 2012/01/13 22:17:25, cbentzel wrote:
> Why not DoConnectComplete? Is this guaranteed to complete synchronously?

We don't actually have an API that would allow for asynchronous completion.
DatagramClientSocket::Connect() does not take callback. In fact, I think we
should rather DCHECK that the rv here is not ERR_IO_PENDING.

[szym, 2012-01-17 15:38:25]

Still working on tests.

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:207: return DNSDomainToString(qname());
On 2012/01/13 22:17:25, cbentzel wrote:
> [Note to self: Looks like the expansion is done by DnsResponseParser]
> 
> This doesn't handle the label compression case correctly (see 4.1.4 of
RFC1035).
> DNSDomainToString will return an empty string in that case.
> 
> Not sure if the expansion should be done in qname() or here.
> 
> It may not be a big issue in practice since it will typically done for
response
> names, but should be defensive about it.  

The whole question section must be exactly as in the query, so the name in the
question cannot be compressed by the server.

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

http://codereview.chromium.org/9190031/diff/12005/net/dns/dns_response.h#newc...
net/dns/dns_response.h:108: base::StringPiece qname() const;
On 2012/01/13 22:17:25, cbentzel wrote:
> Where are these being used? 

I'm not sure these will be used outside of tests. DnsTransaction::GetType()
should match DnsResponse::qtype() and qname() will be accessed via
GetDottedName().

[szym, 2012-01-19 16:35:46]

A complete suite of tests. http://codereview.chromium.org/9251019/ needs to be
committed first to remove ref-counting from DelayedSocketData.

[mmenke, 2012-01-19 17:24:47]

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

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver....
net/dns/async_host_resolver.cc:431: scoped_ptr<DnsTransaction> dns_req =
client_->CreateTransaction(
nit:  "scoped_ptr<DnsTransaction> dns_req(client_...);" is preferred.

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver....
net/dns/async_host_resolver.cc:431: scoped_ptr<DnsTransaction> dns_req =
client_->CreateTransaction(
nit:  dns_transaction

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver....
net/dns/async_host_resolver.h:112: size_t max_dns_requests_;
nit:  max_dns_transactions

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver....
net/dns/async_host_resolver.h:115: DnsTransactionList dns_requests_;
nit:  dns_transactions_

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
net/dns/async_host_resolver_unittest.cc:68: 
nit:  Remove line break

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
net/dns/async_host_resolver_unittest.cc:100: uint16 qtype_;
nit:  You can make some of these const.

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
net/dns/async_host_resolver_unittest.cc:124: std::map<Key, DnsResponse*>
responses;
This isn't used anywhere, as MockDnsClient now tracks them.

http://codereview.chromium.org/9190031/diff/3002/net/dns/dns_config_service_w...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9190031/diff/3002/net/dns/dns_config_service_w...
net/dns/dns_config_service_win.cc:243: // TODO(szym): read
AppendToMultiLabelName to determine ndots BUG=109902
Why remove this comment?

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

http://codereview.chromium.org/9190031/diff/3002/net/dns/dns_transaction.cc#n...
net/dns/dns_transaction.cc:311: std::string labelled_hostname;
nit:  "labeled" is preferred in the US, technically.  I don't really care, but
since we deliberately went with "canceled" elsewhere...

[szym, 2012-01-20 19:38:27]

Thanks for review. This CL is still blocked on
http://codereview.chromium.org/9251019/

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/3002/net/dns/async_host_resolver_...
net/dns/async_host_resolver_unittest.cc:124: std::map<Key, DnsResponse*>
responses;
On 2012/01/19 17:24:47, Matt Menke wrote:
> This isn't used anywhere, as MockDnsClient now tracks them.

Used in Finish().

[cbentzel, 2012-01-21 03:41:16]

LGTM

http://codereview.chromium.org/9190031/diff/32001/net/dns/async_host_resolver...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/32001/net/dns/async_host_resolver...
net/dns/async_host_resolver_unittest.cc:52: public
base::SupportsWeakPtr<MockDnsTransactionFactory> {
Nit: maybe just indent by 4 from line start here.

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_session.cc#newc...
net/dns/dns_session.cc:40: // TODO(szym): Adapt timeout to observed RTT.
BUG=110197
Nit: direct link to crbug.com is more common than BUG=

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction_uni...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:38: class TestUDPClientSocket : public
MockUDPClientSocket {
I wonder if it would make more sense to add a peer-endpoint expectation to
MockConnect. That way you could just use the existing
MockUDPClientSocket+SocketFactory and validate that the servers are being
accessed in the correct order.

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:213: void AddResponse(const char*
dotted_name,
Why not std::string instead?

[mmenke, 2012-01-24 18:27:00]

LGTM

http://codereview.chromium.org/9190031/diff/32001/net/dns/async_host_resolver...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/32001/net/dns/async_host_resolver...
net/dns/async_host_resolver_unittest.cc:56: class MockRequest : public
DnsTransaction,
nit:  Might want to call this MockTransaction.

http://codereview.chromium.org/9190031/diff/32001/net/dns/async_host_resolver...
net/dns/async_host_resolver_unittest.cc:123: std::map<Key, DnsResponse*>
responses;
While you're here, could you rename these to end in underscores?

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

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction.h#n...
net/dns/dns_transaction.h:4: 
nit:  Extra blank line not needed.

[szym, 2012-01-25 18:04:33]

Thanks for the review.

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction_uni...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/9190031/diff/32001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:38: class TestUDPClientSocket : public
MockUDPClientSocket {
On 2012/01/21 03:41:16, cbentzel wrote:
> I wonder if it would make more sense to add a peer-endpoint expectation to
> MockConnect. That way you could just use the existing
> MockUDPClientSocket+SocketFactory and validate that the servers are being
> accessed in the correct order.

I'll put it on my list. I would actually like to replace many of the DCHECKs in
socket_test_util with ASSERT_. This way a failing test would not lead to a
crash. This would also preserve the independence of server order and suffix
order in DnsTransactionTest.

[commit-bot: I haz the power, 2012-01-25 18:06:51]

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

[commit-bot: I haz the power, 2012-01-25 20:06:29]

Try job failure for 9190031-46003 (retry) on win_rel for step "ui_tests".
It's a second try, previously, steps "unit_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-25 21:23:18]

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

[commit-bot: I haz the power, 2012-01-26 00:04:01]

Change committed as 119146