Fixes an invalid read bug in ClientSocketPoolBaseHelper and cancels ConnectJobs on network changes.

The bug seems to be running callbacks that delete the ClientSocketPoolBaseHelper object.  We need to hold onto a self-reference in the stack frame.
The new test does not crash, but will trigger invalid reads in valgrind without the fix.
The fixes are overly conservative.  At every entry point that can lead to releasing the last reference, we hold onto a self-reference.  Not all of them can currently lead to crashes since the last operation is running the callback.  But this conservative measure will help future-proof the code in case anyone adds code without thinking about it.  The references are cheap anyway.
BUG=44724

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

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

2 files changed, 38 insertions, 0 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index ddaa430..f584996c 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -342,6 +342,10 @@ void ClientSocketPoolBaseHelper::OnBackupSocketTimerFired(
 
 void ClientSocketPoolBaseHelper::CancelRequest(
     const std::string& group_name, const ClientSocketHandle* handle) {
+  // Running callbacks can cause the last outside reference to be released.
+  // Hold onto a reference.
+  scoped_refptr<ClientSocketPoolBaseHelper> ref_holder(this);
+
   CHECK(ContainsKey(group_map_, group_name));
 
   Group& group = group_map_[group_name];
@@ -480,6 +484,10 @@ void ClientSocketPoolBaseHelper::DecrementIdleCount() {
 
 void ClientSocketPoolBaseHelper::DoReleaseSocket(const std::string& group_name,
                                                  ClientSocket* socket) {
+  // Running callbacks can cause the last outside reference to be released.
+  // Hold onto a reference.
+  scoped_refptr<ClientSocketPoolBaseHelper> ref_holder(this);
+
   GroupMap::iterator i = group_map_.find(group_name);
   CHECK(i != group_map_.end());
 
@@ -573,6 +581,10 @@ int ClientSocketPoolBaseHelper::FindTopStalledGroup(Group** group,
 
 void ClientSocketPoolBaseHelper::OnConnectJobComplete(
     int result, ConnectJob* job) {
+  // Running callbacks can cause the last outside reference to be released.
+  // Hold onto a reference.
+  scoped_refptr<ClientSocketPoolBaseHelper> ref_holder(this);
+
   DCHECK_NE(ERR_IO_PENDING, result);
   const std::string group_name = job->group_name();
   GroupMap::iterator group_it = group_map_.find(group_name);
@@ -618,6 +630,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
 }
 
 void ClientSocketPoolBaseHelper::OnIPAddressChanged() {
+  CancelAllConnectJobs();
   CloseIdleSockets();
 }
 
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index bf0f96e..a694a6f 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -1635,6 +1635,31 @@ TEST_F(ClientSocketPoolBaseTest, ReleasedSocketReleasesToo) {
   EXPECT_EQ(OK, request.WaitForResult());
 }
 
+// http://crbug.com/44724 regression test.
+// We start releasing the pool when we flush on network change.  When that
+// happens, the only active references are in the ClientSocketHandles.  When a
+// ConnectJob completes and calls back into the last ClientSocketHandle, that
+// callback can release the last reference and delete the pool.  After the
+// callback finishes, we go back to the stack frame within the now-deleted pool.
+// Executing any code that refers to members of the now-deleted pool can cause
+// crashes.
+TEST_F(ClientSocketPoolBaseTest, CallbackThatReleasesPool) {
+  CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingFailingJob);
+
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  EXPECT_EQ(ERR_IO_PENDING,
+            InitHandle(&handle, "a", kDefaultPriority,
+                       &callback, pool_, BoundNetLog()));
+
+  // Simulate flushing the pool.
+  pool_ = NULL;
+
+  // We'll call back into this now.
+  callback.WaitForResult();
+}
+
 }  // namespace
 
 }  // namespace net