Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1163903002:
Remove "Default Address Family" behavior from the HostResolver. (Closed)
|
Created:
5 years, 6 months ago by cbentzel Modified:
5 years, 6 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, arv+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove "Default Address Family" behavior from the HostResolver.
This behavior was used to either force IPv4 resolution on dual-stack devices (using the --disable-ipv6 command-line flag) or to force IPv6 resolution.
Now that the flags have gone away, this behavior is no longer needed in the HostResolver.
BUG=344685
Committed: https://crrev.com/1906f870be1bb55d447d101fc7ebac296a9cc9ce
Cr-Commit-Position: refs/heads/master@{#333062}
Patch Set 1 #Patch Set 2 : Diff relative to other changes #Patch Set 3 : Another rebase try #Patch Set 4 : Fix one test #Patch Set 5 : Fix all HostResolverImpl unit_tests #Patch Set 6 : Override IPv6 reachability tests for unit tests #
Total comments: 4
Patch Set 7 : No need for protected #
Total comments: 7
Patch Set 8 : Address Paul's comments #Patch Set 9 : Fix whitespace #Patch Set 10 : DualHosts #Patch Set 11 : Adjust comment on TestHostResolverImpl #
Total comments: 7
Patch Set 12 : TestHostResolverImpl style nits #
Messages
Total messages: 20 (4 generated)
cbentzel@chromium.org changed reviewers: + pauljensen@chromium.org
This uses the virtual method for the IPv6 reachability tests. I also considered bringing back the probe disabling logic in HostResolverImpl, but prefer to not have that logic in place for run-time code if it is only needed for test harnesses.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:150: virtual bool IsIPv6Reachable(const BoundNetLog& net_log); Drive-by nit: This doesn't have to be protected for subclasses to override it. https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:422: : HostResolverImpl(options, net_log) {} nit: Add virtual destructor.
https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... File net/dns/host_resolver_impl.h (right): https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... net/dns/host_resolver_impl.h:150: virtual bool IsIPv6Reachable(const BoundNetLog& net_log); On 2015/06/03 13:31:03, mmenke wrote: > Drive-by nit: This doesn't have to be protected for subclasses to override it. Done. https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/100001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:422: : HostResolverImpl(options, net_log) {} On 2015/06/03 13:31:03, mmenke wrote: > nit: Add virtual destructor. Done.
https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (left): https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1831: resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4); By removing this, have you stopped this test from testing the intended behavior? I think the behavior being tested relies on a restricted request, at least according to this comment: https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol... https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:419: class TestHostResolverImpl : public HostResolverImpl { How about adding a comment mentioning that use of this class prevents IPv6 probing? https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1457: CreateRequest("ok_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve()); Any particular reason you changed the priority from MEDIUM to IDLE?
https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (left): https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1831: resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4); On 2015/06/03 16:26:06, pauljensen wrote: > By removing this, have you stopped this test from testing the intended behavior? > I think the behavior being tested relies on a restricted request, at least > according to this comment: > https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol... Great catch. I noticed on my machine that localhost only resolved to 127.0.0.1 and most of the test is skipped. I actually want to understand why this test uses SystemHostResolverProc rather than mocking it out so it will actually run on all platforms. I'll try to get in touch with szym@ about it. https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:419: class TestHostResolverImpl : public HostResolverImpl { On 2015/06/03 16:26:06, pauljensen wrote: > How about adding a comment mentioning that use of this class prevents IPv6 > probing? Done. https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1457: CreateRequest("ok_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve()); On 2015/06/03 16:26:06, pauljensen wrote: > Any particular reason you changed the priority from MEDIUM to IDLE? Fewer characters to type. I'll change to MEDIUM since it obviously raised a question.
https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (left): https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1831: resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4); On 2015/06/03 22:58:08, cbentzel wrote: > On 2015/06/03 16:26:06, pauljensen wrote: > > By removing this, have you stopped this test from testing the intended > behavior? > > I think the behavior being tested relies on a restricted request, at least > > according to this comment: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol... > > Great catch. > > I noticed on my machine that localhost only resolved to 127.0.0.1 and most of > the test is skipped. > > I actually want to understand why this test uses SystemHostResolverProc rather > than mocking it out so it will actually run on all platforms. I'll try to get in > touch with szym@ about it. OK, I looked at this some more. The old version of the test acted as if the requests were AF_UNSPECIFIED, but the HostResolverImpl would choose an effective key of AF_IPV4 and set the host resolver flag that IPv6 was disabled - and made sure that localhost resolutions would still give AAAA in this case if the system host resolver did as well. The first request would go to the system getaddrinfo call. The second request would be handled out of HOSTS. By treating IPv6 probes as failing with the TestHostResolverImpl, the second request is actually handled identically with the new version of the code from the old version. But the first version does not go through this check because we treat "use_local_ipv6_" as true since there is no DnsConfig. So in that case we'd still keep the AF_UNSPECIFIED behavior on a dual host machine. Trying to figure out a good workaround for that without introducing runtime logic.
On 2015/06/04 13:06:42, cbentzel wrote: > https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... > File net/dns/host_resolver_impl_unittest.cc (left): > > https://codereview.chromium.org/1163903002/diff/120001/net/dns/host_resolver_... > net/dns/host_resolver_impl_unittest.cc:1831: > resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4); > On 2015/06/03 22:58:08, cbentzel wrote: > > On 2015/06/03 16:26:06, pauljensen wrote: > > > By removing this, have you stopped this test from testing the intended > > behavior? > > > I think the behavior being tested relies on a restricted request, at least > > > according to this comment: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol... > > > > Great catch. > > > > I noticed on my machine that localhost only resolved to 127.0.0.1 and most of > > the test is skipped. > > > > I actually want to understand why this test uses SystemHostResolverProc rather > > than mocking it out so it will actually run on all platforms. I'll try to get > in > > touch with szym@ about it. > > OK, I looked at this some more. > > The old version of the test acted as if the requests were AF_UNSPECIFIED, but > the HostResolverImpl would choose an effective key of AF_IPV4 and set the host > resolver flag that IPv6 was disabled - and made sure that localhost resolutions > would still give AAAA in this case if the system host resolver did as well. > > The first request would go to the system getaddrinfo call. > The second request would be handled out of HOSTS. > > By treating IPv6 probes as failing with the TestHostResolverImpl, the second > request is actually handled identically with the new version of the code from > the old version. > > But the first version does not go through this check because we treat > "use_local_ipv6_" as true since there is no DnsConfig. So in that case we'd > still keep the AF_UNSPECIFIED behavior on a dual host machine. > > Trying to figure out a good workaround for that without introducing runtime > logic. Latest patchset reproduces behavior from before. In particular: - The first time through, we go to the system HostResolverProc while specifying that there is no reachable IPv6 and do the localhost-only retry logic. - The second time through, we go through the ServeFromHosts logic while specifying that there is no reachable IPv6 and do the localhost-only retry logic. Happy to talk it through some more.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
Cool, it looks like you restored the behavior of that test. https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:426: : HostResolverImpl(options, net_log), ipv6_reachable_(true) {} nit: use C++11 delegated constructor so we don't forget to initialize member variables added in the future in all constructors. https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:436: bool ipv6_reachable_; nit: const https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1784: ChangeDnsConfig(config); Any particular reason you changed from this: ChangeDnsConfig(DnsConfig()) to this? DnsConfig config = CreateValidDnsConfig(); config.use_local_ipv6 = false; ChangeDnsConfig(config); info_proc.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); which also required adding: HostResolver::RequestInfo info_hosts(HostPortPair("localhost", 80)); info_hosts.set_address_family(ADDRESS_FAMILY_UNSPECIFIED); and also required renaming config->config_hosts?
Thanks for the review. https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:426: : HostResolverImpl(options, net_log), ipv6_reachable_(true) {} On 2015/06/05 14:58:47, pauljensen wrote: > nit: use C++11 delegated constructor so we don't forget to initialize member > variables added in the future in all constructors. Cool! As you can tell, I haven't been doing enough C++ development since Chromium adopted C++11. https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:436: bool ipv6_reachable_; On 2015/06/05 14:58:47, pauljensen wrote: > nit: const Done. https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1784: ChangeDnsConfig(config); On 2015/06/05 14:58:47, pauljensen wrote: > Any particular reason you changed from this: > ChangeDnsConfig(DnsConfig()) > to this? > DnsConfig config = CreateValidDnsConfig(); > config.use_local_ipv6 = false; > ChangeDnsConfig(config); > info_proc.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); > which also required adding: > HostResolver::RequestInfo info_hosts(HostPortPair("localhost", 80)); > info_hosts.set_address_family(ADDRESS_FAMILY_UNSPECIFIED); > and also required renaming config->config_hosts? Yes, there was a reason. The first part of this test validates that if the "proc" approach to resolution is used, we'll still try to get both A and AAAA results for names on localhost even if there is no globally reachable IPv6 DNS address. If there is not a valid DNS config, then HostResolverImpl::use_local_ipv6_ is conservatively set to true. This means that we will bypass the IPv6 reachability test in HostResolverImpl::GetEffectiveKeyForRequest - which also means that the initial request is ADDRESS_FAMILY_UNSPEC, rather than ADDRESS_FAMILY_IPV4 with HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6. That would mean that the test doesn't exercise what it means to exercise. So, I have to specify a valid DNS config so the IPv6 reachability test is done (and fails). But I also need to make sure that proc is used rather than the DnsTask approach, so it's why I need to also specify HOST_RESOLVER_SYSTEM_ONLY. ---- I then need to modify the config again later on so it includes the pretend HOSTS file - since the second part of the test is there to make sure that the host resolution gets retried for localhost addresses when there is no globally reachable IPv6 address.
https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... net/dns/host_resolver_impl_unittest.cc:1784: ChangeDnsConfig(config); On 2015/06/05 15:25:43, cbentzel wrote: > On 2015/06/05 14:58:47, pauljensen wrote: > > Any particular reason you changed from this: > > ChangeDnsConfig(DnsConfig()) > > to this? > > DnsConfig config = CreateValidDnsConfig(); > > config.use_local_ipv6 = false; > > ChangeDnsConfig(config); > > info_proc.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); > > which also required adding: > > HostResolver::RequestInfo info_hosts(HostPortPair("localhost", 80)); > > info_hosts.set_address_family(ADDRESS_FAMILY_UNSPECIFIED); > > and also required renaming config->config_hosts? > > Yes, there was a reason. > > The first part of this test validates that if the "proc" approach to resolution > is used, we'll still try to get both A and AAAA results for names on localhost > even if there is no globally reachable IPv6 DNS address. > > If there is not a valid DNS config, then HostResolverImpl::use_local_ipv6_ is > conservatively set to true. This means that we will bypass the IPv6 reachability > test in HostResolverImpl::GetEffectiveKeyForRequest - which also means that the > initial request is ADDRESS_FAMILY_UNSPEC, rather than ADDRESS_FAMILY_IPV4 with > HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6. That would mean that the test > doesn't exercise what it means to exercise. > > So, I have to specify a valid DNS config so the IPv6 reachability test is done > (and fails). But I also need to make sure that proc is used rather than the > DnsTask approach, so it's why I need to also specify HOST_RESOLVER_SYSTEM_ONLY. > > ---- > > I then need to modify the config again later on so it includes the pretend HOSTS > file - since the second part of the test is there to make sure that the host > resolution gets retried for localhost addresses when there is no globally > reachable IPv6 address. Ah, I didn't notice that DnsConfig's use_local_ipv6 is modified before setting into HostResolveImpl's use_local_ipv6. Why not just modify the original line 1849 UNSPECIFIED->IPv4?
On 2015/06/05 15:45:55, pauljensen wrote: > https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... > File net/dns/host_resolver_impl_unittest.cc (right): > > https://codereview.chromium.org/1163903002/diff/200001/net/dns/host_resolver_... > net/dns/host_resolver_impl_unittest.cc:1784: ChangeDnsConfig(config); > On 2015/06/05 15:25:43, cbentzel wrote: > > On 2015/06/05 14:58:47, pauljensen wrote: > > > Any particular reason you changed from this: > > > ChangeDnsConfig(DnsConfig()) > > > to this? > > > DnsConfig config = CreateValidDnsConfig(); > > > config.use_local_ipv6 = false; > > > ChangeDnsConfig(config); > > > info_proc.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); > > > which also required adding: > > > HostResolver::RequestInfo info_hosts(HostPortPair("localhost", 80)); > > > info_hosts.set_address_family(ADDRESS_FAMILY_UNSPECIFIED); > > > and also required renaming config->config_hosts? > > > > Yes, there was a reason. > > > > The first part of this test validates that if the "proc" approach to > resolution > > is used, we'll still try to get both A and AAAA results for names on localhost > > even if there is no globally reachable IPv6 DNS address. > > > > If there is not a valid DNS config, then HostResolverImpl::use_local_ipv6_ is > > conservatively set to true. This means that we will bypass the IPv6 > reachability > > test in HostResolverImpl::GetEffectiveKeyForRequest - which also means that > the > > initial request is ADDRESS_FAMILY_UNSPEC, rather than ADDRESS_FAMILY_IPV4 with > > HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6. That would mean that the test > > doesn't exercise what it means to exercise. > > > > So, I have to specify a valid DNS config so the IPv6 reachability test is done > > (and fails). But I also need to make sure that proc is used rather than the > > DnsTask approach, so it's why I need to also specify > HOST_RESOLVER_SYSTEM_ONLY. > > > > ---- > > > > I then need to modify the config again later on so it includes the pretend > HOSTS > > file - since the second part of the test is there to make sure that the host > > resolution gets retried for localhost addresses when there is no globally > > reachable IPv6 address. > > Ah, I didn't notice that DnsConfig's use_local_ipv6 is modified before setting > into HostResolveImpl's use_local_ipv6. > Why not just modify the original line 1849 UNSPECIFIED->IPv4? I suppose that could work if I also set HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 in the request - but it seems to be a bit less close to the actual runtime flow.
> > Why not just modify the original line 1849 UNSPECIFIED->IPv4? > > I suppose that could work if I also set > HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 in the request - but it seems to > be a bit less close to the actual runtime flow. lgtm either way. I like that your method is more realistic.
The CQ bit was checked by cbentzel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163903002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1906f870be1bb55d447d101fc7ebac296a9cc9ce Cr-Commit-Position: refs/heads/master@{#333062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
