Changed the jingle network code in ChromeAsyncSocket to use the client socket pool. This also all...

jingle/notifier/base/chrome_async_socket.cc

Index: jingle/notifier/base/chrome_async_socket.cc
===================================================================
--- jingle/notifier/base/chrome_async_socket.cc	(revision 81429)
+++ jingle/notifier/base/chrome_async_socket.cc	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -18,27 +18,35 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "jingle/notifier/base/fake_ssl_client_socket.h"
 #include "net/base/address_list.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_util.h"
 #include "net/base/ssl_config_service.h"
 #include "net/base/sys_addrinfo.h"
+#include "net/http/http_network_session.h"
 #include "net/socket/client_socket_factory.h"
+#include "net/socket/client_socket_handle.h"
+#include "net/socket/client_socket_pool_manager.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/tcp_client_socket.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "talk/base/socketaddress.h"
 
 namespace notifier {
 
 ChromeAsyncSocket::ChromeAsyncSocket(
-    net::ClientSocketFactory* client_socket_factory,
     const net::SSLConfig& ssl_config,
-    net::CertVerifier* cert_verifier,
+    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,

[akalin, 2011/04/13 20:49:32]

It seems to me that a socket shouldn't need to know about the full
URLRequestContextGetter.  In fact, shouldn't it suffice to pass in a
ClientSocketPool that it can request sockets from?  Or maybe
ClientSocketPoolBase/ConnectJob.  Basically, I think ChromeAsyncSocket should
simply call some magical "Connect" method that handles proxy resolution, ssl
handshake, etc. behind the scenes.

[sanjeevr, 2011/04/13 21:20:21]

This is not straightforward. Basically there is complicated logic about which
socket pools to request sockets from and the sockets are layered on top of each
other.

+    bool use_fake_ssl_client_socket,
     size_t read_buf_size,
     size_t write_buf_size,
     net::NetLog* net_log)
-    : connect_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
+    : proxy_resolve_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
+                              &ChromeAsyncSocket::ProcessProxyResolveDone),
+      connect_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
                         &ChromeAsyncSocket::ProcessConnectDone),
       read_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
                      &ChromeAsyncSocket::ProcessReadDone),
@@ -46,9 +54,11 @@
                      &ChromeAsyncSocket::ProcessWriteDone),
       ssl_connect_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
                             &ChromeAsyncSocket::ProcessSSLConnectDone),
-      client_socket_factory_(client_socket_factory),
+      fake_ssl_handshake_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),

[akalin, 2011/04/13 20:49:32]

i don't think ChromeAsyncSocket should know anything about FakeSSLClientSocket
-- it should get a ClientSocket(Handle) from somewhere and use that interface.

[sanjeevr, 2011/04/13 21:20:21]

Yes, we can supply this layer externally.

+                            &ChromeAsyncSocket::ProcessFakeSSLHandshakeDone),
       ssl_config_(ssl_config),
-      cert_verifier_(cert_verifier),
+      request_context_getter_(request_context_getter),
+      use_fake_ssl_client_socket_(use_fake_ssl_client_socket),
       bound_net_log_(
           net::BoundNetLog::Make(net_log, net::NetLog::SOURCE_SOCKET)),
       state_(STATE_CLOSED),
