Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 21368005: Detect domain-specific resolvers on OS X and disable DnsClient in such cases. (Closed)

Created:
6 years, 6 months ago by szym
Modified:
6 years, 6 months ago
Reviewers:
Mark Mentovai, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org
Visibility:
Public.

Description

Detect domain-specific resolvers on OS X and disable DnsClient This CL also changes the handling of DNS configuration options not yet fully implemented by DnsClient. Rather than DnsConfigService failing to provide DnsConfig, the new field DnsConfig.unhandled_options indicates to DnsClient that it should disable itself. BUG=265970 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218617

Patch Set 1 #

Patch Set 2 : fixup #

Patch Set 3 : roll dnsinfo back to configd-395.7 #

Patch Set 4 : logging, change dependency to System.framework #

Patch Set 5 : propagate DnsConfig::unhandled_options #

Patch Set 6 : put System before SystemConfiguration in net.gyp #

Patch Set 7 : remove explicit dependency on System.framework #

Patch Set 8 : Use dlsym to dynamically get dnsinfo. #

Patch Set 9 : dont forget to reset unhandled_options to false #

Patch Set 10 : add unhandled_options to NetLog #

Total comments: 14

Patch Set 11 : respond to mark's review #

Patch Set 12 : move dnsinfo.h to apple_apsl #

Patch Set 13 : update net.gyp #

Total comments: 12

Patch Set 14 : responded to Mark's second review #

Patch Set 15 : remove blank line #

Total comments: 31

Patch Set 16 : sync #

Patch Set 17 : addressed nits #

Total comments: 4

Patch Set 18 : Improve comment on DnsClient::SetConfig. Update histogram codepaths. #

Total comments: 4

