[net/dns] Removes locking from DnsConfigServiceWin and adds local computer name.

[net/dns] Removes locking from DnsConfigServiceWin and adds local computer name.

Also adds hostname normalization to lower case when resolving from HOSTS.

BUG=112907, 118412, 118995
TEST=./net_unittests --gtest_filter=DnsConfigService*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128498

[mmenke, 2012-03-19 15:18:01]

http://codereview.chromium.org/9721002/diff/1/net/dns/dns_config_service_win.cc
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9721002/diff/1/net/dns/dns_config_service_win....
net/dns/dns_config_service_win.cc:129:
watcher_.StartWatching(key_.watch_event(), this);
nit:  Think this should be indented one more space.

http://codereview.chromium.org/9721002/diff/1/net/dns/dns_config_service_win....
net/dns/dns_config_service_win.cc:505: base::Unretained(this)))) {
nit:  Indent this to the open paren, if it fits, so it's clearer that this is an
argument of bind.

http://codereview.chromium.org/9721002/diff/1/net/dns/dns_config_service_win....
net/dns/dns_config_service_win.cc:552: UpdateService();
What if the last call to OnHostsChanged failed?

Same issue if this fails, but OnHostsChanged succeeds.

http://codereview.chromium.org/9721002/diff/1/net/dns/dns_config_service_win....
net/dns/dns_config_service_win.cc:617: 
nit:  Remove extra blank line.

[szym, 2012-03-20 16:06:22]

Thanks for the review, although I will need to rewrite much of this CL.  First,
there is no need to watch for local hostname changes as Windows will not apply
the change unless the computer is rebooted. Furthermore, it is incorrect to
return loopback addresses when querying for local hostname.
See http://crbug.com/112907

Since storing the up-to-date address information in DnsHosts would require an
equivalent of NetworkChangeNotifier::IPAddressObserver, I am considering calling
GetAdapterAddresses on demand, i.e., from within
HostResolverImpl::ServeFromHosts. I think that the complexity of adding a custom
call there is much less than dealing with it OnIPAddressChange. Note that all
resolutions for local hostname (and localhost) will be synchronous, so such jobs
cannot be aborted or canceled.

[mmenke, 2012-03-20 16:24:07]

On 2012/03/20 16:06:22, szym wrote:
> Thanks for the review, although I will need to rewrite much of this CL. 
First,
> there is no need to watch for local hostname changes as Windows will not apply
> the change unless the computer is rebooted. Furthermore, it is incorrect to
> return loopback addresses when querying for local hostname.
> See http://crbug.com/112907
> 
> Since storing the up-to-date address information in DnsHosts would require an
> equivalent of NetworkChangeNotifier::IPAddressObserver, I am considering
calling
> GetAdapterAddresses on demand, i.e., from within
> HostResolverImpl::ServeFromHosts. I think that the complexity of adding a
custom
> call there is much less than dealing with it OnIPAddressChange. Note that all
> resolutions for local hostname (and localhost) will be synchronous, so such
jobs
> cannot be aborted or canceled.

I believe GetAdapterAddresses can have pretty bad performance.  See
http://crbug.com/83508 (We don't actually use GetAdapterAddresses there, but
rather a function that was chosen instead for being less janky)

[szym, 2012-03-20 17:40:10]

Indeed, a comment from eroman on http://crrev.com/62050 indicates that it
could take a considerable time (~5ms) to enumerate all adapters. Even MSDN
admits: "*GetAdaptersAddresses* is implemented only as a synchronous
function call. The *GetAdaptersAddresses* function requires a significant
amount of network resources and time to complete since all of the low-level
network interface tables must be traversed."

In that case, we should definitely cache the result. So DnsConfigServiceWin
would probably become an IPAddressObserver. I think we should address
http://crbug.com/117824. We need to avoid unnecessarily aborting jobs.

On Tue, Mar 20, 2012 at 12:24 PM, <mmenke@chromium.org> wrote:

> On 2012/03/20 16:06:22, szym wrote:
>
>> Thanks for the review, although I will need to rewrite much of this CL.
>>
> First,
>
>> there is no need to watch for local hostname changes as Windows will not
>> apply
>> the change unless the computer is rebooted. Furthermore, it is incorrect
>> to
>> return loopback addresses when querying for local hostname.
>> See http://crbug.com/112907
>>
>
>  Since storing the up-to-date address information in DnsHosts would
>> require an
>> equivalent of NetworkChangeNotifier::**IPAddressObserver, I am
>> considering
>>
> calling
>
>> GetAdapterAddresses on demand, i.e., from within
>> HostResolverImpl::**ServeFromHosts. I think that the complexity of
>> adding a
>>
> custom
>
>> call there is much less than dealing with it OnIPAddressChange. Note that
>> all
>> resolutions for local hostname (and localhost) will be synchronous, so
>> such
>>
> jobs
>
>> cannot be aborted or canceled.
>>
>
> I believe GetAdapterAddresses can have pretty bad performance.  See
> http://crbug.com/83508 (We don't actually use GetAdapterAddresses there,
> but
> rather a function that was chosen instead for being less janky)
>
>
https://chromiumcodereview.**appspot.com/9721002/<https://chromiumcodereview....
>

