Fix valgrind memcheck issues with GetPushStream unit test.

BUG=None
TEST=valgrind --leak-check=yes net_unittests --gtest_filter="*SpdySessionTest.GetPushStream*"

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

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

5 files changed, 47 insertions, 10 deletions

diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index f0d0904..902ef79 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -982,8 +982,7 @@ void SpdySession::OnSyn(const spdy::SpdySynStreamControlFrame& frame,
     //
     // TODO(cbentzel): Minimize allocations and copies of HttpResponseInfo
     // object. Should it just be part of SpdyStream?
-    HttpResponseInfo* response_info = new HttpResponseInfo();
-    stream->set_response_info_pointer(response_info);
+    stream->SetPushResponse(new HttpResponseInfo());
   }
 
   pushed_streams_.push_back(stream);
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index b10f645..cff7b82 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -91,7 +91,7 @@ class SpdySession : public base::RefCounted<SpdySession>,
 
  private:
   friend class base::RefCounted<SpdySession>;
-  FRIEND_TEST(SpdySessionTest, DISABLED_GetPushStream);
+  FRIEND_TEST(SpdySessionTest, GetPushStream);
 
   enum State {
     IDLE,
diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 603c269..980311a 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -172,7 +172,7 @@ static const uint8 kPush[] = {
 
 }  // namespace
 
-TEST_F(SpdySessionTest, DISABLED_GetPushStream) {
+TEST_F(SpdySessionTest, GetPushStream) {
   SpdySessionTest::TurnOffCompression();
 
   SessionDependencies session_deps;
@@ -230,6 +230,16 @@ TEST_F(SpdySessionTest, DISABLED_GetPushStream) {
   EXPECT_EQ(test_push_path, second_stream->path());
   EXPECT_EQ(2U, second_stream->stream_id());
   EXPECT_EQ(0, second_stream->priority());
+
+  // Clean up
+  second_stream = NULL;
+  session = NULL;
+  spdy_session_pool->CloseAllSessions();
+
+  // RunAllPending needs to be called here because the
+  // ClientSocketPoolBase posts a task to clean up and destroy the
+  // underlying socket.
+  MessageLoop::current()->RunAllPending();
 }
 
 }  // namespace net
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 022ccd2..547a1f0 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -62,6 +62,13 @@ const HttpResponseInfo* SpdyStream::GetResponseInfo() const {
   return response_;
 }
 
+void SpdyStream::SetPushResponse(HttpResponseInfo* response_info) {
+  DCHECK(!response_);
+  DCHECK(!push_response_.get());
+  push_response_.reset(response_info);
+  response_ = response_info;
+}
+
 const HttpRequestInfo* SpdyStream::GetRequestInfo() const {
   return &request_;
 }
@@ -153,10 +160,22 @@ int SpdyStream::SendRequest(UploadDataStream* upload_data,
   CHECK(!cancelled_);
   CHECK(response);
 
-  if (response_) {
-    *response = *response_;
-    delete response_;
+  // SendRequest can be called in two cases.
+  //
+  // a) A client initiated request. In this case, response_ should be NULL
+  //    to start with.
+  // b) A client request which matches a response that the server has already
+  //    pushed. In this case, the value of |*push_response_| is copied over to
+  //    the new response object |*response|. |push_response_| is cleared
+  //    and |*push_response_| is deleted, and |response_| is reset to
+  //    |response|.
+  if (push_response_.get()) {
+    *response = *push_response_;
+    push_response_.reset(NULL);
+    response_ = NULL;
   }
+
+  DCHECK_EQ(static_cast<HttpResponseInfo*>(NULL), response_);
   response_ = response;
 
   if (upload_data) {
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index 235e645..37f6900bf 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -127,9 +127,7 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
 
   bool cancelled() const { return cancelled_; }
 
-  void set_response_info_pointer(HttpResponseInfo* response_info) {
-    response_ = response_info;
-  }
+  void SetPushResponse(HttpResponseInfo* response_info);
 
  private:
   friend class base::RefCounted<SpdyStream>;
@@ -192,7 +190,18 @@ class SpdyStream : public base::RefCounted<SpdyStream> {
   // For cached responses, this time could be "far" in the past.
   base::Time request_time_;
 
+  // The push_response_ is the HTTP response data which is part of
+  // a server-initiated SYN_STREAM. If a client request comes in
+  // which matches the push stream, the data in push_response_ will
+  // be copied over to the response_ object owned by the caller
+  // of the request.
+  scoped_ptr<HttpResponseInfo> push_response_;
+
+  // response_ is the HTTP response data object which is filled in
+  // when a SYN_REPLY comes in for the stream. It is not owned by this
+  // stream object.
   HttpResponseInfo* response_;
+
   scoped_ptr<UploadDataStream> request_body_stream_;
 
   bool response_complete_;  // TODO(mbelshe): fold this into the io_state.