From 3264fc3cf0162e1d2302b31afc55c8c3d7aef71d Mon Sep 17 00:00:00 2001 From: "cbentzel@chromium.org" Date: Mon, 10 May 2010 16:48:04 +0000 Subject: 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 --- net/data/valgrind/net_unittests.gtest.txt | 1 - net/spdy/spdy_session.cc | 3 +-- net/spdy/spdy_session.h | 2 +- net/spdy/spdy_session_unittest.cc | 12 +++++++++++- net/spdy/spdy_stream.cc | 25 ++++++++++++++++++++++--- net/spdy/spdy_stream.h | 15 ++++++++++++--- 6 files changed, 47 insertions(+), 11 deletions(-) (limited to 'net') diff --git a/net/data/valgrind/net_unittests.gtest.txt b/net/data/valgrind/net_unittests.gtest.txt index 7e77080..681e60c 100644 --- a/net/data/valgrind/net_unittests.gtest.txt +++ b/net/data/valgrind/net_unittests.gtest.txt @@ -2,5 +2,4 @@ KeygenHandlerTest.*SmokeTest # Fails Valgrind with varying stack traces. http://crbug.com/43179 -SpdySessionTest.GetPushStream SpdyNetworkTransactionTest.PostWithEarlySynReply 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, private: friend class base::RefCounted; - 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(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 { 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; @@ -192,7 +190,18 @@ class SpdyStream : public base::RefCounted { // 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 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 request_body_stream_; bool response_complete_; // TODO(mbelshe): fold this into the io_state. -- cgit v1.1