Detect domain-specific resolvers on OS X and disable DnsClient in such cases.

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

[szym, 2013-08-06 15:05:32]

mmenke: PTAL chrome/ and net/

mark: PTAL third_party/ and net/dns/dns_config_service_posix.cc

[Mark Mentovai, 2013-08-06 15:41:00]

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, 2013-08-06 15:48:58]

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, 2013-08-06 15:56:16]

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, 2013-08-06 17:50:25]

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, 2013-08-06 18:05:28]

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_servic...
File net/dns/dns_config_service_posix.cc (right):

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:71:
DCHECK(dns_info_api().dns_configuration_free);
I think you’d want to be defensive and wrap the next line in an “if” instead of
or in addition to the DCHECK. The DCHECK won’t do anything in a release build. I
don’t think we actually want this to crash in release mode if
dns_configuration_notify_key and dns_configuration_copy are present but Apple
removes dns_configuration_free in some future version of the system. We’d maybe
want it to log something, but not to crash. Crashing in debug mode is fine.

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:173: if (resolver->options &&
!strcmp(resolver->options, "mdns"))
Do we know anything about the “options” format? Is it possible for it to contain
multiple options?

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:258: dns_config_.unhandled_options = true;
It seems like ReadDnsConfig should be responsible for setting this field.

https://codereview.chromium.org/21368005/diff/98001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/21368005/diff/98001/net/net.gyp#newcode1383
net/net.gyp:1383: 'third_party/apple_apsl/dnsinfo.h',
This path is wrong. You would want to use ../third_party.

You don’t really need to add this to the .gyp file at all, though.

https://codereview.chromium.org/21368005/diff/98001/third_party/apple_apsl/RE...
File third_party/apple_apsl/README.chromium (right):

https://codereview.chromium.org/21368005/diff/98001/third_party/apple_apsl/RE...
third_party/apple_apsl/README.chromium:62: 
extra blank line nit

[szym, 2013-08-06 22:14:19]

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 can remove this now.

Done.

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
File net/dns/dns_config_service_posix.cc (right):

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:71:
DCHECK(dns_info_api().dns_configuration_free);
On 2013/08/06 18:05:28, Mark Mentovai wrote:
> I think you’d want to be defensive and wrap the next line in an “if” instead
of
> or in addition to the DCHECK. The DCHECK won’t do anything in a release build.
I
> don’t think we actually want this to crash in release mode if
> dns_configuration_notify_key and dns_configuration_copy are present but Apple
> removes dns_configuration_free in some future version of the system. We’d
maybe
> want it to log something, but not to crash. Crashing in debug mode is fine.

Done.

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:173: if (resolver->options &&
!strcmp(resolver->options, "mdns"))
On 2013/08/06 18:05:28, Mark Mentovai wrote:
> Do we know anything about the “options” format? Is it possible for it to
contain
> multiple options?

http://opensource.apple.com/source/mDNSResponder/mDNSResponder-379.37/mDNSMac...
checks:

dns_resolver_t *r = ...;
if (r->port == 5353 || r->n_nameserver == 0) {
  char *opt = r->options;
  if (opt && !strncmp(opt, "mdns", strlen(opt)))
    // assume r is multicast
}

So this is for Tiger only. On newer Mac OS it will bail on the check for
n_nameserver in line 171.

https://codereview.chromium.org/21368005/diff/98001/net/dns/dns_config_servic...
net/dns/dns_config_service_posix.cc:258: dns_config_.unhandled_options = true;
On 2013/08/06 18:05:28, Mark Mentovai wrote:
> It seems like ReadDnsConfig should be responsible for setting this field.

Done, although I'm not sure it's easier to follow this way.

https://codereview.chromium.org/21368005/diff/98001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/21368005/diff/98001/net/net.gyp#newcode1383
net/net.gyp:1383: 'third_party/apple_apsl/dnsinfo.h',
On 2013/08/06 18:05:28, Mark Mentovai wrote:
> This path is wrong. You would want to use ../third_party.
> 
> You don’t really need to add this to the .gyp file at all, though.