Patch Set 19 : responded to review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -13 lines) Patch
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M net/dns/dns_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -1 line 0 comments Download
M net/dns/dns_client.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/dns/dns_config_service.h View 1 chunk +4 lines, -0 lines 0 comments Download
M net/dns/dns_config_service.cc View 1 2 3 4 5 6 7 8 9 4 chunks +5 lines, -1 line 0 comments Download
M net/dns/dns_config_service_posix.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M net/dns/dns_config_service_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +113 lines, -8 lines 0 comments Download
M net/dns/host_resolver_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/apple_apsl/README.chromium View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +10 lines, -0 lines 0 comments Download
A third_party/apple_apsl/dnsinfo.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +114 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
szym
mmenke: PTAL chrome/ and net/ mark: PTAL third_party/ and net/dns/dns_config_service_posix.cc
6 years, 6 months ago (2013-08-06 15:05:32 UTC) #1
Mark Mentovai
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc#newcode29 net/dns/dns_config_service_posix.cc:29: // dnsinfo symbols are available via System.framework, but can ...
6 years, 6 months ago (2013-08-06 15:41:00 UTC) #2
szym
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc#newcode57 net/dns/dns_config_service_posix.cc:57: dns_configuration_notify_key_t dns_configuration_notify_key; On 2013/08/06 15:41:00, Mark Mentovai wrote: > ...
6 years, 6 months ago (2013-08-06 15:48:58 UTC) #3
Mark Mentovai
You should get rid of the __OSX_AVAILABLE_STARTING and the <Availability.h> bits. Those are just likely ...
6 years, 6 months ago (2013-08-06 15:56:16 UTC) #4
szym
Thanks for the review, Mark. https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_service_posix.cc#newcode29 net/dns/dns_config_service_posix.cc:29: // dnsinfo symbols are ...
6 years, 6 months ago (2013-08-06 17:50:25 UTC) #5
Mark Mentovai
https://codereview.chromium.org/21368005/diff/98001/net/DEPS File net/DEPS (right): https://codereview.chromium.org/21368005/diff/98001/net/DEPS#newcode5 net/DEPS:5: "+third_party/apple_dnsinfo", You can remove this now. https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc ...
6 years, 6 months ago (2013-08-06 18:05:28 UTC) #6
szym
https://codereview.chromium.org/21368005/diff/98001/net/DEPS File net/DEPS (right): https://codereview.chromium.org/21368005/diff/98001/net/DEPS#newcode5 net/DEPS:5: "+third_party/apple_dnsinfo", On 2013/08/06 18:05:28, Mark Mentovai wrote: > You ...
6 years, 6 months ago (2013-08-06 22:14:19 UTC) #7
Mark Mentovai
LGTM
6 years, 6 months ago (2013-08-06 22:27:25 UTC) #8
mmenke
On 2013/08/06 22:27:25, Mark Mentovai wrote: > LGTM Taking the rest of the week off ...
6 years, 6 months ago (2013-08-07 00:45:13 UTC) #9
szym
mmenke: ping
6 years, 6 months ago (2013-08-16 15:44:04 UTC) #10
mmenke
On 2013/08/16 15:44:04, szym wrote: > mmenke: ping Sorry, so many reviews in my queue ...
6 years, 6 months ago (2013-08-16 15:50:48 UTC) #11
mmenke
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_client.cc File net/dns/dns_client.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_client.cc#newcode30 net/dns/dns_client.cc:30: if (config.IsValid() && !config.unhandled_options) { Is the second check ...
6 years, 6 months ago (2013-08-16 17:35:39 UTC) #12
szym
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_client.cc File net/dns/dns_client.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_client.cc#newcode30 net/dns/dns_client.cc:30: if (config.IsValid() && !config.unhandled_options) { On 2013/08/16 17:35:39, mmenke ...
6 years, 6 months ago (2013-08-16 17:48:26 UTC) #13
mmenke
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc#newcode259 net/dns/dns_config_service_posix.cc:259: DCHECK(dns_config_.unhandled_options); On 2013/08/16 17:35:39, mmenke wrote: > Think it's ...
6 years, 6 months ago (2013-08-16 17:51:24 UTC) #14
mmenke
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc#newcode167 net/dns/dns_config_service_posix.cc:167: // TODO(szym): Parse dns_config_t for resolvers rather than res_state. ...
6 years, 6 months ago (2013-08-16 18:24:19 UTC) #15
szym
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc#newcode178 net/dns/dns_config_service_posix.cc:178: if (num_resolvers > 1) { On 2013/08/16 18:24:19, mmenke ...
6 years, 6 months ago (2013-08-16 18:40:40 UTC) #16
mmenke
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc#newcode178 net/dns/dns_config_service_posix.cc:178: if (num_resolvers > 1) { On 2013/08/16 18:40:40, szym ...
6 years, 6 months ago (2013-08-16 18:43:26 UTC) #17
szym
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_service_posix.cc#newcode26 net/dns/dns_config_service_posix.cc:26: #include "third_party/apple_apsl/dnsinfo.h" On 2013/08/16 17:35:39, mmenke wrote: > nit: ...
6 years, 6 months ago (2013-08-17 05:08:50 UTC) #18
mmenke
I think you should take a look at HostResolverImpl, and make sure there are no ...
6 years, 6 months ago (2013-08-19 14:12:04 UTC) #19
szym
On 2013/08/19 14:12:04, mmenke wrote: > I think you should take a look at HostResolverImpl, ...
6 years, 6 months ago (2013-08-19 14:25:18 UTC) #20
szym
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc File chrome/browser/net/dns_probe_service.cc (right): https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc#newcode137 chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options = false; On 2013/08/19 14:12:04, mmenke wrote: > ...
6 years, 6 months ago (2013-08-19 14:26:15 UTC) #21
mmenke1
On 2013/08/19 14:25:18, szym wrote: > On 2013/08/19 14:12:04, mmenke wrote: > > I think ...
6 years, 6 months ago (2013-08-19 14:31:18 UTC) #22
mmenke
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc File chrome/browser/net/dns_probe_service.cc (right): https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc#newcode137 chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options = false; On 2013/08/19 14:26:16, szym wrote: > ...
6 years, 6 months ago (2013-08-19 14:54:03 UTC) #23
szym
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc File chrome/browser/net/dns_probe_service.cc (right): https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_probe_service.cc#newcode137 chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options = false; On 2013/08/19 14:54:03, mmenke wrote: > ...
6 years, 6 months ago (2013-08-20 18:55:34 UTC) #24
mmenke
LGTM, modulo comments. https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_service_posix.cc#newcode56 net/dns/dns_config_service_posix.cc:56: dlsym(handle_, "dns_configuration_free")); nit: These all should ...
6 years, 6 months ago (2013-08-20 19:11:26 UTC) #25
szym
Thanks for the review. https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_service_posix.cc File net/dns/dns_config_service_posix.cc (right): https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_service_posix.cc#newcode56 net/dns/dns_config_service_posix.cc:56: dlsym(handle_, "dns_configuration_free")); On 2013/08/20 19:11:27, ...
6 years, 6 months ago (2013-08-20 19:30:41 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/21368005/156001
6 years, 6 months ago (2013-08-20 19:31:10 UTC) #27
commit-bot: I haz the power
6 years, 6 months ago (2013-08-21 03:25:25 UTC) #28
Message was sent while issue was closed.
Change committed as 218617

Powered by Google App Engine
This is Rietveld 408576698