[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=

Review URL: https://chromiumcodereview.appspot.com/10389098

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@136681 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 79 insertions, 66 deletions

diff --git a/jingle/notifier/base/chrome_async_socket.cc b/jingle/notifier/base/chrome_async_socket.cc
index 1fba65c..ca2fde4 100644
--- a/jingle/notifier/base/chrome_async_socket.cc
+++ b/jingle/notifier/base/chrome_async_socket.cc
@@ -19,7 +19,6 @@
 #include "net/base/io_buffer.h"
 #include "net/base/net_util.h"
 #include "net/base/ssl_config_service.h"
-#include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/tcp_client_socket.h"
@@ -28,14 +27,14 @@
 namespace notifier {
 
 ChromeAsyncSocket::ChromeAsyncSocket(
-    ResolvingClientSocketFactory* client_socket_factory,
+    ResolvingClientSocketFactory* resolving_client_socket_factory,
     size_t read_buf_size,
     size_t write_buf_size)
-    : client_socket_factory_(client_socket_factory),
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      resolving_client_socket_factory_(resolving_client_socket_factory),
       state_(STATE_CLOSED),
       error_(ERROR_NONE),
       net_error_(net::OK),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       read_state_(IDLE),
       read_buf_(new net::IOBufferWithSize(read_buf_size)),
       read_start_(0U),
@@ -43,7 +42,7 @@ ChromeAsyncSocket::ChromeAsyncSocket(
       write_state_(IDLE),
       write_buf_(new net::IOBufferWithSize(write_buf_size)),
       write_end_(0U) {
-  DCHECK(client_socket_factory_.get());
+  DCHECK(resolving_client_socket_factory_.get());
   DCHECK_GT(read_buf_size, 0U);
   DCHECK_GT(write_buf_size, 0U);
 }
@@ -91,8 +90,7 @@ bool ChromeAsyncSocket::Connect(const talk_base::SocketAddress& address) {
     DoNonNetError(ERROR_WRONGSTATE);
     return false;
   }
-  // We can't work with an empty hostname and IP address.
-  if (address.hostname().empty() && (address.ip() == 0)) {
+  if (address.hostname().empty() || address.port() == 0) {
     DoNonNetError(ERROR_DNS);
     return false;
   }
@@ -103,16 +101,17 @@ bool ChromeAsyncSocket::Connect(const talk_base::SocketAddress& address) {
 
   state_ = STATE_CONNECTING;
 
-  DCHECK_EQ(false, weak_factory_.HasWeakPtrs());
+  DCHECK(!weak_ptr_factory_.HasWeakPtrs());
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
-  net::HostPortPair dest_host_port_pair(address.IPAsString(), address.port());
+  net::HostPortPair dest_host_port_pair(address.hostname(), address.port());
 
   transport_socket_.reset(
-      client_socket_factory_->CreateTransportClientSocket(
+      resolving_client_socket_factory_->CreateTransportClientSocket(
           dest_host_port_pair));
   int status = transport_socket_->Connect(
       base::Bind(&ChromeAsyncSocket::ProcessConnectDone,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   if (status != net::ERR_IO_PENDING) {
     // We defer execution of ProcessConnectDone instead of calling it
     // directly here as the caller may not expect an error/close to
@@ -120,9 +119,10 @@ bool ChromeAsyncSocket::Connect(const talk_base::SocketAddress& address) {
     // the connect always happens asynchronously.
     MessageLoop* message_loop = MessageLoop::current();
     CHECK(message_loop);
-    message_loop->PostTask(FROM_HERE,
-                           base::Bind(&ChromeAsyncSocket::ProcessConnectDone,
-                                      weak_factory_.GetWeakPtr(), status));
+    message_loop->PostTask(
+        FROM_HERE,
+        base::Bind(&ChromeAsyncSocket::ProcessConnectDone,
+                   weak_ptr_factory_.GetWeakPtr(), status));
   }
   return true;
 }
@@ -158,7 +158,8 @@ void ChromeAsyncSocket::PostDoRead() {
   CHECK(message_loop);
   message_loop->PostTask(
       FROM_HERE,
-      base::Bind(&ChromeAsyncSocket::DoRead, weak_factory_.GetWeakPtr()));
+      base::Bind(&ChromeAsyncSocket::DoRead,
+                 weak_ptr_factory_.GetWeakPtr()));
   read_state_ = POSTED;
 }
 
@@ -177,7 +178,7 @@ void ChromeAsyncSocket::DoRead() {
       transport_socket_->Read(
           read_buf_.get(), read_buf_->size(),
           base::Bind(&ChromeAsyncSocket::ProcessReadDone,
-                     base::Unretained(this)));
+                     weak_ptr_factory_.GetWeakPtr()));
   read_state_ = PENDING;
   if (status != net::ERR_IO_PENDING) {
     ProcessReadDone(status);
@@ -291,7 +292,8 @@ void ChromeAsyncSocket::PostDoWrite() {
   CHECK(message_loop);
   message_loop->PostTask(
       FROM_HERE,
-      base::Bind(&ChromeAsyncSocket::DoWrite, weak_factory_.GetWeakPtr()));
+      base::Bind(&ChromeAsyncSocket::DoWrite,
+                 weak_ptr_factory_.GetWeakPtr()));
   write_state_ = POSTED;
 }
 
@@ -309,7 +311,7 @@ void ChromeAsyncSocket::DoWrite() {
       transport_socket_->Write(
           write_buf_.get(), write_end_,
           base::Bind(&ChromeAsyncSocket::ProcessWriteDone,
-                     base::Unretained(this)));
+                     weak_ptr_factory_.GetWeakPtr()));
   write_state_ = PENDING;
   if (status != net::ERR_IO_PENDING) {
     ProcessWriteDone(status);
@@ -360,7 +362,7 @@ bool ChromeAsyncSocket::Close() {
 // (not STATE_CLOSED) -> STATE_CLOSED
 
 void ChromeAsyncSocket::DoClose() {
-  weak_factory_.InvalidateWeakPtrs();
+  weak_ptr_factory_.InvalidateWeakPtrs();
   if (transport_socket_.get()) {
     transport_socket_->Disconnect();
   }
@@ -397,23 +399,25 @@ bool ChromeAsyncSocket::StartTls(const std::string& domain_name) {
   DCHECK_EQ(write_end_, 0U);
 
   // Clear out any posted DoRead() tasks.
-  weak_factory_.InvalidateWeakPtrs();
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
   DCHECK(transport_socket_.get());
-  net::ClientSocketHandle* socket_handle = new net::ClientSocketHandle();
+  scoped_ptr<net::ClientSocketHandle> socket_handle(
+      new net::ClientSocketHandle());
   socket_handle->set_socket(transport_socket_.release());
   transport_socket_.reset(
-      client_socket_factory_->CreateSSLClientSocket(
-          socket_handle, net::HostPortPair(domain_name, 443)));
+      resolving_client_socket_factory_->CreateSSLClientSocket(
+          socket_handle.release(), net::HostPortPair(domain_name, 443)));
   int status = transport_socket_->Connect(
       base::Bind(&ChromeAsyncSocket::ProcessSSLConnectDone,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   if (status != net::ERR_IO_PENDING) {
     MessageLoop* message_loop = MessageLoop::current();
     CHECK(message_loop);
-    message_loop->PostTask(FROM_HERE,
-                           base::Bind(&ChromeAsyncSocket::ProcessSSLConnectDone,
-                                      weak_factory_.GetWeakPtr(), status));
+    message_loop->PostTask(
+        FROM_HERE,
+        base::Bind(&ChromeAsyncSocket::ProcessSSLConnectDone,
+                   weak_ptr_factory_.GetWeakPtr(), status));
   }
   return true;
 }
diff --git a/jingle/notifier/base/chrome_async_socket.h b/jingle/notifier/base/chrome_async_socket.h
index d2df8df..8cf8e8d 100644
--- a/jingle/notifier/base/chrome_async_socket.h
+++ b/jingle/notifier/base/chrome_async_socket.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
@@ -34,10 +34,11 @@ class ResolvingClientSocketFactory;
 
 class ChromeAsyncSocket : public buzz::AsyncSocket {
  public:
-  // Takes ownership of |client_socket_factory|.
-  ChromeAsyncSocket(ResolvingClientSocketFactory* client_socket_factory,
-                    size_t read_buf_size,
-                    size_t write_buf_size);
+  // Takes ownership of |resolving_client_socket_factory|.
+  ChromeAsyncSocket(
+      ResolvingClientSocketFactory* resolving_client_socket_factory,
+      size_t read_buf_size,
+      size_t write_buf_size);
 
   // Does not raise any signals.
   virtual ~ChromeAsyncSocket();
@@ -62,8 +63,11 @@ class ChromeAsyncSocket : public buzz::AsyncSocket {
   // If state() is not STATE_CLOSED, sets error to ERROR_WRONGSTATE
   // and returns false.
   //
-  // If |address| is not resolved, sets error to ERROR_DNS and returns
-  // false.
+  // If |address| has an empty hostname or a zero port, sets error to
+  // ERROR_DNS and returns false.  (We don't use the IP address even
+  // if it's present, as DNS resolution is done by
+  // |resolving_client_socket_factory_|.  But it's perfectly fine if
+  // the hostname is a stringified IP address.)
   //
   // Otherwise, starts the connection process and returns true.
   // SignalConnected will be raised when the connection is successful;
@@ -176,19 +180,15 @@ class ChromeAsyncSocket : public buzz::AsyncSocket {
   // Close functions.
   void DoClose();
 
-  scoped_ptr<ResolvingClientSocketFactory> client_socket_factory_;
+  base::WeakPtrFactory<ChromeAsyncSocket> weak_ptr_factory_;
+  scoped_ptr<ResolvingClientSocketFactory> resolving_client_socket_factory_;
 
   // buzz::AsyncSocket state.
   buzz::AsyncSocket::State state_;
   buzz::AsyncSocket::Error error_;
   net::Error net_error_;
 
-  // Used by read/write loops.
-  base::WeakPtrFactory<ChromeAsyncSocket> weak_factory_;
-
   // NULL iff state() == STATE_CLOSED.
-  //
-  // TODO(akalin): Use ClientSocketPool.
   scoped_ptr<net::StreamSocket> transport_socket_;
 
   // State for the read loop.  |read_start_| <= |read_end_| <=
diff --git a/jingle/notifier/base/chrome_async_socket_unittest.cc b/jingle/notifier/base/chrome_async_socket_unittest.cc
index 0b1af48..00eb8ac 100644
--- a/jingle/notifier/base/chrome_async_socket_unittest.cc
+++ b/jingle/notifier/base/chrome_async_socket_unittest.cc
@@ -11,15 +11,17 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
-#include "base/sys_byteorder.h"
 #include "jingle/notifier/base/resolving_client_socket_factory.h"
+#include "net/base/address_list.h"
 #include "net/base/mock_cert_verifier.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_util.h"
 #include "net/base/ssl_config_service.h"
 #include "net/socket/socket_test_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "talk/base/sigslot.h"
+#include "talk/base/ipaddress.h"
 #include "talk/base/socketaddress.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -98,22 +100,6 @@ class AsyncSocketDataProvider : public net::SocketDataProvider {
   DISALLOW_COPY_AND_ASSIGN(AsyncSocketDataProvider);
 };
 
-// Takes a 32-bit integer in host byte order and converts it to a
-// net::IPAddressNumber.
-net::IPAddressNumber Uint32ToIPAddressNumber(uint32 ip) {
-  uint32 ip_nbo = base::HostToNet32(ip);
-  const unsigned char* const ip_start =
-      reinterpret_cast<const unsigned char*>(&ip_nbo);
-  return net::IPAddressNumber(ip_start, ip_start + (sizeof ip_nbo));
-}
-
-net::AddressList SocketAddressToAddressList(
-    const talk_base::SocketAddress& address) {
-  DCHECK_NE(address.ip(), 0U);
-  return net::AddressList::CreateFromIPAddress(
-      Uint32ToIPAddressNumber(address.ip()), address.port());
-}
-
 class MockXmppClientSocketFactory : public ResolvingClientSocketFactory {
  public:
   MockXmppClientSocketFactory(
@@ -153,7 +139,7 @@ class ChromeAsyncSocketTest
  protected:
   ChromeAsyncSocketTest()
       : ssl_socket_data_provider_(net::ASYNC, net::OK),
-        addr_(0xaabbccdd, 35) {}
+        addr_("localhost", 35) {}
 
   virtual ~ChromeAsyncSocketTest() {}
 
@@ -165,10 +151,15 @@ class ChromeAsyncSocketTest
     mock_client_socket_factory->AddSSLSocketDataProvider(
         &ssl_socket_data_provider_);
 
+    // Fake DNS resolution for |addr_| and pass it to the factory.
+    net::IPAddressNumber resolved_addr;
+    EXPECT_TRUE(net::ParseIPLiteralToNumber("127.0.0.1", &resolved_addr));
+    const net::AddressList address_list =
+        net::AddressList::CreateFromIPAddress(resolved_addr, addr_.port());
     scoped_ptr<MockXmppClientSocketFactory> mock_xmpp_client_socket_factory(
         new MockXmppClientSocketFactory(
             mock_client_socket_factory.release(),
-            SocketAddressToAddressList(addr_)));
+            address_list));
     chrome_async_socket_.reset(
         new ChromeAsyncSocket(mock_xmpp_client_socket_factory.release(),
                               14, 20)),
@@ -468,9 +459,21 @@ TEST_F(ChromeAsyncSocketTest, DoubleClose) {
   ExpectClosed();
 }
 
-TEST_F(ChromeAsyncSocketTest, UnresolvedConnect) {
-  const talk_base::SocketAddress unresolved_addr(0, 0);
-  EXPECT_FALSE(chrome_async_socket_->Connect(unresolved_addr));
+TEST_F(ChromeAsyncSocketTest, NoHostnameConnect) {
+  talk_base::IPAddress ip_address;
+  EXPECT_TRUE(talk_base::IPFromString("127.0.0.1", &ip_address));
+  const talk_base::SocketAddress no_hostname_addr(ip_address, addr_.port());
+  EXPECT_FALSE(chrome_async_socket_->Connect(no_hostname_addr));
+  ExpectErrorState(ChromeAsyncSocket::STATE_CLOSED,
+                   ChromeAsyncSocket::ERROR_DNS);
+
+  EXPECT_TRUE(chrome_async_socket_->Close());
+  ExpectClosed();
+}
+
+TEST_F(ChromeAsyncSocketTest, ZeroPortConnect) {
+  const talk_base::SocketAddress zero_port_addr(addr_.hostname(), 0);
+  EXPECT_FALSE(chrome_async_socket_->Connect(zero_port_addr));
   ExpectErrorState(ChromeAsyncSocket::STATE_CLOSED,
                    ChromeAsyncSocket::ERROR_DNS);
 
diff --git a/jingle/notifier/base/proxy_resolving_client_socket.cc b/jingle/notifier/base/proxy_resolving_client_socket.cc
index ab89ffb..3c3d082c 100644
--- a/jingle/notifier/base/proxy_resolving_client_socket.cc
+++ b/jingle/notifier/base/proxy_resolving_client_socket.cc
@@ -44,6 +44,8 @@ ProxyResolvingClientSocket::ProxyResolvingClientSocket(
   net::URLRequestContext* request_context =
       request_context_getter->GetURLRequestContext();
   DCHECK(request_context);
+  DCHECK(!dest_host_port_pair_.host().empty());
+  DCHECK_GT(dest_host_port_pair_.port(), 0);
   net::HttpNetworkSession::Params session_params;
   session_params.client_socket_factory = socket_factory;
   session_params.host_resolver = request_context->host_resolver();
@@ -108,6 +110,7 @@ int ProxyResolvingClientSocket::Connect(
 
   // First we try and resolve the proxy.
   GURL url("http://" + dest_host_port_pair_.ToString());
+  DCHECK(url.is_valid());
   int status = network_session_->proxy_service()->ResolveProxy(
       url,
       &proxy_info_,
diff --git a/jingle/notifier/base/proxy_resolving_client_socket.h b/jingle/notifier/base/proxy_resolving_client_socket.h
index f94ac4c..88b488a 100644
--- a/jingle/notifier/base/proxy_resolving_client_socket.h
+++ b/jingle/notifier/base/proxy_resolving_client_socket.h
@@ -34,10 +34,12 @@ namespace notifier {
 
 class ProxyResolvingClientSocket : public net::StreamSocket {
  public:
-  // Constructs a new ProxyResolvingClientSocket. |socket_factory| is the
-  // ClientSocketFactory that will be used by the underlying HttpNetworkSession.
-  // If |socket_factory| is NULL, the default socket factory
-  // (net::ClientSocketFactory::GetDefaultFactory()) will be used.
+  // Constructs a new ProxyResolvingClientSocket. |socket_factory| is
+  // the ClientSocketFactory that will be used by the underlying
+  // HttpNetworkSession.  If |socket_factory| is NULL, the default
+  // socket factory (net::ClientSocketFactory::GetDefaultFactory())
+  // will be used.  |dest_host_port_pair| is the destination for this
+  // socket.  The hostname must be non-empty and the port must be > 0.
   ProxyResolvingClientSocket(
       net::ClientSocketFactory* socket_factory,
       const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
diff --git a/jingle/notifier/base/xmpp_client_socket_factory.cc b/jingle/notifier/base/xmpp_client_socket_factory.cc
index 87b439d..22ae27c 100644
--- a/jingle/notifier/base/xmpp_client_socket_factory.cc
+++ b/jingle/notifier/base/xmpp_client_socket_factory.cc
@@ -30,6 +30,7 @@ XmppClientSocketFactory::~XmppClientSocketFactory() {}
 
 net::StreamSocket* XmppClientSocketFactory::CreateTransportClientSocket(
     const net::HostPortPair& host_and_port) {
+  // TODO(akalin): Use socket pools.
   net::StreamSocket* transport_socket = new ProxyResolvingClientSocket(
       NULL,
       request_context_getter_,