diff options
author | mbelshe@chromium.org <mbelshe@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-12-21 21:01:12 +0000 |
---|---|---|
committer | mbelshe@chromium.org <mbelshe@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-12-21 21:01:12 +0000 |
commit | 92683b5a068ab7c7f58251dbee9b047314977bae (patch) | |
tree | 5f177d57e8b6f39473f1325610fd90ed9119b1a1 /net | |
parent | f57838fd46869064f0125bb33b1c4f7cae07019a (diff) | |
download | chromium_src-92683b5a068ab7c7f58251dbee9b047314977bae.zip chromium_src-92683b5a068ab7c7f58251dbee9b047314977bae.tar.gz chromium_src-92683b5a068ab7c7f58251dbee9b047314977bae.tar.bz2 |
Fix crash with CancelStream. The stream didn't inform
the FlipSession, and the FlipSession continued to try to
notify the stream of new data.
Also re-enable the CancelTransaction test, which did catch
the crash.
BUG=none
TEST=FlipNetworkTransactionTest.CancelTransaction
Review URL: http://codereview.chromium.org/506062
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35106 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net')
-rw-r--r-- | net/flip/flip_network_transaction_unittest.cc | 15 | ||||
-rw-r--r-- | net/flip/flip_session.cc | 14 | ||||
-rwxr-xr-x | net/flip/flip_stream.cc | 13 |
3 files changed, 26 insertions, 16 deletions
diff --git a/net/flip/flip_network_transaction_unittest.cc b/net/flip/flip_network_transaction_unittest.cc index 29d266c..77ed06c 100644 --- a/net/flip/flip_network_transaction_unittest.cc +++ b/net/flip/flip_network_transaction_unittest.cc @@ -39,7 +39,15 @@ class SessionDependencies { : host_resolver(new MockHostResolver), proxy_service(CreateNullProxyService()), ssl_config_service(new SSLConfigServiceDefaults), - flip_session_pool(new FlipSessionPool) {} + flip_session_pool(new FlipSessionPool) { + // Note: The CancelledTransaction test does cleanup by running all tasks + // in the message loop (RunAllPending). Unfortunately, that doesn't clean + // up tasks on the host resolver thread; and TCPConnectJob is currently + // not cancellable. Using synchronous lookups allows the test to shutdown + // cleanly. Until we have cancellable TCPConnectJobs, use synchronous + // lookups. + host_resolver->set_synchronous_mode(true); + } // Custom proxy service dependency. explicit SessionDependencies(ProxyService* proxy_service) @@ -441,10 +449,7 @@ TEST_F(FlipNetworkTransactionTest, ResponseWithoutSynReply) { EXPECT_EQ(ERR_SYN_REPLY_NOT_RECEIVED, out.rv); } -// TODO(willchan): Look into why TCPConnectJobs are still alive when this test -// goes away. They're calling into the ClientSocketFactory which doesn't exist -// anymore, so it crashes. -TEST_F(FlipNetworkTransactionTest, DISABLED_CancelledTransaction) { +TEST_F(FlipNetworkTransactionTest, CancelledTransaction) { MockWrite writes[] = { MockWrite(true, reinterpret_cast<const char*>(kGetSyn), arraysize(kGetSyn)), diff --git a/net/flip/flip_session.cc b/net/flip/flip_session.cc index a38e4f6..b6abbe9 100644 --- a/net/flip/flip_session.cc +++ b/net/flip/flip_session.cc @@ -395,6 +395,9 @@ bool FlipSession::CancelStream(flip::FlipStreamId stream_id) { if (!IsStreamActive(stream_id)) return false; + // TODO(mbelshe): We should send a FIN_STREAM control frame here + // so that the server can cancel a large send. + // TODO(mbelshe): Write a method for tearing down a stream // that cleans it out of the active list, the pending list, // etc. @@ -531,7 +534,11 @@ void FlipSession::OnWriteComplete(int result) { DCHECK_GT(result, static_cast<int>(flip::FlipFrame::size())); result -= static_cast<int>(flip::FlipFrame::size()); } - stream->OnWriteComplete(result); + + // It is possible that the stream was cancelled while we were writing + // to the socket. + if (!stream->cancelled()) + stream->OnWriteComplete(result); // Cleanup the write which just completed. in_flight_write_.release(); @@ -770,6 +777,7 @@ void FlipSession::OnStreamFrameData(flip::FlipStreamId stream_id, LOG(INFO) << "Flip data for stream " << stream_id << ", " << len << " bytes"; bool valid_stream = IsStreamActive(stream_id); if (!valid_stream) { + // NOTE: it may just be that the stream was cancelled. LOG(WARNING) << "Received data frame for invalid stream " << stream_id; return; } @@ -866,6 +874,7 @@ void FlipSession::OnSynReply(const flip::FlipSynReplyControlFrame* frame, flip::FlipStreamId stream_id = frame->stream_id(); bool valid_stream = IsStreamActive(stream_id); if (!valid_stream) { + // NOTE: it may just be that the stream was cancelled. LOG(WARNING) << "Received SYN_REPLY for invalid stream " << stream_id; return; } @@ -903,6 +912,7 @@ void FlipSession::OnSynReply(const flip::FlipSynReplyControlFrame* frame, scoped_refptr<FlipStream> stream = active_streams_[stream_id]; CHECK(stream->stream_id() == stream_id); + CHECK(!stream->cancelled()); HttpResponseInfo response; if (FlipHeadersToHttpResponse(*headers, &response)) { GetSSLInfo(&response.ssl_info); @@ -949,11 +959,13 @@ void FlipSession::OnFin(const flip::FlipFinStreamControlFrame* frame) { flip::FlipStreamId stream_id = frame->stream_id(); bool valid_stream = IsStreamActive(stream_id); if (!valid_stream) { + // NOTE: it may just be that the stream was cancelled. LOG(WARNING) << "Received FIN for invalid stream" << stream_id; return; } scoped_refptr<FlipStream> stream = active_streams_[stream_id]; CHECK(stream->stream_id() == stream_id); + CHECK(!stream->cancelled()); if (frame->status() == 0) { stream->OnDataReceived(NULL, 0); } else { diff --git a/net/flip/flip_stream.cc b/net/flip/flip_stream.cc index 9f76b95..f283241 100755 --- a/net/flip/flip_stream.cc +++ b/net/flip/flip_stream.cc @@ -146,16 +146,7 @@ void FlipStream::Cancel() { cancelled_ = true; user_callback_ = NULL; - // TODO(willchan): Should we also call FlipSession::CancelStream()? What - // makes more sense? If we cancel the stream, perhaps we should send the - // server a control packet to tell it to stop sending to the dead stream. But - // then does FlipSession deactivate the stream? It would log warnings for - // data packets for an invalid stream then, unless we maintained some data - // structure to keep track of cancelled stream ids. Ugh. Currently - // FlipStream does not call CancelStream(). We should free up all the memory - // associated with the stream though (ditch all the IOBuffers) and at all - // FlipSession's entrypoints into FlipStream check |cancelled_| and not copy - // the data. + session_->CancelStream(stream_id_); } void FlipStream::OnResponseReceived(const HttpResponseInfo& response) { @@ -231,6 +222,8 @@ bool FlipStream::OnDataReceived(const char* data, int length) { } void FlipStream::OnWriteComplete(int status) { + // TODO(mbelshe): Check for cancellation here. If we're cancelled, we + // should discontinue the DoLoop. DoLoop(status); } |