[mmenke, 2012-03-20 17:53:20]

SGTM.

On Tue, Mar 20, 2012 at 1:39 PM, Szymon Jakubczak <szym@chromium.org> wrote:

> Indeed, a comment from eroman on http://crrev.com/62050 indicates that it
> could take a considerable time (~5ms) to enumerate all adapters. Even MSDN
> admits: "*GetAdaptersAddresses* is implemented only as a synchronous
> function call. The *GetAdaptersAddresses* function requires a significant
> amount of network resources and time to complete since all of the low-level
> network interface tables must be traversed."
>
> In that case, we should definitely cache the result. So
> DnsConfigServiceWin would probably become an IPAddressObserver. I think we
> should address http://crbug.com/117824. We need to avoid unnecessarily
> aborting jobs.
>
>
> On Tue, Mar 20, 2012 at 12:24 PM, <mmenke@chromium.org> wrote:
>
>> On 2012/03/20 16:06:22, szym wrote:
>>
>>> Thanks for the review, although I will need to rewrite much of this CL.
>>>
>> First,
>>
>>> there is no need to watch for local hostname changes as Windows will not
>>> apply
>>> the change unless the computer is rebooted. Furthermore, it is incorrect
>>> to
>>> return loopback addresses when querying for local hostname.
>>> See http://crbug.com/112907
>>>
>>
>>  Since storing the up-to-date address information in DnsHosts would
>>> require an
>>> equivalent of NetworkChangeNotifier::**IPAddressObserver, I am
>>> considering
>>>
>> calling
>>
>>> GetAdapterAddresses on demand, i.e., from within
>>> HostResolverImpl::**ServeFromHosts. I think that the complexity of
>>> adding a
>>>
>> custom
>>
>>> call there is much less than dealing with it OnIPAddressChange. Note
>>> that all
>>> resolutions for local hostname (and localhost) will be synchronous, so
>>> such
>>>
>> jobs
>>
>>> cannot be aborted or canceled.
>>>
>>
>> I believe GetAdapterAddresses can have pretty bad performance.  See
>> http://crbug.com/83508 (We don't actually use GetAdapterAddresses there,
>> but
>> rather a function that was chosen instead for being less janky)
>>
>>
https://chromiumcodereview.**appspot.com/9721002/<https://chromiumcodereview....
>>
>
>

[szym, 2012-03-21 19:53:26]

This should work now with the exception of lowercase normalization.

[mmenke, 2012-03-22 15:36:24]

On 2012/03/21 19:53:26, szym wrote:
> This should work now with the exception of lowercase normalization.

Did you mean to upload another version?

[szym, 2012-03-22 15:47:28]

On 2012/03/22 15:36:24, Matt Menke wrote:
> On 2012/03/21 19:53:26, szym wrote:
> > This should work now with the exception of lowercase normalization.
> 
> Did you mean to upload another version?

I'm currently adding lowercase normalization: to ParseDnsHosts,
DnsConfigServiceWin::HostsReader and HostResolverImpl::Resolve. Otherwise the
logic will stay the same.

[mmenke1, 2012-03-22 15:48:06]

On 2012/03/22 15:47:28, szym wrote:
> On 2012/03/22 15:36:24, Matt Menke wrote:
> > On 2012/03/21 19:53:26, szym wrote:
> > > This should work now with the exception of lowercase normalization.
> > 
> > Did you mean to upload another version?
> 
> I'm currently adding lowercase normalization: to ParseDnsHosts,
> DnsConfigServiceWin::HostsReader and HostResolverImpl::Resolve. Otherwise the
> logic will stay the same.

Weird...For some reason, I was just seeing patch set 1

[mmenke, 2012-03-22 15:48:47]

On 2012/03/22 15:48:06, mmenke wrote:
> On 2012/03/22 15:47:28, szym wrote:
> > On 2012/03/22 15:36:24, Matt Menke wrote:
> > > On 2012/03/21 19:53:26, szym wrote:
> > > > This should work now with the exception of lowercase normalization.
> > > 
> > > Did you mean to upload another version?
> > 
> > I'm currently adding lowercase normalization: to ParseDnsHosts,
> > DnsConfigServiceWin::HostsReader and HostResolverImpl::Resolve. Otherwise
the
> > logic will stay the same.
> 
> Weird...For some reason, I was just seeing patch set 1

