Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(110)

Issue 9369045: [net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous DNS ready for experiments. (Closed)

Created:
8 years, 10 months ago by szym
Modified:
8 years, 10 months ago
Reviewers:
cbentzel, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

[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

Patch Set 1 #

Patch Set 2 : Fixes to flags. #

Patch Set 3 : Added missing implementation. #

Total comments: 1

Patch Set 4 : Ready for test-drive. #

Total comments: 18

Patch Set 5 : Responded to review, removed async_host_resolver* #

Patch Set 6 : Fix ClientSocketFactory lifetime. Move ScopedAllowIO to StartWatch. Added DnsTask logging. #

Total comments: 37

Patch Set 7 : Flatten net-logging, added comments, detailed error from DnsResponse' #

Patch Set 8 : Flatenned DNS_TRANSACTION NetLog event into parent. #

Patch Set 9 : Rebased to master. #

Total comments: 33

Patch Set 10 : Flatten DNS_TRANSACTION again. (not responded to the review yet) #

Patch Set 11 : Fixed Abort and AbortAllInProgressJobs. #

Patch Set 12 : Added DnsResponseTest.ParseToAddressListFail #

Patch Set 13 : Rebased to master. #

Total comments: 9

Patch Set 14 : Responded to review. #

Patch Set 15 : Denitted. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+974 lines, -1673 lines) Patch
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -13 lines 0 comments Download
M chrome/browser/resources/net_internals/events_view.css View 1 2 3 4 5 6 7 9 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/resources/net_internals/source_entry.js View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -3 lines 0 comments Download
M net/base/address_list.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M net/base/address_list.cc View 1 2 3 4 5 3 chunks +5 lines, -3 lines 0 comments Download
M net/base/address_list_unittest.cc View 1 2 3 4 5 6 2 chunks +8 lines, -5 lines 0 comments Download
M net/base/host_resolver.h View 1 2 3 4 1 chunk +5 lines, -3 lines 0 comments Download
M net/base/host_resolver_impl.h View 1 2 3 4 5 6 7 8 9 10 7 chunks +27 lines, -6 lines 0 comments Download
M net/base/host_resolver_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 40 chunks +371 lines, -180 lines 0 comments Download
M net/base/host_resolver_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 21 chunks +85 lines, -63 lines 0 comments Download
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 10 7 chunks +23 lines, -57 lines 0 comments Download
M net/base/net_log_source_type_list.h View 1 2 3 4 5 6 7 9 1 chunk +10 lines, -13 lines 0 comments Download
M net/base/priority_queue.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -1 line 0 comments Download
D net/dns/async_host_resolver.h View 1 2 3 4 1 chunk +0 lines, -138 lines 0 comments Download
M net/dns/async_host_resolver.cc View 1 2 3 4 1 chunk +0 lines, -524 lines 0 comments Download
D net/dns/async_host_resolver_unittest.cc View 1 2 3 4 1 chunk +0 lines, -544 lines 0 comments Download
M net/dns/dns_config_service.h View 1 2 3 chunks +3 lines, -1 line 0 comments Download
M net/dns/dns_config_service_posix.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M net/dns/dns_config_service_posix_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M net/dns/dns_config_service_win.cc View 1 2 3 4 5 6 3 chunks +8 lines, -5 lines 0 comments Download
M net/dns/dns_response.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +24 lines, -4 lines 0 comments Download
M net/dns/dns_response.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +77 lines, -7 lines 0 comments Download
M net/dns/dns_response_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +245 lines, -27 lines 0 comments Download
M net/dns/dns_session.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M net/dns/dns_test_util.h View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +12 lines, -31 lines 0 comments Download
M net/dns/dns_transaction.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/dns/dns_transaction.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +31 lines, -16 lines 0 comments Download
M net/dns/dns_transaction_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +10 lines, -9 lines 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -3 lines 0 comments Download

Messages

Total messages: 38 (0 generated)
szym
Everything put together. This is the first draft. Job limits on ProcTask (after DnsTask fails) ...
8 years, 10 months ago (2012-02-09 22:52:56 UTC) #1
cbentzel
Cool. By "first draft" are you interested in getting some design feedback, a full review, ...
8 years, 10 months ago (2012-02-10 01:26:14 UTC) #2
szym
So far it compiles and passes the old unit tests, but I have not yet ...
8 years, 10 months ago (2012-02-10 02:04:18 UTC) #3
szym
If you're curious, this is now ready for test-drive. Still need to write the missing ...
8 years, 10 months ago (2012-02-10 18:11:47 UTC) #4
cbentzel
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): ...
8 years, 10 months ago (2012-02-10 19:51:08 UTC) #5
szym
Thanks for the comments. I'll now move some tests to dns_response_unittest.cc and also switch to ...
8 years, 10 months ago (2012-02-10 21:49:35 UTC) #6
cbentzel
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.h#newcode67 net/base/host_resolver_impl.h:67: public DnsConfigService::Observer, On 2012/02/10 21:49:36, szym wrote: > On ...
8 years, 10 months ago (2012-02-10 22:18:49 UTC) #7
szym
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 ...
8 years, 10 months ago (2012-02-11 00:01:37 UTC) #8
szym
I think this is in a decent shape for a thorough review at this stage. ...
8 years, 10 months ago (2012-02-13 20:16:26 UTC) #9
szym
I should not that the main outstanding issue which will cause performance and potentially correctness ...
8 years, 10 months ago (2012-02-13 20:18:36 UTC) #10
cbentzel
On 2012/02/13 20:18:36, szym wrote: > I should not that the main outstanding issue which ...
8 years, 10 months ago (2012-02-14 01:27:24 UTC) #11
cbentzel
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_win.cc File net/dns/dns_config_service_win.cc (right): ...
8 years, 10 months ago (2012-02-14 02:01:54 UTC) #12
cbentzel
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 ...
8 years, 10 months ago (2012-02-14 13:08:25 UTC) #13
cbentzel
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.cc#newcode420 net/base/host_resolver_impl.cc:420: // TODO(szym): Add tighter limits for proc_dispatcher_ Why do ...
8 years, 10 months ago (2012-02-14 13:12:06 UTC) #14
szym
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.cc#newcode420 net/base/host_resolver_impl.cc:420: // TODO(szym): Add tighter limits for proc_dispatcher_ On 2012/02/14 ...
8 years, 10 months ago (2012-02-14 15:56:24 UTC) #15
szym
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 net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl); On 2012/02/14 02:01:54, cbentzel wrote: ...
8 years, 10 months ago (2012-02-14 17:44:38 UTC) #16
cbentzel
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.cc#newcode1363 net/base/host_resolver_impl.cc:1363: resolver_->OnJobFinished(this, addr_list, ttl); On 2012/02/14 15:56:24, szym wrote: > ...
8 years, 10 months ago (2012-02-14 18:01:27 UTC) #17
szym
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 net/dns/dns_response.cc:259: ttl_sec = std::min(ttl_sec, record.ttl); On 2012/02/14 18:01:27, cbentzel wrote: ...
8 years, 10 months ago (2012-02-14 18:07:19 UTC) #18
cbentzel
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> ...
8 years, 10 months ago (2012-02-14 18:21:34 UTC) #19
mmenke
http://codereview.chromium.org/9369045/diff/17002/net/base/address_list_unittest.cc File net/base/address_list_unittest.cc (right): http://codereview.chromium.org/9369045/diff/17002/net/base/address_list_unittest.cc#newcode314 net/base/address_list_unittest.cc:314: ParseIPLiteralToNumber(tests[i].ip_address, &ip_number); nit: While you're here, might want to ...
8 years, 10 months ago (2012-02-14 18:46:00 UTC) #20
szym
I have flattened the NetLog a bit by merging HOST_RESOLVER_IMPL_PROC_TASK into HOST_RESOLVER_IMPL_JOB. I was planning ...
8 years, 10 months ago (2012-02-14 23:53:51 UTC) #21
szym
On 2012/02/14 23:53:51, szym wrote: > I have flattened the NetLog a bit by merging ...
8 years, 10 months ago (2012-02-15 00:07:29 UTC) #22
mmenke
On 2012/02/15 00:07:29, szym wrote: > On 2012/02/14 23:53:51, szym wrote: > > I have ...
8 years, 10 months ago (2012-02-15 00:17:19 UTC) #23
szym
> > I also think that the benefit of having an independent > > HOST_RESOLVER_IMPL_PROC_TASK ...
8 years, 10 months ago (2012-02-15 16:15:31 UTC) #24
mmenke1
On 2012/02/15 16:15:31, szym wrote: > > > I also think that the benefit of ...
8 years, 10 months ago (2012-02-15 16:31:18 UTC) #25
mmenke1
On 2012/02/15 16:31:18, mmenke wrote: > On 2012/02/15 16:15:31, szym wrote: > > > > ...
8 years, 10 months ago (2012-02-15 16:32:10 UTC) #26
szym
Ready to land as is.
8 years, 10 months ago (2012-02-15 17:49:01 UTC) #27
szym
On 2012/02/15 17:49:01, szym wrote: > Ready to land as is. Argh, I need to ...
8 years, 10 months ago (2012-02-15 18:13:38 UTC) #28
cbentzel
Most of this is nits/style. There are two points I want addressed before giving an ...
8 years, 10 months ago (2012-02-15 19:48:35 UTC) #29
mmenke
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.cc#newcode391 net/base/host_resolver_impl.cc:391: // to update it when ...
8 years, 10 months ago (2012-02-15 19:54:16 UTC) #30
szym
I'll remove the DispatcherHandle, but I'd rather keep the logic of updating the Handle within ...
8 years, 10 months ago (2012-02-15 20:28:05 UTC) #31
szym
I'm currently working on DnsResponseTest.ParseToAddressListFail but you might want to take a look at the ...
8 years, 10 months ago (2012-02-16 22:50:40 UTC) #32
szym
The test is also in place now. PTAL. On 2012/02/16 22:50:40, szym wrote: > I'm ...
8 years, 10 months ago (2012-02-17 06:35:32 UTC) #33
mmenke
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.cc#newcode437 net/base/host_resolver_impl.cc:437: config_service->Watch(); This was going ...
8 years, 10 months ago (2012-02-21 16:34:24 UTC) #34
szym
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.cc#newcode437 net/base/host_resolver_impl.cc:437: config_service->Watch(); On 2012/02/21 16:34:24, Matt Menke wrote: > This ...
8 years, 10 months ago (2012-02-21 17:18:19 UTC) #35
cbentzel
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.cc#newcode1381 net/base/host_resolver_impl.cc:1381: // A handle used |HostResolverImpl::dispatcher_|. Nit: used by
8 years, 10 months ago (2012-02-21 18:41:59 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/9369045/43074
8 years, 10 months ago (2012-02-21 19:43:54 UTC) #37
commit-bot: I haz the power
8 years, 10 months ago (2012-02-21 21:23:24 UTC) #38
Change committed as 122878

Powered by Google App Engine
This is Rietveld 408576698