[Sync] Make ChromeAsyncSocket use only the hostname on connect

[Sync] Make ChromeAsyncSocket use only the hostname on connect

This is because DNS resolution should be done by its
ResolvingClientSocketFactory (which may resolve through a proxy).

This is just tightening up the preconditions as ChromeAsyncSocket's
callers were already providing the hostname.

Remove some uses of base::Unretained.

Some other minor cleanup.

BUG=
TEST=

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

[akalin, 2012-05-11 19:29:39]

+rlarocque for review

[rlarocque, 2012-05-11 20:04:18]

The change looks OK, except for that one concern in the test code.  

I'm curious, though: what do you have against IP addresses?  Is this just a case
of removing unused functionality, or has supporting IP addresses been a problem?

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
File jingle/notifier/base/chrome_async_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
jingle/notifier/base/chrome_async_socket_unittest.cc:157:
net::AddressList::CreateFromIPAddress(resolved_addr, addr_.port());
It's probably nothing, but this has me a bit concerned.

This line creates a sockaddr that ignores the hostname part of addr_.  That
address is used (and presumably stored somewhere) on line 161.  The original
addr_ is also stored, and later used in DoOpenClosed().  

Is it a problem that the MockXmppClientSocketFactory and DoOpenClosed use
different hostnames?  Why allow this inconsistency?

[akalin, 2012-05-11 20:32:20]

Addressed comments, PTAL

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
File jingle/notifier/base/chrome_async_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
jingle/notifier/base/chrome_async_socket_unittest.cc:157:
net::AddressList::CreateFromIPAddress(resolved_addr, addr_.port());
On 2012/05/11 20:04:18, rlarocque wrote:
> It's probably nothing, but this has me a bit concerned.
> 
> This line creates a sockaddr that ignores the hostname part of addr_.  That
> address is used (and presumably stored somewhere) on line 161.  The original
> addr_ is also stored, and later used in DoOpenClosed().  
> 
> Is it a problem that the MockXmppClientSocketFactory and DoOpenClosed use
> different hostnames?  Why allow this inconsistency?

sockaddrs don't store the hostname, but just the ip+port, right?  At least
Chrome's version doesn't (although I notice address_list has a canonical_name
member, it's deprecated.)

So basically DoOpenClosed uses hostname+port, and then the factory uses the
ip+port, which is faking the result of DNS resolution.

https://chromiumcodereview.appspot.com/10389098/diff/10001/jingle/notifier/ba...
File jingle/notifier/base/chrome_async_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10389098/diff/10001/jingle/notifier/ba...
jingle/notifier/base/chrome_async_socket_unittest.cc:154: // Fake DNS resolution
for |addr_| and pass it to the factory.
Added comment.

[akalin, 2012-05-11 20:35:01]

On 2012/05/11 20:04:18, rlarocque wrote:
> I'm curious, though: what do you have against IP addresses?  Is this just a
case
> of removing unused functionality, or has supporting IP addresses been a
problem?

It's basically unused functionality.  This code used to DNS-resolve itself, but
when proxy resolution was added, that code was bypassed (because the proxy
resolution code handles DNS resolution, even if there is no proxy).

This CL came out of http://codereview.chromium.org/10382003/ , where someone was
getting mysterious crashes.  It turned out to be because they changed that one
line to pass in the (empty) ip address instead of the hostname.  I think
strengthening the precondition would help prevent future errors.

[rlarocque, 2012-05-11 20:46:16]

Thanks for the explanation.  LGTM.

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
File jingle/notifier/base/chrome_async_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10389098/diff/7/jingle/notifier/base/c...
jingle/notifier/base/chrome_async_socket_unittest.cc:157:
net::AddressList::CreateFromIPAddress(resolved_addr, addr_.port());
On 2012/05/11 20:32:20, akalin wrote:
> On 2012/05/11 20:04:18, rlarocque wrote:
> > It's probably nothing, but this has me a bit concerned.
> > 
> > This line creates a sockaddr that ignores the hostname part of addr_.  That
> > address is used (and presumably stored somewhere) on line 161.  The original
> > addr_ is also stored, and later used in DoOpenClosed().  
> > 
> > Is it a problem that the MockXmppClientSocketFactory and DoOpenClosed use
> > different hostnames?  Why allow this inconsistency?
> 
> sockaddrs don't store the hostname, but just the ip+port, right?  At least
> Chrome's version doesn't (although I notice address_list has a canonical_name
> member, it's deprecated.)
> 
> So basically DoOpenClosed uses hostname+port, and then the factory uses the
> ip+port, which is faking the result of DNS resolution.

You're right, I was being a bit loose with the terminology.  A typical sockaddr
is just an IP address and port.  

I was concerned that the fake-resolved address might not match the results of a
DNS lookup of the hard-coded hostname.  However, if you're asserting that
"example.com" resolves to 127.0.0.1 (for the purposes of this test), then I
guess it's OK.

I'd feel slightly better about this if the hostname was "localhost", but I'm OK
with this either way.

[commit-bot: I haz the power, 2012-05-11 20:58:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10389098/7006

[akalin, 2012-05-11 20:58:45]

On 2012/05/11 20:46:16, rlarocque wrote:
> I'd feel slightly better about this if the hostname was "localhost", but I'm
OK
> with this either way.

That would be less confusing indeed.  Done.

Also added a comment clarifying that a hostname that is a stringified IP address
(e.g. "127.0.0.1") is ok.