Removed.

https://codereview.chromium.org/21368005/diff/98001/third_party/apple_apsl/RE...
File third_party/apple_apsl/README.chromium (right):

https://codereview.chromium.org/21368005/diff/98001/third_party/apple_apsl/RE...
third_party/apple_apsl/README.chromium:62: 
On 2013/08/06 18:05:28, Mark Mentovai wrote:
> extra blank line nit

Done.

[Mark Mentovai, 2013-08-06 22:27:25]

LGTM

[mmenke, 2013-08-07 00:45:13]

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, 2013-08-16 15:44:04]

mmenke: ping

[mmenke, 2013-08-16 15:50:48]

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, 2013-08-16 17:35:39]

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#ne...
net/dns/dns_client.cc:30: if (config.IsValid() && !config.unhandled_options) {
Is the second check needed?  We always return an error when we set
config.unhandled_options to true, so it seems like we could just DCHECK on it
here.  Or am I missing something?

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:26: #include
"third_party/apple_apsl/dnsinfo.h"
nit:  Blank line between system and non-system headers.  Also between headers
and namespace.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:41: if (!handle_) return;
Splirt onto two lines.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:42: dns_configuration_notify_key =
These should be initialized to NULL.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:52: ~DnsInfoApi() {
nit:  Blank line between functions.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:53: if (handle_) dlclose(handle_);
nit:  Should be on two different lines.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:65: base::LazyInstance<DnsInfoApi>::Leaky
api = LAZY_INSTANCE_INITIALIZER;
I can't find another instance where this is done - lazy instances are generally
just globals.  What's the motivation for the difference here?

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:77: #endif
nit:  #endif  // defined(OS_MACOSX)

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:93: if
(!dns_info_api().dns_configuration_notify_key) return false;
Split onto two lines.

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.
This comment isn't quite right - we actually do still get the resolvers from
res_state.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:170: for (int i = 0; i <
dns_config->n_resolver; ++i) {
Is there documentation on this data structure somewhere?  I'm not really sure of
the logic behind this.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:183: #endif
nit:  #endif  // defined(OS_MACOSX)

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:259: DCHECK(dns_config_.unhandled_options);
Think it's worth a comment why this shouldn't happen.

[szym, 2013-08-16 17:48:26]

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#ne...
net/dns/dns_client.cc:30: if (config.IsValid() && !config.unhandled_options) {
On 2013/08/16 17:35:39, mmenke wrote:
> Is the second check needed?  We always return an error when we set
> config.unhandled_options to true, so it seems like we could just DCHECK on it
> here.  Or am I missing something?

We still want to detect DNS changes, and for that DnsConfigService must not
fail. This CL introduces "unhandled_options" to be separate from whether
DnsConfigService is working. "unhandled_options = true" means, we got a
DnsConfig, but our DnsClient cannot fully execute it.

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:65: base::LazyInstance<DnsInfoApi>::Leaky
api = LAZY_INSTANCE_INITIALIZER;
On 2013/08/16 17:35:39, mmenke wrote:
> I can't find another instance where this is done - lazy instances are
generally
> just globals.  What's the motivation for the difference here?

There is no need for it to be global, and I'd prefer to hide it so that the
struct cannot be accidentally modified. (see mmentovai's comment
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
)

But the bug here is that it should be static.

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: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.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:170: for (int i = 0; i <
dns_config->n_resolver; ++i) {
On 2013/08/16 17:35:39, mmenke wrote:
> Is there documentation on this data structure somewhere?  I'm not really sure
of
> the logic behind this.

The source code is the only documentation. In particular, this is based on
http://opensource.apple.com/source/mDNSResponder/mDNSResponder-379.37/mDNSMac...
: ConfigResolvers

[mmenke, 2013-08-16 17:51:24]

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:259: DCHECK(dns_config_.unhandled_options);
On 2013/08/16 17:35:39, mmenke wrote:
> Think it's worth a comment why this shouldn't happen.

Sorry, misread this code.

[mmenke, 2013-08-16 18:24:19]

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, 2013-08-16 18:40:40]

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, 2013-08-16 18:43:26]

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.

[szym, 2013-08-17 05:08:50]

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:26: #include
"third_party/apple_apsl/dnsinfo.h"
On 2013/08/16 17:35:39, mmenke wrote:
> nit:  Blank line between system and non-system headers.  Also between headers
> and namespace.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:41: if (!handle_) return;
On 2013/08/16 17:35:39, mmenke wrote:
> Splirt onto two lines.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:42: dns_configuration_notify_key =
On 2013/08/16 17:35:39, mmenke wrote:
> These should be initialized to NULL.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:52: ~DnsInfoApi() {
On 2013/08/16 17:35:39, mmenke wrote:
> nit:  Blank line between functions.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:53: if (handle_) dlclose(handle_);
On 2013/08/16 17:35:39, mmenke wrote:
> nit:  Should be on two different lines.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:65: base::LazyInstance<DnsInfoApi>::Leaky
api = LAZY_INSTANCE_INITIALIZER;
On 2013/08/16 17:48:27, szym wrote:
> On 2013/08/16 17:35:39, mmenke wrote:
> > I can't find another instance where this is done - lazy instances are
> generally
> > just globals.  What's the motivation for the difference here?
> 
> There is no need for it to be global, and I'd prefer to hide it so that the
> struct cannot be accidentally modified. (see mmentovai's comment
>
https://codereview.chromium.org/21368005/diff/66001/net/dns/dns_config_servic...
> )
> 
> But the bug here is that it should be static. 

I added static.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:77: #endif
On 2013/08/16 17:35:39, mmenke wrote:
> nit:  #endif  // defined(OS_MACOSX)

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:93: if
(!dns_info_api().dns_configuration_notify_key) return false;
On 2013/08/16 17:35:39, mmenke wrote:
> Split onto two lines.

Done.

https://codereview.chromium.org/21368005/diff/105001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:183: #endif
On 2013/08/16 17:35:39, mmenke wrote:
> nit:  #endif  // defined(OS_MACOSX)

Done.

[mmenke, 2013-08-19 14:12:04]

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, 2013-08-19 14:25:18]

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.

[szym, 2013-08-19 14:26:15]

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: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.

[mmenke1, 2013-08-19 14:31:18]

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.

[mmenke, 2013-08-19 14:54:03]

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.

[szym, 2013-08-20 18:55:34]

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, 2013-08-20 19:11:26]

LGTM, modulo comments.

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
File net/dns/dns_config_service_posix.cc (right):

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:56: dlsym(handle_,
"dns_configuration_free"));
nit:  These all should be using 4-space indent, not 2.

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:72: const DnsInfoApi& dns_info_api() {
This function name isn't a simple accessor, at least on the first call, so
should probably shouldn't be named using this style.

[szym, 2013-08-20 19:30:41]

Thanks for the review.

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
File net/dns/dns_config_service_posix.cc (right):

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:56: dlsym(handle_,
"dns_configuration_free"));
On 2013/08/20 19:11:27, mmenke wrote:
> nit:  These all should be using 4-space indent, not 2.

Done.

https://codereview.chromium.org/21368005/diff/140001/net/dns/dns_config_servi...
net/dns/dns_config_service_posix.cc:72: const DnsInfoApi& dns_info_api() {
On 2013/08/20 19:11:27, mmenke wrote:
> This function name isn't a simple accessor, at least on the first call, so
> should probably shouldn't be named using this style.

Done.

[commit-bot: I haz the power, 2013-08-20 19:31:10]

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

[commit-bot: I haz the power, 2013-08-21 03:25:25]

Change committed as 218617