Stop refcounting ClientSocketPool.

Establishes that HttpNetworkSession owns all the socket pools.
Move out all the socket pools into a ClientSocketPoolManager.  This is because
of the dependency tree amongst socket pools, which dictates the order in which
they must be constructed and destructed.  In order to better establish it, I
moved them out to their own class.  HttpNetworkSession owns the
ClientSocketPoolManager which owns the pools.  We pass the pools as raw
pointers everywhere.

Note that ClientSocketPoolManager owns more pools than are publicly accessible via its interface.  That's because some of them are wrapped by publicly exposed pools.

Also, ClientSocketPoolHistograms used to be reference counted.  That's because it can be shared by multiple ClientSocketPools.  But it's effectively a global as well, so I make their lifetimes persist for the length of ClientSocketPoolManager too.

I also removed internal refcounting in ClientSocketPoolBase.  I had refcounted
it before I knew about ScopedRunnableMethodFactory back when I first started.

I cleaned up the unit tests a lot.  Back when I was a young padawan, I didn't
really know what I was doing, so I copy/pasted a metric asston of code.  Turns
out most of it was stupid, so I fixed it.  I also stopped the use of
implementation inheritance with ClientSocketPoolTest because it's discouraged
by the style guide and more importantly because it caused the
ClientSocketHandles within the TestSocketRequest vector to be destroyed _after_
the pools themselves were destroyed, which is bad since the handles will call
pool_->Release() which blows up.

BUG=56215,56215
TEST=Existing unit tests

Review URL: http://codereview.chromium.org/3389020

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

1 files changed, 27 insertions, 32 deletions

diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 2e33abc..ba49e9b 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -131,8 +131,7 @@ namespace internal {
 // ClientSocketPoolBaseHelper.  This class is not for external use, please use
 // ClientSocketPoolBase instead.
 class ClientSocketPoolBaseHelper
-    : public base::RefCounted<ClientSocketPoolBaseHelper>,
-      public ConnectJob::Delegate,
+    : public ConnectJob::Delegate,
       public NetworkChangeNotifier::Observer {
  public:
   class Request {
@@ -181,6 +180,8 @@ class ClientSocketPoolBaseHelper
       base::TimeDelta used_idle_socket_timeout,
       ConnectJobFactory* connect_job_factory);
 
+  ~ClientSocketPoolBaseHelper();
+
   // See ClientSocketPool::RequestSocket for documentation on this function.
   // ClientSocketPoolBaseHelper takes ownership of |request|, which must be
   // heap allocated.
@@ -351,8 +352,6 @@ class ClientSocketPoolBaseHelper
   typedef std::map<const ClientSocketHandle*, CallbackResultPair>
       PendingCallbackMap;
 
-  ~ClientSocketPoolBaseHelper();
-
   static void InsertRequestIntoQueue(const Request* r,
                                      RequestQueue* pending_requests);
   static const Request* RemoveRequestFromQueue(RequestQueue::iterator it,
@@ -485,6 +484,8 @@ class ClientSocketPoolBaseHelper
   // make sure that they are discarded rather than reused.
   int pool_generation_number_;
 
+  ScopedRunnableMethodFactory<ClientSocketPoolBaseHelper> method_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientSocketPoolBaseHelper);
 };
 
@@ -538,15 +539,14 @@ class ClientSocketPoolBase {
   ClientSocketPoolBase(
       int max_sockets,
       int max_sockets_per_group,
-      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
+      ClientSocketPoolHistograms* histograms,
       base::TimeDelta unused_idle_socket_timeout,
       base::TimeDelta used_idle_socket_timeout,
       ConnectJobFactory* connect_job_factory)
       : histograms_(histograms),
-        helper_(new internal::ClientSocketPoolBaseHelper(
-            max_sockets, max_sockets_per_group,
-            unused_idle_socket_timeout, used_idle_socket_timeout,
-            new ConnectJobFactoryAdaptor(connect_job_factory))) {}
+        helper_(max_sockets, max_sockets_per_group,
+                unused_idle_socket_timeout, used_idle_socket_timeout,
+                new ConnectJobFactoryAdaptor(connect_job_factory)) {}
 
   virtual ~ClientSocketPoolBase() {}
 
@@ -562,64 +562,64 @@ class ClientSocketPoolBase {
                     CompletionCallback* callback,
                     const BoundNetLog& net_log) {
     Request* request = new Request(handle, callback, priority, params, net_log);
-    return helper_->RequestSocket(group_name, request);
+    return helper_.RequestSocket(group_name, request);
   }
 
   void CancelRequest(const std::string& group_name,
                      ClientSocketHandle* handle) {
-    return helper_->CancelRequest(group_name, handle);
+    return helper_.CancelRequest(group_name, handle);
   }
 
   void ReleaseSocket(const std::string& group_name, ClientSocket* socket,
                      int id) {
-    return helper_->ReleaseSocket(group_name, socket, id);
+    return helper_.ReleaseSocket(group_name, socket, id);
   }
 
-  void CloseIdleSockets() { return helper_->CloseIdleSockets(); }
+  void CloseIdleSockets() { return helper_.CloseIdleSockets(); }
 
-  int idle_socket_count() const { return helper_->idle_socket_count(); }
+  int idle_socket_count() const { return helper_.idle_socket_count(); }
 
   int IdleSocketCountInGroup(const std::string& group_name) const {
-    return helper_->IdleSocketCountInGroup(group_name);
+    return helper_.IdleSocketCountInGroup(group_name);
   }
 
   LoadState GetLoadState(const std::string& group_name,
                          const ClientSocketHandle* handle) const {
-    return helper_->GetLoadState(group_name, handle);
+    return helper_.GetLoadState(group_name, handle);
   }
 
   virtual void OnConnectJobComplete(int result, ConnectJob* job) {
-    return helper_->OnConnectJobComplete(result, job);
+    return helper_.OnConnectJobComplete(result, job);
   }
 
   int NumConnectJobsInGroup(const std::string& group_name) const {
-    return helper_->NumConnectJobsInGroup(group_name);
+    return helper_.NumConnectJobsInGroup(group_name);
   }
 
   bool HasGroup(const std::string& group_name) const {
-    return helper_->HasGroup(group_name);
+    return helper_.HasGroup(group_name);
   }
 
   void CleanupIdleSockets(bool force) {
-    return helper_->CleanupIdleSockets(force);
+    return helper_.CleanupIdleSockets(force);
   }
 
   DictionaryValue* GetInfoAsValue(const std::string& name,
                                   const std::string& type) const {
-    return helper_->GetInfoAsValue(name, type);
+    return helper_.GetInfoAsValue(name, type);
   }
 
   base::TimeDelta ConnectionTimeout() const {
-    return helper_->ConnectionTimeout();
+    return helper_.ConnectionTimeout();
   }
 
-  scoped_refptr<ClientSocketPoolHistograms> histograms() const {
+  ClientSocketPoolHistograms* histograms() const {
     return histograms_;
   }
 
-  void EnableConnectBackupJobs() { helper_->EnableConnectBackupJobs(); }
+  void EnableConnectBackupJobs() { helper_.EnableConnectBackupJobs(); }
 
-  void Flush() { helper_->Flush(); }
+  void Flush() { helper_.Flush(); }
 
  private:
   // This adaptor class exists to bridge the
@@ -654,13 +654,8 @@ class ClientSocketPoolBase {
   };
 
   // Histograms for the pool
-  const scoped_refptr<ClientSocketPoolHistograms> histograms_;
-
-  // The reason for reference counting here is because the operations on
-  // the ClientSocketPoolBaseHelper which release sockets can cause the
-  // ClientSocketPoolBase<T> reference to drop to zero.  While we're deep
-  // in cleanup code, we'll often hold a reference to |self|.
-  scoped_refptr<internal::ClientSocketPoolBaseHelper> helper_;
+  ClientSocketPoolHistograms* const histograms_;
+  internal::ClientSocketPoolBaseHelper helper_;
 
   DISALLOW_COPY_AND_ASSIGN(ClientSocketPoolBase);
 };