[net/dns] Treat Name Resolution Policy Table as unhandled option.

[net/dns] Treat Name Resolution Policy Table as unhandled option.

Also, conservatively assume that non-empty NRPT or unknown DnsConfig could mean
DirectAccess is used, and disable IPv6 probing in that case.

BUG=259792
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223595

[cbentzel, 2013-09-16 10:11:49]

This should be associated with the DirectAccess bug.

Otherwise LGTM

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.h (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.h:107: CONFIG_PARSE_WIN_UNHANDLED_OPTIONS,
Did you add this new value simply to see UMA for how frequently this case
happens?

[mmenke, 2013-09-16 16:34:46]

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service.cc:79: dict->SetBoolean("use_local_ipv6",
use_local_ipv6);
nit: Should update the DNS_CONFIG_CHANGED description in net_log_event_types.h. 
Maybe just replace the documentation with a pointer to DnsConfig in
dns_config_service.h? Not sure how much value there is in duplicated the
documentation in the two files.  Does make it a little easier to look events in
general, but how often do people do that?

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service.h:89: // DnsClient and getaddrinfo.
Think the comment is unclear - both sentences, but particularly the second.

Suggestion for the second sentence "This is exposed for HostResolver to skip
IPv6 probes, as it may cause them to return an incorrect results."

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.cc:358: DnsConfig* config) {
I'm assuming this all exactly the same as before, except "return
CONFIG_PARSE_WIN_OK;" was replaced with "return;"?

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.h (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.h:107: CONFIG_PARSE_WIN_UNHANDLED_OPTIONS,
You should add this to histograms.xml in this CL, too.  Now that the histograms
are public, no need for a separate CL.

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win_unittest.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win_unittest.cc:10: #include
"testing/gtest/include/gtest/gtest.h"
Think we should be including basictypes.h for arraysize.

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1833: use_local_ipv6_ = !dns_config.IsValid() ||
dns_config.use_local_ipv6;
What are the latest numbers on invalid DNS configs on windows?  Reluctant to
suggest more complexity, but we could just check if we have any non-loopback
IPv6 address as a fallback, if it's significant.

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:2146: if (probe_ipv6_support_ && !use_local_ipv6_)
{
Should we be unit testing this, both in the case of setting it on start, and in
the case of it changing?

[szym, 2013-09-16 19:17:35]

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service.cc:79: dict->SetBoolean("use_local_ipv6",
use_local_ipv6);
On 2013/09/16 16:34:46, mmenke wrote:
> nit: Should update the DNS_CONFIG_CHANGED description in
net_log_event_types.h. 
> Maybe just replace the documentation with a pointer to DnsConfig in
> dns_config_service.h? Not sure how much value there is in duplicated the
> documentation in the two files.  Does make it a little easier to look events
in
> general, but how often do people do that?

I replaced the primitives with

<other>: <See DnsConfig>

This breaks the convention in net_log_event_types.h. Let me know if it's ok.

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service.h:89: // DnsClient and getaddrinfo.
On 2013/09/16 16:34:46, mmenke wrote:
> Think the comment is unclear - both sentences, but particularly the second.
> 
> Suggestion for the second sentence "This is exposed for HostResolver to skip
> IPv6 probes, as it may cause them to return an incorrect results."

Done.

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.cc:358: DnsConfig* config) {
On 2013/09/16 16:34:46, mmenke wrote:
> I'm assuming this all exactly the same as before, except "return
> CONFIG_PARSE_WIN_OK;" was replaced with "return;"?

correct

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.h (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.h:107: CONFIG_PARSE_WIN_UNHANDLED_OPTIONS,
On 2013/09/16 16:34:46, mmenke wrote:
> You should add this to histograms.xml in this CL, too.  Now that the
histograms
> are public, no need for a separate CL.

The CL initially had it, but I removed it so that it's easier to revert this
without touching histograms.xml. That said, I think once this lands, the enum
ConfigParseWinResult will need to stay anyway.

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.h:107: CONFIG_PARSE_WIN_UNHANDLED_OPTIONS,
On 2013/09/16 10:11:50, cbentzel wrote:
> Did you add this new value simply to see UMA for how frequently this case
> happens?

Yes. This follows dns_config_service_posix.*

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win_unittest.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/dns_config_servic...
net/dns/dns_config_service_win_unittest.cc:10: #include
"testing/gtest/include/gtest/gtest.h"
On 2013/09/16 16:34:46, mmenke wrote:
> Think we should be including basictypes.h for arraysize.

Done.

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:2146: if (probe_ipv6_support_ && !use_local_ipv6_)
{
On 2013/09/16 16:34:46, mmenke wrote:
> Should we be unit testing this, both in the case of setting it on start, and
in
> the case of it changing?

The probe uses ClientSocketFactory::GetDefaultFactory so it is not really
testable.

[mmenke, 2013-09-16 19:25:18]

LGTM!

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:2146: if (probe_ipv6_support_ && !use_local_ipv6_)
{
On 2013/09/16 19:17:35, szym wrote:
> On 2013/09/16 16:34:46, mmenke wrote:
> > Should we be unit testing this, both in the case of setting it on start, and
> in
> > the case of it changing?
> 
> The probe uses ClientSocketFactory::GetDefaultFactory so it is not really
> testable.

Ahh, right.  I'd only thought about the DNS config side of things, not the
issues involved with the probe.

[mmenke, 2013-09-16 20:57:57]

Also may want to add the no probes when no DNS config behavior to the
description.

[szym, 2013-09-16 23:41:27]

isherman: please approve change to histograms.xml

[szym, 2013-09-16 23:48:18]

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/23522036/diff/28001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1833: use_local_ipv6_ = !dns_config.IsValid() ||
dns_config.use_local_ipv6;
On 2013/09/16 16:34:46, mmenke wrote:
> What are the latest numbers on invalid DNS configs on windows?  Reluctant to
> suggest more complexity, but we could just check if we have any non-loopback
> IPv6 address as a fallback, if it's significant.

After ttuttle's fix, HaveDnsConfig hovers at 0.5%.

[szym, 2013-09-16 23:48:40]

isherman: PTAL histograms.xml

[Ilya Sherman, 2013-09-17 00:15:00]

histograms.xml lgtm

[commit-bot: I haz the power, 2013-09-17 01:41:15]

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

[szym, 2013-09-17 01:49:39]

FTR: I have confirmed that DirectAccess and NRPT manifest in the tested registry
keys by setting up a DirectAccess configuration using Windows Server 2012 +
Windows 7 Ultimate.

[commit-bot: I haz the power, 2013-09-17 06:47:12]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-09-17 12:24:12]

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

[commit-bot: I haz the power, 2013-09-17 12:24:55]

Change committed as 223595