From c940b7c4d7d6daab56ba8604979df1aef382e4e2 Mon Sep 17 00:00:00 2001 From: "simonjam@chromium.org" Date: Mon, 30 Jan 2012 20:57:27 +0000 Subject: Disconnect pipeline socket immediately on Close(true) while a Read*() is pending. This is a speculative fix for bug 105320. The chain of events that could tickle this is: 1. A HttpNetworkTransaction is canceled while a Read is blocked. 2. When the transaction is deleted: - It calls HttpPipelinedStream::Close(true), which queues tasks to evict the other streams on the pipeline. - It deletes the HttpPipelinedStream, which deletes the active HttpStreamParser. 4. The response has already arrived and is already on the message queue. It runs and tries to callback to the deleted HttpStreamParser. We likely crash. 5. The eviction tasks run and delete their streams, which allows the HttpPipelinedConnectionImpl destructor to run, which closes the socket. BUG=105320 TEST=net_unittests Review URL: http://codereview.chromium.org/9223033 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119728 0039d316-1c4b-4281-b951-d872f2087c98 --- net/http/http_pipelined_connection_impl.cc | 6 ++-- .../http_pipelined_connection_impl_unittest.cc | 40 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) (limited to 'net') diff --git a/net/http/http_pipelined_connection_impl.cc b/net/http/http_pipelined_connection_impl.cc index 1feda75..65e2d606 100644 --- a/net/http/http_pipelined_connection_impl.cc +++ b/net/http/http_pipelined_connection_impl.cc @@ -528,10 +528,8 @@ int HttpPipelinedConnectionImpl::DoEvictPendingReadHeaders(int result) { } if (stream_info_map_[evicted_id].state == STREAM_READ_PENDING) { stream_info_map_[evicted_id].state = STREAM_READ_EVICTED; - QueueUserCallback(evicted_id, - stream_info_map_[evicted_id].read_headers_callback, - ERR_PIPELINE_EVICTION, - FROM_HERE); + stream_info_map_[evicted_id].read_headers_callback.Run( + ERR_PIPELINE_EVICTION); } } read_next_state_ = READ_STATE_NONE; diff --git a/net/http/http_pipelined_connection_impl_unittest.cc b/net/http/http_pipelined_connection_impl_unittest.cc index 8d68924..fbba3bc 100644 --- a/net/http/http_pipelined_connection_impl_unittest.cc +++ b/net/http/http_pipelined_connection_impl_unittest.cc @@ -1115,6 +1115,46 @@ TEST_F(HttpPipelinedConnectionImplTest, CloseBeforeReadCallbackRuns) { MessageLoop::current()->RunAllPending(); } +TEST_F(HttpPipelinedConnectionImplTest, NoGapBetweenCloseAndEviction) { + MockWrite writes[] = { + MockWrite(false, 0, "GET /close.html HTTP/1.1\r\n\r\n"), + MockWrite(false, 2, "GET /dummy.html HTTP/1.1\r\n\r\n"), + }; + MockRead reads[] = { + MockRead(false, 1, "HTTP/1.1 200 OK\r\n"), + MockRead(true, 3, "Content-Length: 7\r\n\r\n"), + }; + Initialize(reads, arraysize(reads), writes, arraysize(writes)); + + scoped_ptr close_stream(NewTestStream("close.html")); + scoped_ptr dummy_stream(NewTestStream("dummy.html")); + + HttpRequestHeaders headers; + HttpResponseInfo response; + EXPECT_EQ(OK, close_stream->SendRequest(headers, NULL, &response, + callback_.callback())); + + TestCompletionCallback close_callback; + EXPECT_EQ(ERR_IO_PENDING, + close_stream->ReadResponseHeaders(close_callback.callback())); + + EXPECT_EQ(OK, dummy_stream->SendRequest(headers, NULL, &response, + callback_.callback())); + + TestCompletionCallback dummy_callback; + EXPECT_EQ(ERR_IO_PENDING, + dummy_stream->ReadResponseHeaders(dummy_callback.callback())); + + close_stream->Close(true); + close_stream.reset(); + + EXPECT_TRUE(dummy_callback.have_result()); + EXPECT_EQ(ERR_PIPELINE_EVICTION, dummy_callback.WaitForResult()); + dummy_stream->Close(true); + dummy_stream.reset(); + pipeline_.reset(); +} + TEST_F(HttpPipelinedConnectionImplTest, RecoverFromDrainOnRedirect) { MockWrite writes[] = { MockWrite(false, 0, "GET /redirect.html HTTP/1.1\r\n\r\n"), -- cgit v1.1