From 6c6ea177b4c8196bbf5ea214c9082e5027997a5a Mon Sep 17 00:00:00 2001 From: "erikchen@google.com" Date: Tue, 27 Jul 2010 20:04:03 +0000 Subject: SPDY sends RST_STREAM upon cancelling request, or bad header parse data. Also fix tsan failure for spdy_http_stream_unittest. Attempted to commit in http://codereview.chromium.org/3014030/show, ran into different tsan failure. TEST=net_unittests, tsan on windows for SpdyHttpStreamTest. BUG=46589, 47478, 50198 Review URL: http://codereview.chromium.org/2811072 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53829 0039d316-1c4b-4281-b951-d872f2087c98 --- .../valgrind/net_unittests.gtest-tsan_win32.txt | 3 -- net/spdy/spdy_http_stream_unittest.cc | 28 ++++++++----- net/spdy/spdy_network_transaction_unittest.cc | 46 +++++++++++++++++++++- net/spdy/spdy_session.cc | 12 +++++- net/spdy/spdy_session_pool.h | 3 +- net/spdy/spdy_stream.cc | 8 +++- 6 files changed, 81 insertions(+), 19 deletions(-) (limited to 'net') diff --git a/net/data/valgrind/net_unittests.gtest-tsan_win32.txt b/net/data/valgrind/net_unittests.gtest-tsan_win32.txt index 6b2663a..0fc6d3c 100644 --- a/net/data/valgrind/net_unittests.gtest-tsan_win32.txt +++ b/net/data/valgrind/net_unittests.gtest-tsan_win32.txt @@ -14,9 +14,6 @@ DiskCacheBackendTest.* # See http://crbug.com/47836 ClientSocketPoolBaseTest.CancelPendingSocketAtSocketLimit -# See http://crbug.com/50198 -SpdyHttpStreamTest.SendRequest - ######################################### # These tests fail if you don't have our SSL certificate installed. # Please see http://dev.chromium.org/developers/testing#TOC-SSL-tests diff --git a/net/spdy/spdy_http_stream_unittest.cc b/net/spdy/spdy_http_stream_unittest.cc index 9c23b54..ab4c606 100644 --- a/net/spdy/spdy_http_stream_unittest.cc +++ b/net/spdy/spdy_http_stream_unittest.cc @@ -10,6 +10,8 @@ namespace net { class SpdyHttpStreamTest : public testing::Test { + public: + OrderedSocketData* data() { return data_; } protected: SpdyHttpStreamTest() {} @@ -49,8 +51,11 @@ TEST_F(SpdyHttpStreamTest, SendRequest) { MockWrite writes[] = { CreateMockWrite(*req.get(), 1), }; + scoped_ptr resp(ConstructSpdyGetSynReply(NULL, 0, 1)); MockRead reads[] = { - MockRead(false, 0, 2) // EOF + CreateMockRead(*resp, 2), + MockRead(true, ERR_IO_PENDING, 3), // Force a pause + MockRead(false, 0, 4) // EOF }; HostPortPair host_port_pair("www.google.com", 80); @@ -70,9 +75,17 @@ TEST_F(SpdyHttpStreamTest, SendRequest) { EXPECT_EQ(ERR_IO_PENDING, http_stream->SendRequest(&response, &callback)); - MessageLoop::current()->RunAllPending(); EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(host_port_pair)); - http_session_->spdy_session_pool()->Remove(session_); + + // This triggers the MockWrite and reads 2 & 3 + callback.WaitForResult(); + + // This triggers read 4. The empty read causes the session to shut down. + data()->CompleteRead(); + + EXPECT_TRUE(!http_session_->spdy_session_pool()->HasSession(host_port_pair)); + EXPECT_TRUE(data()->at_read_eof()); + EXPECT_TRUE(data()->at_write_eof()); } // Test case for bug: http://code.google.com/p/chromium/issues/detail?id=50058 @@ -80,16 +93,12 @@ TEST_F(SpdyHttpStreamTest, SpdyURLTest) { EnableCompression(false); SpdySession::SetSSLMode(false); - scoped_ptr req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST)); - MockWrite writes[] = { - CreateMockWrite(*req.get(), 1), - }; MockRead reads[] = { MockRead(false, 0, 2), // EOF }; HostPortPair host_port_pair("www.google.com", 80); - EXPECT_EQ(OK, InitSession(reads, arraysize(reads), writes, arraysize(writes), + EXPECT_EQ(OK, InitSession(reads, arraysize(reads), NULL, 0, host_port_pair)); HttpRequestInfo request; @@ -113,7 +122,8 @@ TEST_F(SpdyHttpStreamTest, SpdyURLTest) { MessageLoop::current()->RunAllPending(); EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(host_port_pair)); - http_session_->spdy_session_pool()->Remove(session_); + http_session_->spdy_session_pool()->CloseAllSessions(); + EXPECT_TRUE(!data()->at_read_eof()); } // TODO(willchan): Write a longer test for SpdyStream that exercises all diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc index bce3feb..3ac38cb 100644 --- a/net/spdy/spdy_network_transaction_unittest.cc +++ b/net/spdy/spdy_network_transaction_unittest.cc @@ -1128,6 +1128,46 @@ TEST_P(SpdyNetworkTransactionTest, CancelledTransaction) { helper.VerifyDataNotConsumed(); } +// Verify that the client sends a Rst Frame upon cancelling the stream. +TEST_P(SpdyNetworkTransactionTest, CancelledTransactionSendRst) { + scoped_ptr req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST)); + scoped_ptr rst( + ConstructSpdyRstStream(1, spdy::CANCEL)); + MockWrite writes[] = { + CreateMockWrite(*req, 1), + CreateMockWrite(*rst, 3), + }; + + scoped_ptr resp(ConstructSpdyGetSynReply(NULL, 0, 1)); + MockRead reads[] = { + CreateMockRead(*resp, 2), + MockRead(true, 0, 0, 4) // EOF + }; + + scoped_refptr data( + new OrderedSocketData(reads, arraysize(reads), + writes, arraysize(writes))); + + NormalSpdyTransactionHelper helper(CreateGetRequest(), + BoundNetLog(), + GetParam()); + helper.AddData(data.get()); + helper.RunPreTestSetup(); + HttpNetworkTransaction* trans = helper.trans(); + + TestCompletionCallback callback; + + int rv = trans->Start(&CreateGetRequest(), &callback, BoundNetLog()); + EXPECT_EQ(ERR_IO_PENDING, rv); + rv = callback.WaitForResult(); + helper.ResetTrans(); // Cancel the transaction. + + // Finish running rest of tasks. + MessageLoop::current()->RunAllPending(); + data->CompleteRead(); + helper.VerifyDataConsumed(); +} + class SpdyNetworkTransactionTest::StartTransactionCallback : public CallbackRunner< Tuple1 > { public: @@ -1696,8 +1736,11 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) { scoped_ptr compressed( ConstructSpdyGet(NULL, 0, true, 1, LOWEST)); + scoped_ptr rst( + ConstructSpdyRstStream(1, spdy::PROTOCOL_ERROR)); MockWrite writes[] = { CreateMockWrite(*compressed), + CreateMockWrite(*rst), }; scoped_ptr resp(ConstructSpdyGetSynReply(NULL, 0, 1)); @@ -1705,7 +1748,6 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) { MockRead reads[] = { CreateMockRead(*resp), CreateMockRead(*body), - MockRead(true, 0, 0) // EOF }; scoped_refptr data( @@ -1715,7 +1757,7 @@ TEST_P(SpdyNetworkTransactionTest, DecompressFailureOnSynReply) { BoundNetLog(), GetParam()); helper.RunToCompletion(data.get()); TransactionHelperResult out = helper.output(); - EXPECT_EQ(ERR_SYN_REPLY_NOT_RECEIVED, out.rv); + EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv); data->Reset(); EnableCompression(false); diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc index 9ceb951..5d6fb02 100644 --- a/net/spdy/spdy_session.cc +++ b/net/spdy/spdy_session.cc @@ -1122,8 +1122,16 @@ void SpdySession::OnControl(const spdy::SpdyControlFrame* frame) { uint32 type = frame->type(); if (type == spdy::SYN_STREAM || type == spdy::SYN_REPLY) { if (!spdy_framer_.ParseHeaderBlock(frame, headers.get())) { - LOG(WARNING) << "Could not parse Spdy Control Frame Header"; - // TODO(mbelshe): Error the session? + LOG(WARNING) << "Could not parse Spdy Control Frame Header."; + int stream_id = 0; + if (type == spdy::SYN_STREAM) + stream_id = (reinterpret_cast + (frame))->stream_id(); + if (type == spdy::SYN_REPLY) + stream_id = (reinterpret_cast + (frame))->stream_id(); + if(IsStreamActive(stream_id)) + ResetStream(stream_id, spdy::PROTOCOL_ERROR); return; } } diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h index 362fdc9..1c18438 100644 --- a/net/spdy/spdy_session_pool.h +++ b/net/spdy/spdy_session_pool.h @@ -71,7 +71,8 @@ class SpdySessionPool // Close all Spdy Sessions; used for debugging. void CloseAllSessions(); - // Removes a SpdySession from the SpdySessionPool. + // Removes a SpdySession from the SpdySessionPool. This should only be called + // by SpdySession, because otherwise session->state_ is not set to CLOSED. void Remove(const scoped_refptr& session); // NetworkChangeNotifier::Observer methods: diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc index 72a1a4f..145141e 100644 --- a/net/spdy/spdy_stream.cc +++ b/net/spdy/spdy_stream.cc @@ -53,7 +53,7 @@ void SpdyStream::SetDelegate(Delegate* delegate) { void SpdyStream::DetachDelegate() { delegate_ = NULL; - if (!response_complete_ && !cancelled()) + if (!response_complete_) Cancel(); } @@ -216,8 +216,12 @@ void SpdyStream::OnClose(int status) { } void SpdyStream::Cancel() { + if (cancelled()) + return; + cancelled_ = true; - session_->CloseStream(stream_id_, ERR_ABORTED); + if(session_->IsStreamActive(stream_id_)) + session_->ResetStream(stream_id_, spdy::CANCEL); } int SpdyStream::DoSendRequest(bool has_upload_data) { -- cgit v1.1