Flip: Fix a bunch of leaks.

FlipIOBuffer::release() was broken.  It called release() on scoped_refptrs which does not decrease refcount, hence it leaked IOBuffers and FlipStreams.
Redo the memory management for FlipSession.  Stop using raw pointers in FlipSessionPool to hold onto FlipSession.  FlipSessionPool uses scoped_refptr now to track Fli
pSessions.  Instead of having FlipSession call Release() on itself, it now calls FlipSessionPool to remove itself from the pool when the tcp connection is closed.
In FlipStreamTest, manually call FlipSessionPool::Remove() since there is no tcp connection event to trigger FlipSession to remove itself.
BUG=http://crbug.com/28493

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

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

6 files changed, 61 insertions, 33 deletions

diff --git a/net/flip/flip_io_buffer.cc b/net/flip/flip_io_buffer.cc
index b3aecc5..5235c62 100644
--- a/net/flip/flip_io_buffer.cc
+++ b/net/flip/flip_io_buffer.cc
@@ -21,4 +21,9 @@ FlipIOBuffer::FlipIOBuffer() : priority_(0), position_(0), stream_(NULL) {}
 
 FlipIOBuffer::~FlipIOBuffer() {}
 
+void FlipIOBuffer::release() {
+  buffer_ = NULL;
+  stream_ = NULL;
+}
+
 }  // namespace net