@@ -56,6 +66,8 @@
       net_error_(net::OK),
       scoped_runnable_method_factory_(
           ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      pac_request_(NULL),
+      tried_direct_connect_fallback_(false),
       read_state_(IDLE),
       read_buf_(new net::IOBufferWithSize(read_buf_size)),
       read_start_(0U),
@@ -63,9 +75,24 @@
       write_state_(IDLE),
       write_buf_(new net::IOBufferWithSize(write_buf_size)),
       write_end_(0U) {
-  DCHECK(client_socket_factory_.get());
+  DCHECK(request_context_getter_.get());
   DCHECK_GT(read_buf_size, 0U);
   DCHECK_GT(write_buf_size, 0U);
+  net::HttpNetworkSession::Params session_params;

[akalin, 2011/04/13 20:49:32]

It seems to me that you can at least construct the HttpNetworkSession outside of
the constructor and pass that in instead of the URLRequestContextGetter.

[sanjeevr, 2011/04/13 21:20:21]

This is also doable (though it is almost the same thing)

+  session_params.host_resolver =
+      request_context_getter_->GetURLRequestContext()->host_resolver();
+  session_params.cert_verifier =
+      request_context_getter_->GetURLRequestContext()->cert_verifier();
+  session_params.dnsrr_resolver =
+      request_context_getter_->GetURLRequestContext()->dnsrr_resolver();
+  session_params.proxy_service =
+      request_context_getter_->GetURLRequestContext()->proxy_service();
+  session_params.ssl_config_service =
+      request_context_getter_->GetURLRequestContext()->ssl_config_service();
+  session_params.http_auth_handler_factory =
+      request_context_getter_->GetURLRequestContext()->
+          http_auth_handler_factory();
+  network_session_ = new net::HttpNetworkSession(session_params);
 }
 
 ChromeAsyncSocket::~ChromeAsyncSocket() {}
@@ -103,6 +130,51 @@
   DoNetError(static_cast<net::Error>(status));
 }
 
+// STATE_CLOSED -> STATE_CONNECTING
+bool ChromeAsyncSocket::Connect(const talk_base::SocketAddress& address) {
+  // Bypass for unit-tests.
+  if (client_socket_factory_.get())
+    return ConnectUsingFactory(address);
+
+  if (state_ != STATE_CLOSED) {
+    LOG(DFATAL) << "Connect() called on non-closed socket";
+    DoNonNetError(ERROR_WRONGSTATE);
+    return false;
+  }
+
+  // We can't work with an empty hostname and IP address.
+  if (address.hostname().empty() && (address.ip() == 0)) {
+    DoNonNetError(ERROR_DNS);
+    return false;
+  }
+
+
+  DCHECK_EQ(state_, buzz::AsyncSocket::STATE_CLOSED);
+  DCHECK_EQ(read_state_, IDLE);
+  DCHECK_EQ(write_state_, IDLE);
+
+  state_ = STATE_CONNECTING;
+
+  DCHECK(scoped_runnable_method_factory_.empty());
+  scoped_runnable_method_factory_.RevokeAll();
+
+  // First we try and resolve the proxy.
+  dest_host_port_pair_ =
+      net::HostPortPair(address.IPAsString(), address.port());
+
+  GURL url = GURL("http://" + dest_host_port_pair_.ToString());
+  int status = request_context_getter_->GetURLRequestContext()->
+      proxy_service()->ResolveProxy(url,
+                                    &proxy_info_,
+                                    &proxy_resolve_callback_,
+                                    &pac_request_,
+                                    bound_net_log_);
+  if (status != net::ERR_IO_PENDING) {
+    ProcessProxyResolveDone(status);
+  }
+  return true;
+}
+
 namespace {
 
 // Takes a 32-bit integer in host byte order and converts it to a
@@ -123,14 +195,14 @@
 
 }  // namespace
 
