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
mmenke: PTAL chrome/ and net/ mark: PTAL third_party/ and net/dns/dns_config_service_posix.cc
7 years, 7 months ago
(2013-08-06 15:05:32 UTC)
#1
mmenke: PTAL chrome/ and net/
mark: PTAL third_party/ and net/dns/dns_config_service_posix.cc
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 ...
7 years, 7 months ago
(2013-08-06 15:41:00 UTC)
#2
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:29: // dnsinfo symbols are available via
System.framework, but can
System.framework is just a symbolic link to libSystem.dylib. Nobody knows it by
that name. Replace System.framework with libSystem.dylib on this line and on
line 31…
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:30: // also be present in
SystemConfiguration. To avoid confusion,
…and say SystemConfiguration.framework on this line to eliminate ambiguity.
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:55: bool ready() const { return handle_; }
ready() isn’t a great interface. It checks that the library is open, but not
that the symbols were looked up successfully. When working with private
interfaces, you really need to be more careful.
I recommend eliminating ready() and instead checking for the actual functions
you need at the points of use. On line 87, instead of calling ready(), you could
say
if (!dns_info_api().dns_configuration_notify_key)
return false;
Alternatively, you can keep ready() and make it check for all of the symbols
(perhaps as an in-line check, or perhaps by dlclosing and NULLing out handle_ in
the constructor if any dlsym fails).
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:57: dns_configuration_notify_key_t
dns_configuration_notify_key;
Someone outside of this class can accidentally say
dns_info_api().dns_configuration_notify_key = NULL;
That’s kinda bad. You can make the fields const, which will require you to
rewrite this constructor in initializer list format, but that’d be better…
DnsInfoApi()
: handle_(dlopen("/usr/lib/libSystem.dylib", RTLD_LAZY | RTLD_NOLOAD),
dns_configuration_notify_key(handle_ ? reinterpret_cast<…>(dlsym(…)) :
NULL),
// and so on
This will almost certainly require you to declare
private:
void* handle_;
before
public:
dns_configuration_notify_key_t dns_configuration_notify_key;
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:169:
dns_info_api().dns_configuration_free(dns_config);
Can you add a scoper to free this instead of calling free explicitly? That would
guard against early returns.
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
File third_party/apple_dnsinfo/README.chromium (right):
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
third_party/apple_dnsinfo/README.chromium:9: Provides dns_config_t which is used
by /net/dns/dns_config_service_posix.cc to
Since all you need is the definition of a struct, you could handle this by just
defining the struct inline in your own code. There is precedent for doing this
in Chrome code, and it’s much easier and ligher-weight than having to bring in a
whole new third_party thing for just one silly header.
However, there’s also precedent for using third_party for this, enough so that
we already have a directory for such code. If you opt to keep this header in
third_party, place it in third_party/apple_apsl, and update the README.chromium
in that directory in the same manner as the entries that are already there.
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
File third_party/apple_dnsinfo/dnsinfo.h (right):
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
third_party/apple_dnsinfo/dnsinfo.h:74: DNS_VAR(uint32_t, flags);
I’m curious why you chose this version of the file from configd-395.7, part of
Mac OS X 10.7.2.
Chrome supports Mac OS X 10.6, and Mac OS X 10.8.x is current. These structures
appear to already have some provisions for compatibility between versions built
in (via the “reserved” fields) so it should be safe to just grab the current
definition of the structure as long as you take care not to use fields that
don’t exist on older systems when running on older systems.
configd-395.7 is neither the configd from the base 10.7 nor the most recent
10.7. It just appears to be some random version.
The current version is
http://opensource.apple.com/source/configd/configd-453.19/dnsinfo/dnsinfo.h.
Generally, just start at http://opensource.apple.com/, pick the OS release
you’re interested in, and then find the source from there.
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: > ...
7 years, 7 months ago
(2013-08-06 15:48:58 UTC)
#3
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
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:
> Someone outside of this class can accidentally say
>
> dns_info_api().dns_configuration_notify_key = NULL;
That's exactly why dns_info_api() returns a _const_ DnsInfoApi&.
>
> That’s kinda bad. You can make the fields const, which will require you to
> rewrite this constructor in initializer list format, but that’d be better…
>
> DnsInfoApi()
> : handle_(dlopen("/usr/lib/libSystem.dylib", RTLD_LAZY | RTLD_NOLOAD),
> dns_configuration_notify_key(handle_ ? reinterpret_cast<…>(dlsym(…)) :
> NULL),
> // and so on
>
> This will almost certainly require you to declare
>
> private:
> void* handle_;
>
> before
>
> public:
> dns_configuration_notify_key_t dns_configuration_notify_key;
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
File third_party/apple_dnsinfo/dnsinfo.h (right):
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
third_party/apple_dnsinfo/dnsinfo.h:74: DNS_VAR(uint32_t, flags);
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> I’m curious why you chose this version of the file from configd-395.7, part of
> Mac OS X 10.7.2.
>
> Chrome supports Mac OS X 10.6, and Mac OS X 10.8.x is current. These
structures
> appear to already have some provisions for compatibility between versions
built
> in (via the “reserved” fields) so it should be safe to just grab the current
> definition of the structure as long as you take care not to use fields that
> don’t exist on older systems when running on older systems.
>
> configd-395.7 is neither the configd from the base 10.7 nor the most recent
> 10.7. It just appears to be some random version.
>
> The current version is
> http://opensource.apple.com/source/configd/configd-453.19/dnsinfo/dnsinfo.h.
> Generally, just start at http://opensource.apple.com/, pick the OS release
> you’re interested in, and then find the source from there.
395.7 is the newest version that would compile. In patches 1&2 I tried 453.19,
but the availability macro:
__OSX_AVAILABLE_STARTING(__MAC_10_8, __IPHONE_6_0);
would not compile.
Mark Mentovai
You should get rid of the __OSX_AVAILABLE_STARTING and the <Availability.h> bits. Those are just likely ...
7 years, 7 months ago
(2013-08-06 15:56:16 UTC)
#4
You should get rid of the __OSX_AVAILABLE_STARTING and the <Availability.h>
bits. Those are just likely to cause things to break as we transition between
SDKs anyway.
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 ...
7 years, 7 months ago
(2013-08-06 17:50:25 UTC)
#5
Thanks for the review, Mark.
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:29: // dnsinfo symbols are available via
System.framework, but can
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> System.framework is just a symbolic link to libSystem.dylib. Nobody knows it
by
> that name. Replace System.framework with libSystem.dylib on this line and on
> line 31…
Done.
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:30: // also be present in
SystemConfiguration. To avoid confusion,
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> …and say SystemConfiguration.framework on this line to eliminate ambiguity.
Done.
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:55: bool ready() const { return handle_; }
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> ready() isn’t a great interface. It checks that the library is open, but not
> that the symbols were looked up successfully. When working with private
> interfaces, you really need to be more careful.
>
> I recommend eliminating ready() and instead checking for the actual functions
> you need at the points of use. On line 87, instead of calling ready(), you
could
> say
>
> if (!dns_info_api().dns_configuration_notify_key)
> return false;
>
> Alternatively, you can keep ready() and make it check for all of the symbols
> (perhaps as an in-line check, or perhaps by dlclosing and NULLing out handle_
in
> the constructor if any dlsym fails).
Done.
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:169:
dns_info_api().dns_configuration_free(dns_config);
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> Can you add a scoper to free this instead of calling free explicitly? That
would
> guard against early returns.
Done.
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
File third_party/apple_dnsinfo/README.chromium (right):
https://codereview.chromium.org/21368005/diff/66001/third_party/apple_dnsinfo...
third_party/apple_dnsinfo/README.chromium:9: Provides dns_config_t which is used
by /net/dns/dns_config_service_posix.cc to
On 2013/08/06 15:41:00, Mark Mentovai wrote:
> Since all you need is the definition of a struct, you could handle this by
just
> defining the struct inline in your own code. There is precedent for doing this
> in Chrome code, and it’s much easier and ligher-weight than having to bring in
a
> whole new third_party thing for just one silly header.
>
> However, there’s also precedent for using third_party for this, enough so that
> we already have a directory for such code. If you opt to keep this header in
> third_party, place it in third_party/apple_apsl, and update the
README.chromium
> in that directory in the same manner as the entries that are already there.
Done. Moved to apple_apsl.
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 ...
7 years, 7 months ago
(2013-08-06 18:05:28 UTC)
#6
7 years, 7 months ago
(2013-08-06 22:27:25 UTC)
#8
LGTM
mmenke
On 2013/08/06 22:27:25, Mark Mentovai wrote: > LGTM Taking the rest of the week off ...
7 years, 7 months ago
(2013-08-07 00:45:13 UTC)
#9
On 2013/08/06 22:27:25, Mark Mentovai wrote:
> LGTM
Taking the rest of the week off (And took today off, too), so won't review until
Monday.
szym
mmenke: ping
7 years, 6 months ago
(2013-08-16 15:44:04 UTC)
#10
mmenke: ping
mmenke
On 2013/08/16 15:44:04, szym wrote: > mmenke: ping Sorry, so many reviews in my queue ...
7 years, 6 months ago
(2013-08-16 15:50:48 UTC)
#11
On 2013/08/16 15:44:04, szym wrote:
> mmenke: ping
Sorry, so many reviews in my queue that I missed this one. I'll get back to you
today.
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 ...
7 years, 6 months ago
(2013-08-16 17:35:39 UTC)
#12
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. ...
7 years, 6 months ago
(2013-08-16 18:24:19 UTC)
#15
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:167: // TODO(szym): Parse dns_config_t for
resolvers rather than res_state.
On 2013/08/16 17:48:27, szym wrote:
> On 2013/08/16 17:35:39, mmenke wrote:
> > This comment isn't quite right - we actually do still get the resolvers from
> > res_state.
>
> We do right now, and that's why I left a TODO here to change it. dns_config_t
is
> the ground truth, because res_init basically copies data from dns_config_t to
> __res_state.
Right, missed the TODO.
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:178: if (num_resolvers > 1) {
Rather then checking if there's more than one resolver, shouldn't we be checking
if there's any resolver with a non-null domain? Or are we guaranteed the first
resolver will be for all domains, and other resolvers will not be? It looks
like we get one nameserver per "resolver", so I'd think that in the case of
multiple nameservers, we'd have multiple resolvers, even if none are scoped to a
particular domain.
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 ...
7 years, 6 months ago
(2013-08-16 18:40:40 UTC)
#16
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:178: if (num_resolvers > 1) {
On 2013/08/16 18:24:19, mmenke wrote:
> Rather then checking if there's more than one resolver, shouldn't we be
checking
> if there's any resolver with a non-null domain?
That is what we need to check.
> Or are we guaranteed the first
> resolver will be for all domains, and other resolvers will not be?
I'm not yet sure, but I'll need to figure it out before I can address the TODO
above.
> It looks like we get one nameserver per "resolver", so I'd think that in the
case of
> multiple nameservers, we'd have multiple resolvers, even if none are scoped to
a
> particular domain.
dns_resolver_t is a configuration of a "resolver" but it has room for multiple
nameservers. |n_nameserver| is the number of items in the |nameserver| array of
sockaddr*.
The command "scutil --dns" dumps the config, and you'll see the different
resolvers, in the order they appear in dns_config_t.
The logic here is: if there is more than one non-mdns resolver (not nameserver),
then we can't handle it. We could check if that resolver has configured a
specific domain (it's possible to distinguish the "domain" from the "search
domain"), but I don't think it's necessary.
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 ...
7 years, 6 months ago
(2013-08-16 18:43:26 UTC)
#17
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
File net/dns/dns_config_service_posix.cc (right):
https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:178: if (num_resolvers > 1) {
On 2013/08/16 18:40:40, szym wrote:
> On 2013/08/16 18:24:19, mmenke wrote:
> > Rather then checking if there's more than one resolver, shouldn't we be
> checking
> > if there's any resolver with a non-null domain?
>
> That is what we need to check.
>
> > Or are we guaranteed the first
> > resolver will be for all domains, and other resolvers will not be?
>
> I'm not yet sure, but I'll need to figure it out before I can address the TODO
> above.
>
> > It looks like we get one nameserver per "resolver", so I'd think that in the
> case of
> > multiple nameservers, we'd have multiple resolvers, even if none are scoped
to
> a
> > particular domain.
>
> dns_resolver_t is a configuration of a "resolver" but it has room for multiple
> nameservers. |n_nameserver| is the number of items in the |nameserver| array
of
> sockaddr*.
>
> The command "scutil --dns" dumps the config, and you'll see the different
> resolvers, in the order they appear in dns_config_t.
>
> The logic here is: if there is more than one non-mdns resolver (not
nameserver),
> then we can't handle it. We could check if that resolver has configured a
> specific domain (it's possible to distinguish the "domain" from the "search
> domain"), but I don't think it's necessary.
Thanks for the explanation. Seems reasonable, then.
I think you should take a look at HostResolverImpl, and make sure there are no ...
7 years, 6 months ago
(2013-08-19 14:12:04 UTC)
#19
I think you should take a look at HostResolverImpl, and make sure there are no
places where the assumption that a valid config implies async dns is enabled, as
it's no longer true. For example:
if (dns_config.IsValid())
UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
Now seems to be wrong (The lines above appear twice). Perhaps we should be
querying the DnsClient instead?
Another place that looks to be wrong is "had_dns_config_ =
resolver_->HaveDnsConfig();" Variable value and name are correct, but it's
assumed that had_dns_config_ is true if and only if we can use the async
resolver.
Should we have a unit test for this? If the current code doesn't crash when we
return a config with unhandled_options as true, I'm having trouble figuring out
why.
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options =
false;
Wonder if we should just skip the probe in this case... Not a concern for this
CL.
szym
On 2013/08/19 14:12:04, mmenke wrote: > I think you should take a look at HostResolverImpl, ...
7 years, 6 months ago
(2013-08-19 14:25:18 UTC)
#20
On 2013/08/19 14:12:04, mmenke wrote:
> I think you should take a look at HostResolverImpl, and make sure there are no
> places where the assumption that a valid config implies async dns is enabled,
as
> it's no longer true. For example:
>
> if (dns_config.IsValid())
> UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
>
> Now seems to be wrong (The lines above appear twice). Perhaps we should be
> querying the DnsClient instead?
Good catch. Changing this to HaveDnsConfig() will suffice. The only other place
querying IsValid() is received_dns_config_ which is there to measure the
effectiveness of DnsConfigService. Its meaning will change after this CL, but
I'd like to compare the UMA before and after to see what fraction of time we are
received a valid config with unhandled options. If you prefer, I can add an
explicit measurement for this. I'd probably change this histogram to an
enumeration.
>
> Another place that looks to be wrong is "had_dns_config_ =
> resolver_->HaveDnsConfig();" Variable value and name are correct, but it's
> assumed that had_dns_config_ is true if and only if we can use the async
> resolver.
>
> Should we have a unit test for this? If the current code doesn't crash when
we
> return a config with unhandled_options as true, I'm having trouble figuring
out
> why.
It does not crash because HaveDnsConfig queries the DnsClient and so it will
return false if there were unhandled_options (DnsClient just ignores such
config). This is as intended. I'll add a simple unit test for
DnsClient.SetConfig(<config with unhandled_options>).GetConfig() == NULL.
>
>
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
> File chrome/browser/net/dns_probe_service.cc (right):
>
>
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
> chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options =
> false;
> Wonder if we should just skip the probe in this case... Not a concern for
this
> CL.
On 2013/08/19 14:25:18, szym wrote: > On 2013/08/19 14:12:04, mmenke wrote: > > I think ...
7 years, 6 months ago
(2013-08-19 14:31:18 UTC)
#22
On 2013/08/19 14:25:18, szym wrote:
> On 2013/08/19 14:12:04, mmenke wrote:
> > I think you should take a look at HostResolverImpl, and make sure there are
no
> > places where the assumption that a valid config implies async dns is
enabled,
> as
> > it's no longer true. For example:
> >
> > if (dns_config.IsValid())
> > UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
> >
> > Now seems to be wrong (The lines above appear twice). Perhaps we should be
> > querying the DnsClient instead?
>
> Good catch. Changing this to HaveDnsConfig() will suffice. The only other
place
> querying IsValid() is received_dns_config_ which is there to measure the
> effectiveness of DnsConfigService. Its meaning will change after this CL, but
> I'd like to compare the UMA before and after to see what fraction of time we
are
> received a valid config with unhandled options. If you prefer, I can add an
> explicit measurement for this. I'd probably change this histogram to an
> enumeration.
>
> >
> > Another place that looks to be wrong is "had_dns_config_ =
> > resolver_->HaveDnsConfig();" Variable value and name are correct, but it's
> > assumed that had_dns_config_ is true if and only if we can use the async
> > resolver.
> >
> > Should we have a unit test for this? If the current code doesn't crash when
> we
> > return a config with unhandled_options as true, I'm having trouble figuring
> out
> > why.
>
> It does not crash because HaveDnsConfig queries the DnsClient and so it will
> return false if there were unhandled_options (DnsClient just ignores such
> config). This is as intended. I'll add a simple unit test for
> DnsClient.SetConfig(<config with unhandled_options>).GetConfig() == NULL.
Ahh, missed that the config is stored with the DnsSession, which SetConfig
doesn't create if the config isn't valid. Should probably update the comments
in dns_client.h, since they just talk about invalid configs.
>
> >
> >
>
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
> > File chrome/browser/net/dns_probe_service.cc (right):
> >
> >
>
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
> > chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options
=
> > false;
> > Wonder if we should just skip the probe in this case... Not a concern for
> this
> > CL.
7 years, 6 months ago
(2013-08-19 14:54:03 UTC)
#23
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options =
false;
On 2013/08/19 14:26:16, szym wrote:
> On 2013/08/19 14:12:04, mmenke wrote:
> > Wonder if we should just skip the probe in this case... Not a concern for
> this
> > CL.
>
> What does the prober do if there's no system config? If it fails gracefully,
> I'll remove this line.
DnsProbeRunner::RunProbe checks if the transaction factory is NULL, and if so,
passes along an "unknown" result.
7 years, 6 months ago
(2013-08-20 18:55:34 UTC)
#24
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):
https://codereview.chromium.org/21368005/diff/125001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:137: system_config.unhandled_options =
false;
On 2013/08/19 14:54:03, mmenke wrote:
> On 2013/08/19 14:26:16, szym wrote:
> > On 2013/08/19 14:12:04, mmenke wrote:
> > > Wonder if we should just skip the probe in this case... Not a concern for
> > this
> > > CL.
> >
> > What does the prober do if there's no system config? If it fails gracefully,
> > I'll remove this line.
>
> DnsProbeRunner::RunProbe checks if the transaction factory is NULL, and if so,
> passes along an "unknown" result.
Removed.
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 ...
7 years, 6 months ago
(2013-08-20 19:11:26 UTC)
#25
Issue 21368005: Detect domain-specific resolvers on OS X and disable DnsClient in such cases.
(Closed)
Created 7 years, 7 months ago by szym
Modified 7 years, 6 months ago
Reviewers: Mark Mentovai, mmenke
Base URL: svn://svn.chromium.org/chrome/trunk/src
Comments: 65