Err, I can see set 2 now.  Not sure what was going on.

[mmenke, 2012-03-22 16:35:04]

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:502: // GetComputerNameEx and watches registry
for changes to local name.
It also watches the HOSTs file, which DnsHostsReaders don't normally do.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:515:
NetworkChangeNotifier::RemoveIPAddressObserver(this);
Shouldn't we just call Cancel()?

Alternatively, here and above, you could use DCHECKs to make sure we were
already cancelled and require Cancel be called before restarting the watch.  The
advantage of that approach is it's a little more robust against Watch() being
called again at unexpected times.  Up to you.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:585: dns_hosts_[DnsHostsKey(localname,
ADDRESS_FAMILY_IPV6)];
This isn't safe.  If we insert an entry into dns_hosts_ for local_ipv6, it could
invalidate |local_ipv4|.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:585: dns_hosts_[DnsHostsKey(localname,
ADDRESS_FAMILY_IPV6)];
Also, suggest you use pointers here instead of references.  Makes things
clearer.  You may even just want to store them on the stack instead, and at the
end, only modify dns_hosts_ if they aren't empty.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:595: adapter->FirstUnicastAddress;
nit:  4-space indent

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:600: address->Address.iSockaddrLength)) {
nit:  Think this would be a little easier to read if you aborted first, then had
the rest of the logic.  Means you don't have to nest quite so deep.

if (!blah) {
  return;
}

if (ipv4)
...

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:621: dns_hosts_[DnsHostsKey("localhost",
ADDRESS_FAMILY_IPV6)];
Again, suggest you use pointers.  Also think "loopback" would be a little
clearer, though it does make linebreaking a little uglier.

[szym, 2012-03-22 18:42:44]

Added lower-case normalization and addressed some of the comments cbentzel@ left
on dns_config_service.*.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:515:
NetworkChangeNotifier::RemoveIPAddressObserver(this);
On 2012/03/22 16:35:05, Matt Menke wrote:
> Shouldn't we just call Cancel()?

SerialWorker::Cancel() cannot be undone. Making it undoable without races with
the WorkerPool is tricky.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:585: dns_hosts_[DnsHostsKey(localname,
ADDRESS_FAMILY_IPV6)];
On 2012/03/22 16:35:05, Matt Menke wrote:
> This isn't safe.  If we insert an entry into dns_hosts_ for local_ipv6, it
could
> invalidate |local_ipv4|.

The references (like iterators) in std::map are not affected by other
insertions. http://kera.name/articles/2011/06/iterator-invalidation-rules/

That said, I don't think this optimization (avoiding looking into the map more
than once) is worth the complexity.

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:621: dns_hosts_[DnsHostsKey("localhost",
ADDRESS_FAMILY_IPV6)];
On 2012/03/22 16:35:05, Matt Menke wrote:
> Again, suggest you use pointers.  Also think "loopback" would be a little
> clearer, though it does make linebreaking a little uglier.

I re-discovered that std::map::insert will not overwrite an existing value.
Which is exactly what I tried to accomplish here.

[mmenke, 2012-03-22 19:36:08]

LGTM

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9721002/diff/15003/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:585: dns_hosts_[DnsHostsKey(localname,
ADDRESS_FAMILY_IPV6)];
On 2012/03/22 18:42:45, szym wrote:
> On 2012/03/22 16:35:05, Matt Menke wrote:
> > This isn't safe.  If we insert an entry into dns_hosts_ for local_ipv6, it
> could
> > invalidate |local_ipv4|.
> 
> The references (like iterators) in std::map are not affected by other
> insertions. http://kera.name/articles/2011/06/iterator-invalidation-rules/
> 
> That said, I don't think this optimization (avoiding looking into the map more
> than once) is worth the complexity.

I had not realized that  Very handy, though I agree that it's not worth the
complexity.

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1809: return
Key(StringToLowerASCII(info.hostname()),
May want to have a unit test that depends on this.

http://codereview.chromium.org/9721002/diff/22001/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/9721002/diff/22001/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:164:
reinterpret_cast<PIP_ADAPTER_ADDRESSES>(malloc(len)));
nit:  Think this will all fit on one line.  Next line might, too.

http://codereview.chromium.org/9721002/diff/22001/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:590: adapter != NULL && !(have_ipv4 &&
have_ipv6);
nit:  Think (!have_ipv4 || !have_ipv6) is a little easier to cognitively follow.
 If you disagree, feel free to leave as-is.

http://codereview.chromium.org/9721002/diff/22001/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:614: }
You could simplify this as well.  I don't think the speed difference matters if
we just use insert here as well, and ditch have_ipv4 and have_ipv6 entirely,
though it wouldn't really simplify things that much.

http://codereview.chromium.org/9721002/diff/22001/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:625: dns_hosts_.insert(
You should add a comment that this will not overwrite any pre-existing entries
from the HOSTs file.