-// STATE_CLOSED -> STATE_CONNECTING
-
-bool ChromeAsyncSocket::Connect(const talk_base::SocketAddress& address) {
+bool ChromeAsyncSocket::ConnectUsingFactory(
+    const talk_base::SocketAddress& address) {
   if (state_ != STATE_CLOSED) {
     LOG(DFATAL) << "Connect() called on non-closed socket";
     DoNonNetError(ERROR_WRONGSTATE);
     return false;
   }
+
   if (address.ip() == 0) {
     DoNonNetError(ERROR_DNS);
     return false;
@@ -146,10 +218,12 @@
   scoped_runnable_method_factory_.RevokeAll();
 
   net::AddressList address_list = SocketAddressToAddressList(address);
-  transport_socket_.reset(
+  net::ClientSocket* transport_socket =
       client_socket_factory_->CreateTransportClientSocket(
-          address_list, bound_net_log_.net_log(), net::NetLog::Source()));
-  int status = transport_socket_->Connect(&connect_callback_);
+          address_list, bound_net_log_.net_log(), net::NetLog::Source());
+  int status = transport_socket->Connect(&connect_callback_);
+  connection_.reset(new net::ClientSocketHandle);
+  connection_->set_socket(transport_socket);
   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
@@ -165,6 +239,64 @@
   return true;
 }
 
+
+void ChromeAsyncSocket::ProcessProxyResolveDone(int status) {
+  DCHECK_NE(status, net::ERR_IO_PENDING);
+  DCHECK_EQ(read_state_, IDLE);
+  DCHECK_EQ(write_state_, IDLE);
+  DCHECK_EQ(state_, STATE_CONNECTING);
+  if (status == net::OK) {
+    // Remove unsupported proxies from the list.
+    proxy_info_.RemoveProxiesWithoutScheme(
+        net::ProxyServer::SCHEME_DIRECT |
+        net::ProxyServer::SCHEME_HTTP | net::ProxyServer::SCHEME_HTTPS |
+        net::ProxyServer::SCHEME_SOCKS4 | net::ProxyServer::SCHEME_SOCKS5);
+
+    if (proxy_info_.is_empty()) {
+      // No proxies/direct to choose from. This happens when we don't support
+      // any of the proxies in the returned list.
+      status = net::ERR_NO_SUPPORTED_PROXIES;
+    }
+  }
+
+  // Since we are faking the URL, it is possible that no proxies match our URL.
+  // Try falling back to a direct connection if we have not tried that before.
+  if (status != net::OK) {
+    if (!tried_direct_connect_fallback_) {
+      tried_direct_connect_fallback_ = true;
+      proxy_info_.UseDirect();
+    } else {
+      DoNetErrorFromStatus(status);
+      DoClose();
+      return;
+    }
+  }
+
+  connection_.reset(new net::ClientSocketHandle);

[akalin, 2011/04/13 20:49:32]

Yeah this part is weird.  Shouldn't we use a socket pool and call that, instead
of having to create sockets ourselves?

[sanjeevr, 2011/04/13 21:20:21]

We are not creating the socket ourselves. InitSocketHandleForRawConnect creates
the required layers of sockets and initializes the socket handle for us.

+  // Now that we have resolved the proxy, we need to connect.
+  status = net::ClientSocketPoolManager::InitSocketHandleForRawConnect(
+      dest_host_port_pair_,
+      network_session_.get(),
+      proxy_info_,
+      ssl_config_,
+      ssl_config_,
+      bound_net_log_,
+      connection_.get(),
+      &connect_callback_);
+  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
+    // happen here.  This is okay, as from the caller's point of view,
+    // the connect always happens asynchronously.
+    MessageLoop* message_loop = MessageLoop::current();
+    CHECK(message_loop);
+    message_loop->PostTask(
+        FROM_HERE,
+        scoped_runnable_method_factory_.NewRunnableMethod(
+            &ChromeAsyncSocket::ProcessConnectDone, status));
+  }
+}
+
 // STATE_CONNECTING -> STATE_OPEN
 // read_state_ == IDLE -> read_state_ == POSTED (via PostDoRead())
 
