[SPDY] Drop the last reference to a SpdySession from a specific place

Namely, SpdySessionPool::RemoveUnavailableSession().

Add some DCHECKs that were waiting on this.

BUG=255701
R=rch@chromium.org

Review URL: https://codereview.chromium.org/18100008

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

3 files changed, 47 insertions, 42 deletions

diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index e80a938..ce5e0d3 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -434,20 +434,20 @@ Error SpdySession::InitializeWithSocket(
     bool is_secure,
     int certificate_error_code) {
   CHECK(!in_io_loop_);
+  DCHECK_EQ(availability_state_, STATE_AVAILABLE);
+  DCHECK_EQ(read_state_, READ_STATE_DO_READ);
+  DCHECK_EQ(write_state_, WRITE_STATE_IDLE);
   DCHECK(!connection_);
+
   DCHECK(certificate_error_code == OK ||
          certificate_error_code < ERR_IO_PENDING);
-
   // TODO(akalin): Check connection->is_initialized() instead. This
   // requires re-working CreateFakeSpdySession(), though.
   DCHECK(connection->socket());
+
   base::StatsCounter spdy_sessions("spdy.sessions");
   spdy_sessions.Increment();
 
-  DCHECK_EQ(availability_state_, STATE_AVAILABLE);
-  DCHECK_EQ(read_state_, READ_STATE_DO_READ);
-  DCHECK_EQ(write_state_, WRITE_STATE_IDLE);
-
   connection_ = connection.Pass();
   is_secure_ = is_secure;
   certificate_error_code_ = certificate_error_code;
@@ -538,6 +538,7 @@ int SpdySession::GetPushStream(
 
   stream->reset();
 
+  // TODO(akalin): Add unit test exercising this code path.
   if (availability_state_ == STATE_CLOSED)
     return ERR_CONNECTION_CLOSED;
 
@@ -579,6 +580,7 @@ int SpdySession::TryCreateStream(SpdyStreamRequest* request,
   if (availability_state_ == STATE_GOING_AWAY)
     return ERR_FAILED;
 
+  // TODO(akalin): Add unit test exercising this code path.
   if (availability_state_ == STATE_CLOSED)
     return ERR_CONNECTION_CLOSED;
 
@@ -607,6 +609,7 @@ int SpdySession::CreateStream(const SpdyStreamRequest& request,
   if (availability_state_ == STATE_GOING_AWAY)
     return ERR_FAILED;
 
+  // TODO(akalin): Add unit test exercising this code path.
   if (availability_state_ == STATE_CLOSED)
     return ERR_CONNECTION_CLOSED;
 
@@ -730,17 +733,17 @@ base::WeakPtr<SpdySession> SpdySession::GetWeakPtr() {
 
 bool SpdySession::CloseOneIdleConnection() {
   CHECK(!in_io_loop_);
-  DCHECK(pool_);
   DCHECK_NE(availability_state_, STATE_CLOSED);
+  DCHECK(pool_);
   if (!active_streams_.empty())
     return false;
-  base::WeakPtr<SpdySession> weak_ptr = weak_factory_.GetWeakPtr();
   CloseSessionResult result =
       DoCloseSession(ERR_CONNECTION_CLOSED, "Closing one idle connection.");
-  DCHECK_EQ(result, SESSION_CLOSED_AND_REMOVED);
-  // Since the underlying socket is only returned when |this| is destroyed,
-  // we should only return true if |this| no longer exists.
-  return !weak_ptr;
+  if (result != SESSION_CLOSED_AND_REMOVED) {
+    NOTREACHED();
+    return false;
+  }
+  return true;
 }
 
 void SpdySession::EnqueueStreamWrite(
@@ -1061,15 +1064,9 @@ void SpdySession::SendResetStreamFrame(SpdyStreamId stream_id,
 
 void SpdySession::PumpReadLoop(ReadState expected_read_state, int result) {
   CHECK(!in_io_loop_);
+  DCHECK_NE(availability_state_, STATE_CLOSED);
   DCHECK_EQ(read_state_, expected_read_state);
 
-  // TODO(akalin): Change this to a DCHECK once the SpdySessionPool
-  // will hold the last reference to this.
-  if (availability_state_ == STATE_CLOSED) {
-    DCHECK_LT(error_on_close_, ERR_IO_PENDING);
-    return;
-  }
-
   result = DoReadLoop(expected_read_state, result);
 
   if (availability_state_ == STATE_CLOSED) {
@@ -1202,15 +1199,9 @@ int SpdySession::DoReadComplete(int result) {
 
 void SpdySession::PumpWriteLoop(WriteState expected_write_state, int result) {
   CHECK(!in_io_loop_);
+  DCHECK_NE(availability_state_, STATE_CLOSED);
   DCHECK_EQ(write_state_, expected_write_state);
 
-  // TODO(akalin): Change this to a DCHECK once the SpdySessionPool
-  // will hold the last reference to this.
-  if (availability_state_ == STATE_CLOSED) {
-    DCHECK_LT(error_on_close_, ERR_IO_PENDING);
-    return;
-  }
-
   result = DoWriteLoop(expected_write_state, result);
 
   if (availability_state_ == STATE_CLOSED) {
@@ -1711,8 +1702,6 @@ void SpdySession::DeleteStream(scoped_ptr<SpdyStream> stream, int status) {
       // Do nothing.
       break;
   }
-
-  // Deleting |stream| may release the last reference to |this|.
 }
 
 base::WeakPtr<SpdyStream> SpdySession::GetActivePushStream(const GURL& url) {
@@ -2549,6 +2538,7 @@ void SpdySession::PlanToCheckPingStatus() {
 
 void SpdySession::CheckPingStatus(base::TimeTicks last_check_time) {
   CHECK(!in_io_loop_);
+  DCHECK_NE(availability_state_, STATE_CLOSED);
 
   // Check if we got a response back for all PINGs we had sent.
   if (pings_in_flight_ == 0) {
@@ -2700,16 +2690,14 @@ void SpdySession::OnWriteBufferConsumed(
     size_t frame_payload_size,
     size_t consume_size,
     SpdyBuffer::ConsumeSource consume_source) {
-  // This can happen when pending writes are deleted when closing the
-  // session.
+  // We can be called with |in_io_loop_| set if a write SpdyBuffer is
+  // deleted (e.g., a stream is closed due to incoming data).
+
   if (availability_state_ == STATE_CLOSED)
     return;
 
   DCHECK_EQ(flow_control_state_, FLOW_CONTROL_STREAM_AND_SESSION);
 
-  // We can be called with |in_io_loop_| set if a write SpdyBuffer is
-  // deleted (e.g., a stream is closed due to incoming data).
-
   if (consume_source == SpdyBuffer::DISCARD) {
     // If we're discarding a frame or part of it, increase the send
     // window by the number of discarded bytes. (Although if we're
@@ -2780,17 +2768,16 @@ void SpdySession::DecreaseSendWindowSize(int32 delta_window_size) {
 void SpdySession::OnReadBufferConsumed(
     size_t consume_size,
     SpdyBuffer::ConsumeSource consume_source) {
-  // TODO(akalin): Change this to a DCHECK when we make |pool_| hold
-  // the last reference to |this|.
-  if (availability_state_ == STATE_CLOSED)
-    return;
-
   // We can be called with |in_io_loop_| set if a read SpdyBuffer is
   // deleted (e.g., discarded by a SpdyReadQueue).
 
+  if (availability_state_ == STATE_CLOSED)
+    return;
+
   DCHECK_EQ(flow_control_state_, FLOW_CONTROL_STREAM_AND_SESSION);
   DCHECK_GE(consume_size, 1u);
   DCHECK_LE(consume_size, static_cast<size_t>(kint32max));
+
   IncreaseRecvWindowSize(static_cast<int32>(consume_size));
 }
 
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index 6825cfd..6c3b78c 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -231,7 +231,10 @@ void SpdySessionPool::RemoveUnavailableSession(
 
   SessionSet::iterator it = sessions_.find(unavailable_session.get());
   CHECK(it != sessions_.end());
+  scoped_refptr<SpdySession> last_ref(*it);
   sessions_.erase(it);
+
+  CHECK(last_ref->HasOneRef());
 }
 
 // Make a copy of |sessions_| in the Close* functions below to avoid
@@ -354,22 +357,31 @@ void SpdySessionPool::RemoveAliases(const SpdySessionKey& key) {
   }
 }
 
+SpdySessionPool::WeakSessionList SpdySessionPool::GetCurrentSessions() const {
+  WeakSessionList current_sessions;
+  for (SessionSet::const_iterator it = sessions_.begin();
+       it != sessions_.end(); ++it) {
+    current_sessions.push_back((*it)->GetWeakPtr());
+  }
+  return current_sessions;
+}
+
 void SpdySessionPool::CloseCurrentSessionsHelper(
     Error error,
     const std::string& description,
     bool idle_only) {
-  SessionSet current_sessions = sessions_;
-  for (SessionSet::const_iterator it = current_sessions.begin();
+  WeakSessionList current_sessions = GetCurrentSessions();
+  for (WeakSessionList::const_iterator it = current_sessions.begin();
        it != current_sessions.end(); ++it) {
-    if (!ContainsKey(sessions_, *it))
+    if (!*it)
       continue;
 
     if (idle_only && (*it)->is_active())
       continue;
 
     (*it)->CloseSessionOnError(error, description);
-    DCHECK(!IsSessionAvailable((*it)->GetWeakPtr()));
-    DCHECK(!ContainsKey(sessions_, *it));
+    DCHECK(!IsSessionAvailable(*it));
+    DCHECK(!*it);
   }
 }
 
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index d2301a0a7..86425e2 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
@@ -144,6 +145,7 @@ class NET_EXPORT SpdySessionPool
   friend class SpdySessionPoolPeer;  // For testing.
 
   typedef std::set<scoped_refptr<SpdySession> > SessionSet;
+  typedef std::vector<base::WeakPtr<SpdySession> > WeakSessionList;
   typedef std::map<SpdySessionKey, base::WeakPtr<SpdySession> >
       AvailableSessionMap;
   typedef std::map<IPEndPoint, SpdySessionKey> AliasMap;
@@ -171,6 +173,10 @@ class NET_EXPORT SpdySessionPool
   // Remove all aliases for |key| from the aliases table.
   void RemoveAliases(const SpdySessionKey& key);
 
+  // Get a copy of the current sessions as a list of WeakPtrs. Used by
+  // CloseCurrentSessionsHelper() below.
+  WeakSessionList GetCurrentSessions() const;
+
   // Close only the currently existing SpdySessions with |error|.  Let
   // any new ones created while this method is running continue to
   // live. If |idle_only| is true only idle sessions are closed.