[cbentzel, 2012-03-22 19:45:15]

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1809: return
Key(StringToLowerASCII(info.hostname()),
I'm not sure you want to do this here. It looks like NetBIOS is case-sensitive
and this would get impacted.

[mmenke, 2012-03-22 19:55:09]

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1809: return
Key(StringToLowerASCII(info.hostname()),
On 2012/03/22 19:45:15, cbentzel wrote:
> I'm not sure you want to do this here. It looks like NetBIOS is case-sensitive
> and this would get impacted.

Err...wait...Doesn't GURL automatically lowercase URLs?  I try to visit
http://FLYING_GOAT/, and end up at http://flying_goat/

[cbentzel, 2012-03-22 19:56:37]

On Thu, Mar 22, 2012 at 3:55 PM, <mmenke@chromium.org> wrote:

>
> http://codereview.chromium.**org/9721002/diff/22001/net/**
>
base/host_resolver_impl.cc<http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc>
> File net/base/host_resolver_impl.cc (right):
>
> http://codereview.chromium.**org/9721002/diff/22001/net/**
>
base/host_resolver_impl.cc#**newcode1809<http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc#newcode1809>
> net/base/host_resolver_impl.**cc:1809: return
> Key(StringToLowerASCII(info.**hostname()),
> On 2012/03/22 19:45:15, cbentzel wrote:
>
>> I'm not sure you want to do this here. It looks like NetBIOS is
>>
> case-sensitive
>
>> and this would get impacted.
>>
>
> Err...wait...Doesn't GURL automatically lowercase URLs?  I try to visit
> http://FLYING_GOAT/, and end up at http://flying_goat/


Not sure. If we do lower-casing before this then I don't understand why
it's needed here.

>
>
>
http://codereview.chromium.**org/9721002/<http://codereview.chromium.org/9721...
>

[mmenke, 2012-03-22 19:58:35]

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1809: return
Key(StringToLowerASCII(info.hostname()),
On 2012/03/22 19:55:09, Matt Menke wrote:
> On 2012/03/22 19:45:15, cbentzel wrote:
> > I'm not sure you want to do this here. It looks like NetBIOS is
case-sensitive
> > and this would get impacted.
> 
> Err...wait...Doesn't GURL automatically lowercase URLs?  I try to visit
> http://FLYING_GOAT/, and end up at http://flying_goat/

Domains used by PAC script don't go through a GURL intermediary.  Not sure about
other cases.

[szym, 2012-03-22 20:04:03]

On 2012/03/22 19:45:15, cbentzel wrote:
>
http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl.cc
> File net/base/host_resolver_impl.cc (right):
> 
>
http://codereview.chromium.org/9721002/diff/22001/net/base/host_resolver_impl...
> net/base/host_resolver_impl.cc:1809: return
> Key(StringToLowerASCII(info.hostname()),
> I'm not sure you want to do this here. It looks like NetBIOS is case-sensitive
> and this would get impacted.

Good point. I chose to normalize here, because I wanted to make sure that the
HostCache is used in a case-insensitive way. That said, as mmenke pointed out,
GURL normalizes hostname, and I lack complete knowledge of who calls
HostResolverImpl::Resolve.

The main reason I'm adding normalization now is that the primary user of local
name is myIPAddress in src/net/proxy/proxy_resolver_js_bindings.h which attempts
to resolve the result |gethostname()|, which is upper-case (NetBIOS-compatible)
on Windows.

So there are a few options here:
1) Convert to lowercase in ServeFromHosts. I confirmed that Windows and Linux
are case insensitive when doing HOSTS lookups.
2) Convert to lowercase in myIPAddress. I confirmed that Windows ignores case
when doing local name lookups.

[szym, 2012-03-22 22:28:44]

> 1) Convert to lowercase in ServeFromHosts. I confirmed that Windows and Linux
> are case insensitive when doing HOSTS lookups.

After talking to Chris, this is the option I implemented. If there are no
objections, I'll flip the CQ switch tomorrow morning.

[cbentzel, 2012-03-23 01:06:16]

Sounds good.

On Thu, Mar 22, 2012 at 6:28 PM, <szym@chromium.org> wrote:

> 1) Convert to lowercase in ServeFromHosts. I confirmed that Windows and
>> Linux
>> are case insensitive when doing HOSTS lookups.
>>
>
> After talking to Chris, this is the option I implemented. If there are no
> objections, I'll flip the CQ switch tomorrow morning.
>
>
>
>
http://codereview.chromium.**org/9721002/<http://codereview.chromium.org/9721...
>

[commit-bot: I haz the power, 2012-03-23 14:16:22]

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

[commit-bot: I haz the power, 2012-03-23 16:17:08]

Change committed as 128498