@@ -174,6 +306,43 @@
   DCHECK_EQ(write_state_, IDLE);
   DCHECK_EQ(state_, STATE_CONNECTING);
   if (status != net::OK) {
+    // If the connection fails, try another proxy.
+    if (!client_socket_factory_.get())
+      status = ReconsiderProxyAfterError(status);
+    if ((status != net::OK) && (status != net::ERR_IO_PENDING)) {
+      // If no more proxies to try, we are done.
+      DoNetErrorFromStatus(status);
+      DoClose();
+    }
+    return;
+  }
+  // If we need to use a fake handshake, process that now.
+  if (use_fake_ssl_client_socket_) {
+    net::ClientSocket* fake_ssl_socket =
+        new FakeSSLClientSocket(connection_.release());
+    connection_.reset(new net::ClientSocketHandle);
+    connection_->set_socket(fake_ssl_socket);
+    status = fake_ssl_socket->Connect(&fake_ssl_handshake_callback_);
+    if (status != net::ERR_IO_PENDING)
+      ProcessFakeSSLHandshakeDone(status);
+  } else {
+    state_ = STATE_OPEN;
+    PostDoRead();
+    // Write buffer should be empty.
+    DCHECK_EQ(write_end_, 0U);
+    SignalConnected();
+  }
+}
+
+// STATE_CONNECTING -> STATE_OPEN
+// read_state_ == IDLE -> read_state_ == POSTED (via PostDoRead())
+
+void ChromeAsyncSocket::ProcessFakeSSLHandshakeDone(int status) {
+  DCHECK_NE(status, net::ERR_IO_PENDING);
+  DCHECK_EQ(read_state_, IDLE);
+  DCHECK_EQ(write_state_, IDLE);
+  DCHECK_EQ(state_, STATE_CONNECTING);
+  if (status != net::OK) {
     DoNetErrorFromStatus(status);
     DoClose();
     return;
@@ -212,9 +381,8 @@
   // finishes.  This is okay, as StartTls() is called only from a read
   // handler (i.e., after a read finishes and before another read is
   // done).
-  int status =
-      transport_socket_->Read(
-          read_buf_.get(), read_buf_->size(), &read_callback_);
+  int status = connection_->socket()->Read(
+      read_buf_.get(), read_buf_->size(), &read_callback_);
   read_state_ = PENDING;
   if (status != net::ERR_IO_PENDING) {
     ProcessReadDone(status);
@@ -343,9 +511,8 @@
   // finishes.  This is okay, as StartTls() is called only after we
   // have received a reply to a message we sent to the server and
   // before we send the next message.
-  int status =
-      transport_socket_->Write(
-          write_buf_.get(), write_end_, &write_callback_);
+  int status = connection_->socket()->Write(
+      write_buf_.get(), write_end_, &write_callback_);
   write_state_ = PENDING;
   if (status != net::ERR_IO_PENDING) {
     ProcessWriteDone(status);
@@ -397,10 +564,10 @@
 
 void ChromeAsyncSocket::DoClose() {
   scoped_runnable_method_factory_.RevokeAll();
-  if (transport_socket_.get()) {
-    transport_socket_->Disconnect();
-  }
-  transport_socket_.reset();
+  // Disconnect the socket.
+  if (connection_.get() && connection_->socket())
+    connection_->socket()->Disconnect();
+  connection_.reset();
   read_state_ = IDLE;
   read_start_ = 0U;
   read_end_ = 0U;
@@ -435,13 +602,20 @@
   // Clear out any posted DoRead() tasks.
   scoped_runnable_method_factory_.RevokeAll();
 
-  DCHECK(transport_socket_.get());
-  transport_socket_.reset(
-      client_socket_factory_->CreateSSLClientSocket(
-          transport_socket_.release(), net::HostPortPair(domain_name, 443),
-          ssl_config_, NULL /* ssl_host_info */,
-          cert_verifier_));
-  int status = transport_socket_->Connect(&ssl_connect_callback_);
+  net::ClientSocketFactory* factory =
+      client_socket_factory_.get() ? client_socket_factory_.get()
+          : net::ClientSocketFactory::GetDefaultFactory();
+
+  DCHECK(connection_.get());
+  net::ClientSocket* ssl_socket = factory->CreateSSLClientSocket(
+      connection_.release(), net::HostPortPair(domain_name, 443),
+      ssl_config_, NULL /* ssl_host_info */,
+      request_context_getter_->GetURLRequestContext()->cert_verifier(),
+      NULL);
+  connection_.reset(new net::ClientSocketHandle);
+  connection_->set_socket(ssl_socket);
+
+  int status = ssl_socket->Connect(&ssl_connect_callback_);
   if (status != net::ERR_IO_PENDING) {
     MessageLoop* message_loop = MessageLoop::current();
     CHECK(message_loop);
@@ -478,4 +652,80 @@
   SignalSSLConnected();
 }
 
+// TODO(sanjeevr): This has largely been copied from
+// HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError. This should be
+// refactored into some common place.
+int ChromeAsyncSocket::ReconsiderProxyAfterError(int error) {
+  DCHECK(!pac_request_);
+  // A failure to resolve the hostname or any error related to establishing a
+  // TCP connection could be grounds for trying a new proxy configuration.
+  //
+  // Why do this when a hostname cannot be resolved?  Some URLs only make sense
+  // to proxy servers.  The hostname in those URLs might fail to resolve if we
+  // are still using a non-proxy config.  We need to check if a proxy config
+  // now exists that corresponds to a proxy server that could load the URL.
+  //
+  switch (error) {
+    case net::ERR_PROXY_CONNECTION_FAILED:
+    case net::ERR_NAME_NOT_RESOLVED:
+    case net::ERR_INTERNET_DISCONNECTED:
+    case net::ERR_ADDRESS_UNREACHABLE:
+    case net::ERR_CONNECTION_CLOSED:
+    case net::ERR_CONNECTION_RESET:
+    case net::ERR_CONNECTION_REFUSED:
+    case net::ERR_CONNECTION_ABORTED:
+    case net::ERR_TIMED_OUT:
+    case net::ERR_TUNNEL_CONNECTION_FAILED:
+    case net::ERR_SOCKS_CONNECTION_FAILED:
+      break;
+    case net::ERR_SOCKS_CONNECTION_HOST_UNREACHABLE:
+      // Remap the SOCKS-specific "host unreachable" error to a more
+      // generic error code (this way consumers like the link doctor
+      // know to substitute their error page).
+      //
+      // Note that if the host resolving was done by the SOCSK5 proxy, we can't
+      // differentiate between a proxy-side "host not found" versus a proxy-side
+      // "address unreachable" error, and will report both of these failures as
+      // ERR_ADDRESS_UNREACHABLE.
+      return net::ERR_ADDRESS_UNREACHABLE;
+    default:
+      return error;
+  }
+
+  if (proxy_info_.is_https() && ssl_config_.send_client_cert) {
+    network_session_->ssl_client_auth_cache()->Remove(
+        proxy_info_.proxy_server().host_port_pair().ToString());
+  }
+
+  GURL url = GURL("http://" + dest_host_port_pair_.ToString());
+  int rv = network_session_->proxy_service()->ReconsiderProxyAfterError(
+      url, &proxy_info_, &proxy_resolve_callback_, &pac_request_,
+      bound_net_log_);
+  if (rv == net::OK || rv == net::ERR_IO_PENDING) {
+    // If the error was during connection setup, there is no socket to
+    // disconnect.
+    if (connection_->socket())
+      connection_->socket()->Disconnect();
+    connection_->Reset();
+  } else {
+    // If ReconsiderProxyAfterError() failed synchronously, it means
+    // there was nothing left to fall-back to, so fail the transaction
+    // with the last connection error we got.
+    rv = error;
+  }
+
+  // We either have new proxy info or there was an error in falling back.
+  // In both cases we want to post ProcessProxyResolveDone (in the error case
+  // we might still want to fall back a direct connection).
+  if (rv != net::ERR_IO_PENDING) {
+    MessageLoop* message_loop = MessageLoop::current();
+    CHECK(message_loop);
+    message_loop->PostTask(
+        FROM_HERE,
+        scoped_runnable_method_factory_.NewRunnableMethod(
+            &ChromeAsyncSocket::ProcessProxyResolveDone, rv));
+  }
+  return rv;
+}
+
 }  // namespace notifier