diff --git a/net/flip/flip_io_buffer.h b/net/flip/flip_io_buffer.h
index 85d9510..ffb4b3b 100644
--- a/net/flip/flip_io_buffer.h
+++ b/net/flip/flip_io_buffer.h
@@ -30,10 +30,7 @@ class FlipIOBuffer {
   // Accessors.
   IOBuffer* buffer() const { return buffer_; }
   size_t size() const { return buffer_->size(); }
-  void release() {
-    buffer_.release();
-    stream_.release();
-  }
+  void release();
   int priority() const { return priority_; }
   const scoped_refptr<FlipStream>& stream() const { return stream_; }
 
diff --git a/net/flip/flip_session.cc b/net/flip/flip_session.cc
index d3bcedc..de87732 100644
--- a/net/flip/flip_session.cc
+++ b/net/flip/flip_session.cc
@@ -169,7 +169,9 @@ FlipSession::~FlipSession() {
     connection_.socket()->Disconnect();
   }
 
-  session_->flip_session_pool()->Remove(this);
+  // TODO(willchan): Don't hardcode port 80 here.
+  DCHECK(!session_->flip_session_pool()->HasSession(
+      HostResolver::RequestInfo(domain_, 80)));
 }
 
 net::Error FlipSession::Connect(const std::string& group_name,
@@ -359,7 +361,7 @@ void FlipSession::OnTCPConnect(int result) {
   if (result != net::OK) {
     net::Error err = static_cast<net::Error>(result);
     CloseAllStreams(err);
-    this->Release();
+    session_->flip_session_pool()->Remove(this);
     return;
   }
 
@@ -426,7 +428,7 @@ void FlipSession::OnReadComplete(int bytes_read) {
     // Session is tearing down.
     net::Error err = static_cast<net::Error>(bytes_read);
     CloseAllStreams(err);
-    this->Release();
+    session_->flip_session_pool()->Remove(this);
     return;
   }
 
diff --git a/net/flip/flip_session_pool.cc b/net/flip/flip_session_pool.cc
index 386fddc..ec3b34d 100644
--- a/net/flip/flip_session_pool.cc
+++ b/net/flip/flip_session_pool.cc
@@ -17,10 +17,10 @@ FlipSessionPool::~FlipSessionPool() {
   CloseAllSessions();
 }
 
-FlipSession* FlipSessionPool::Get(const HostResolver::RequestInfo& info,
-                                  HttpNetworkSession* session) {
+scoped_refptr<FlipSession> FlipSessionPool::Get(
+    const HostResolver::RequestInfo& info, HttpNetworkSession* session) {
   const std::string& domain = info.hostname();
-  FlipSession* flip_session = NULL;
+  scoped_refptr<FlipSession> flip_session;
   FlipSessionList* list = GetSessionList(domain);
   if (list) {
     if (list->size() >= kMaxSessionsPerDomain) {
@@ -32,10 +32,8 @@ FlipSession* FlipSessionPool::Get(const HostResolver::RequestInfo& info,
   }
 
   DCHECK(list);
-  if (!flip_session) {
+  if (!flip_session)
     flip_session = new FlipSession(domain, session);
-    flip_session->AddRef();  // Keep it in the cache.
-  }
 
   DCHECK(flip_session);
   list->push_back(flip_session);
@@ -43,6 +41,14 @@ FlipSession* FlipSessionPool::Get(const HostResolver::RequestInfo& info,
   return flip_session;
 }
 
+scoped_refptr<FlipSession> FlipSessionPool::GetFlipSessionFromSocket(
+    const HostResolver::RequestInfo& info,
+    HttpNetworkSession* session,
+    ClientSocket* socket) {
+  NOTIMPLEMENTED();
+  return NULL;
+}
+
 bool FlipSessionPool::HasSession(const HostResolver::RequestInfo& info) const {
   const std::string& domain = info.hostname();
   if (GetSessionList(domain))
@@ -50,11 +56,10 @@ bool FlipSessionPool::HasSession(const HostResolver::RequestInfo& info) const {
   return false;
 }
 
-void FlipSessionPool::Remove(FlipSession* session) {
+void FlipSessionPool::Remove(const scoped_refptr<FlipSession>& session) {
   std::string domain = session->domain();
   FlipSessionList* list = GetSessionList(domain);
-  if (list == NULL)
-    return;
+  CHECK(list);
   list->remove(session);
   if (list->empty())
     RemoveSessionList(domain);
@@ -98,10 +103,9 @@ void FlipSessionPool::CloseAllSessions() {
     DCHECK(list);
     sessions_.erase(sessions_.begin()->first);
     while (list->size()) {
-      FlipSession* session = list->front();
+      scoped_refptr<FlipSession> session = list->front();
       list->pop_front();
       session->CloseAllStreams(net::OK);
-      session->Release();
     }
     delete list;
   }
diff --git a/net/flip/flip_session_pool.h b/net/flip/flip_session_pool.h
index 76ffc0d..fc3be5c 100644
--- a/net/flip/flip_session_pool.h
+++ b/net/flip/flip_session_pool.h
@@ -27,18 +27,16 @@ class FlipSessionPool : public base::RefCounted<FlipSessionPool> {
 
   // Either returns an existing FlipSession or creates a new FlipSession for
   // use.
-  FlipSession* Get(const HostResolver::RequestInfo& info,
-                   HttpNetworkSession* session);
+  scoped_refptr<FlipSession> Get(
+      const HostResolver::RequestInfo& info, HttpNetworkSession* session);
 
   // Builds a FlipSession from an existing socket.
-  FlipSession* GetFlipSessionFromSocket(
+  // TODO(willchan): Implement this to allow a HttpNetworkTransaction to
+  // upgrade a TCP connection from HTTP to FLIP.
+  scoped_refptr<FlipSession> GetFlipSessionFromSocket(
       const HostResolver::RequestInfo& info,
       HttpNetworkSession* session,
-      ClientSocket* socket) {
-    // TODO(willchan): Implement this to allow a HttpNetworkTransaction to
-    // upgrade a TCP connection from HTTP to FLIP.
-    return NULL;
-  }
+      ClientSocket* socket);
 
   // TODO(willchan): Consider renaming to HasReusableSession, since perhaps we
   // should be creating a new session.
@@ -50,14 +48,15 @@ class FlipSessionPool : public base::RefCounted<FlipSessionPool> {
  private:
   friend class base::RefCounted<FlipSessionPool>;
   friend class FlipSession;  // Needed for Remove().
+  friend class FlipSessionPoolPeer;  // For testing.
 
-  virtual ~FlipSessionPool();
-
-  typedef std::list<FlipSession*> FlipSessionList;
+  typedef std::list<scoped_refptr<FlipSession> > FlipSessionList;
   typedef std::map<std::string, FlipSessionList*> FlipSessionsMap;
 
-  // Return a FlipSession to the pool.
-  void Remove(FlipSession* session);
+  virtual ~FlipSessionPool();
+
+  // Removes a FlipSession from the FlipSessionPool.
+  void Remove(const scoped_refptr<FlipSession>& session);
 
   // Helper functions for manipulating the lists.
   FlipSessionList* AddSessionList(const std::string& domain);
diff --git a/net/flip/flip_stream_unittest.cc b/net/flip/flip_stream_unittest.cc
index 46d3b54..311b51c 100644
--- a/net/flip/flip_stream_unittest.cc
+++ b/net/flip/flip_stream_unittest.cc
@@ -19,6 +19,21 @@
 
 namespace net {
 
+class FlipSessionPoolPeer {
+ public:
+  explicit FlipSessionPoolPeer(const scoped_refptr<FlipSessionPool>& pool)
+      : pool_(pool) {}
+
+  void RemoveFlipSession(const scoped_refptr<FlipSession>& session) {
+    pool_->Remove(session);
+  }
+
+ private:
+  const scoped_refptr<FlipSessionPool> pool_;
+
+  DISALLOW_COPY_AND_ASSIGN(FlipSessionPoolPeer);
+};
+
 namespace {
 
 // Create a proxy service which fails on all requests (falls back to direct).
@@ -62,7 +77,8 @@ HttpNetworkSession* CreateSession(SessionDependencies* session_deps) {
 class FlipStreamTest : public testing::Test {
  protected:
   FlipStreamTest()
-      : session_(CreateSession(&session_deps_)) {}
+      : session_(CreateSession(&session_deps_)),
+        pool_peer_(session_->flip_session_pool()) {}
 
   scoped_refptr<FlipSession> CreateFlipSession() {
     HostResolver::RequestInfo resolve_info("www.google.com", 80);
@@ -77,10 +93,11 @@ class FlipStreamTest : public testing::Test {
 
   SessionDependencies session_deps_;
   scoped_refptr<HttpNetworkSession> session_;
+  FlipSessionPoolPeer pool_peer_;
 };
 
 // Needs fixing, see http://crbug.com/28622
-TEST_F(FlipStreamTest, DISABLED_SendRequest) {
+TEST_F(FlipStreamTest, SendRequest) {
   scoped_refptr<FlipSession> session(CreateFlipSession());
   HttpRequestInfo request;
   request.method = "GET";
@@ -89,6 +106,10 @@ TEST_F(FlipStreamTest, DISABLED_SendRequest) {
 
   scoped_refptr<FlipStream> stream(new FlipStream(session, 1, false));
   EXPECT_EQ(ERR_IO_PENDING, stream->SendRequest(NULL, &callback));
+
+  // Need to manually remove the flip session since normally it gets removed on
+  // socket close/error, but we aren't communicating over a socket here.
+  pool_peer_.RemoveFlipSession(session);
 }
 
 // TODO(willchan): Write a longer test for FlipStream that exercises all