HostResolverImpl will return ERR_NAME_NOT_RESOLVED instead of Ok if address list is empty

HostResolverImpl will return ERR_NAME_NOT_RESOLVED instead of Ok if address list is empty

BUG=224248
TEST=net_unittests --gtest_filter=HostResolverImplTest.EmptyListMeansNameNotResolved
NOTRY=true

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

[mef, 2013-05-03 21:16:45]

Hi Szymon,

could you take a look? I had to replace DCHECK with if() in
net::HostResolverImpl::ProcTask::OnLookupComplete to fix net_unittests crash.

thanks,
-m

[szym, 2013-05-04 07:46:32]

The realization that we had a DCHECK there makes me concerned that this won't
make much of a difference, but I guess it's worth a shot.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (left):

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:685: DCHECK(error || !results.empty());
Ok, hold on a second. If this DCHECK is here, then this means getaddrinfo never
gives us an empty address list and error == OK.

Now, DCHECKs are disabled in release builds, so maybe we're just not catching
it.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:685: // empty results are not OK
nit: Comments should be full sentences, with capital letter and full stop at
end.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:687: error = ERR_NAME_NOT_RESOLVED;
If you change |error| here then your |empty_list_on_ok| below will always be
false. (OnLookupComplete -> OnProcTaskComplete)

I'd suggest keeping it here, but also histogramming it here.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1443:
UMA_HISTOGRAM_BOOLEAN("Net.DNS_EmptyListOnOk", empty_list_on_ok);
I'd suggest putting the histogram in DNS.
Also, what do you want to count here? What fraction of all calls to getaddrinfo
returned OK and empty address list?

Then a name DNS.EmptyAddressListAndNoError would be appropriate.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1444: if (empty_list_on_ok) {
nit: no need for braces

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl_un...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl_un...
net/dns/host_resolver_impl_unittest.cc:540: EXPECT_EQ(ERR_NAME_NOT_RESOLVED,
req->WaitForResult());
To be true to the test this should be:
EXPECT_NE(OK, ...)
and you should also confirm
EXPECT_EQ(0, req->NumberOfAddresses());

[mef, 2013-05-06 16:49:11]

Szymon,

please take another look, I've addressed your comments. I've added another
histogram, and I've added 1 and 2 at the end of the name to distinguish them. It
doesn't feel right though, what is usual policy on histogram names when you need
to collect same thing in different places?

thanks,
-m

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:685: // empty results are not OK
On 2013/05/04 07:46:32, szym wrote:
> nit: Comments should be full sentences, with capital letter and full stop at
> end.

Done.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:687: error = ERR_NAME_NOT_RESOLVED;
On 2013/05/04 07:46:32, szym wrote:
> If you change |error| here then your |empty_list_on_ok| below will always be
> false. (OnLookupComplete -> OnProcTaskComplete)
> 
> I'd suggest keeping it here, but also histogramming it here.

Done.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1443:
UMA_HISTOGRAM_BOOLEAN("Net.DNS_EmptyListOnOk", empty_list_on_ok);
On 2013/05/04 07:46:32, szym wrote:
> I'd suggest putting the histogram in DNS.
> Also, what do you want to count here? What fraction of all calls to
getaddrinfo
> returned OK and empty address list?
> 
> Then a name DNS.EmptyAddressListAndNoError would be appropriate.

Done.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1443:
UMA_HISTOGRAM_BOOLEAN("Net.DNS_EmptyListOnOk", empty_list_on_ok);
On 2013/05/04 07:46:32, szym wrote:
> I'd suggest putting the histogram in DNS.
> Also, what do you want to count here? What fraction of all calls to
getaddrinfo
> returned OK and empty address list?
> 
> Then a name DNS.EmptyAddressListAndNoError would be appropriate.

Done.

https://codereview.chromium.org/14760008/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1444: if (empty_list_on_ok) {
On 2013/05/04 07:46:32, szym wrote:
> nit: no need for braces

Done.

[szym, 2013-05-06 20:07:02]

https://codereview.chromium.org/14760008/diff/6001/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/14760008/diff/6001/net/dns/host_resolver_impl...
net/dns/host_resolver_impl.cc:1444: bool empty_list_on_ok = (net_error == OK &&
addr_list.empty());
As I pointed out before, I don't think this can ever be true.

In line 689 you overwrite |error|. OnProcTaskComplete is actually called in line
745 (via callback_.Run) with the overwritten value.

My suggestion was to remove it from here entirely and leave it in
OnLookupComplete only.

[mef, 2013-05-07 14:53:40]

Szymon,

PTAL. Sorry for being so stubborn. :-)

thanks,
-m

https://codereview.chromium.org/14760008/diff/6001/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/14760008/diff/6001/net/dns/host_resolver_impl...
net/dns/host_resolver_impl.cc:1444: bool empty_list_on_ok = (net_error == OK &&
addr_list.empty());
Done. I left it here because my previous experiment with unit test showed it to
be effective, but I guess it was done after I removed DCHECK but before I've
added the same logic to OnLookupComplete.

On 2013/05/06 20:07:02, szym wrote:
> As I pointed out before, I don't think this can ever be true.
> 
> In line 689 you overwrite |error|. OnProcTaskComplete is actually called in
line
> 745 (via callback_.Run) with the overwritten value.
> 
> My suggestion was to remove it from here entirely and leave it in
> OnLookupComplete only.

[szym, 2013-05-07 15:09:56]

LGTM. Please set BUG=224248

You can also add
TEST=net_unittests
--gtest_filter=HostResolverImplTest.EmptyListMeansNameNotResolved

[commit-bot: I haz the power, 2013-05-07 15:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/14760008/12001

[commit-bot: I haz the power, 2013-05-07 15:30:11]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-07 20:07:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/14760008/12001

[commit-bot: I haz the power, 2013-05-07 21:13:02]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[szym, 2013-05-07 21:24:03]

Setting NOTRY=true to bypass issues with linux_rel trybot.

[commit-bot: I haz the power, 2013-05-07 21:24:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/14760008/12001

[commit-bot: I haz the power, 2013-05-07 21:24:25]

Change committed as 198799