Skip to content

Commit

Permalink
[SPDY] Send a RST_STREAM frame before closing the associated stream
Browse files Browse the repository at this point in the history 
Closing the stream may close the corresponding session.

This is a speculative fix for the bug below.

BUG=263691
R=rch@chromium.org

Review URL: https://codereview.chromium.org/20163002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@213561 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
akalin@chromium.org committed Jul 25, 2013
1 parent 675ee34 commit d8cef85
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 33 deletions.
55 changes: 34 additions & 21 deletions net/spdy/spdy_session.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1033,21 +1033,22 @@ void SpdySession::CloseCreatedStreamIterator(CreatedStreamSet::iterator it,
void SpdySession::ResetStreamIterator(ActiveStreamMap::iterator it,
SpdyRstStreamStatus status,
const std::string& description) {
// Send the RST_STREAM frame first as CloseActiveStreamIterator()
// may close us.
SpdyStreamId stream_id = it->first;
RequestPriority priority = it->second.stream->priority();
EnqueueResetStreamFrame(stream_id, priority, status, description);

// Removes any pending writes for the stream except for possibly an
// in-flight one.
CloseActiveStreamIterator(it, ERR_SPDY_PROTOCOL_ERROR);

SendResetStreamFrame(stream_id, priority, status, description);
}

void SpdySession::SendResetStreamFrame(SpdyStreamId stream_id,
RequestPriority priority,
SpdyRstStreamStatus status,
const std::string& description) {
void SpdySession::EnqueueResetStreamFrame(SpdyStreamId stream_id,
RequestPriority priority,
SpdyRstStreamStatus status,
const std::string& description) {
DCHECK_NE(stream_id, 0u);
DCHECK(active_streams_.find(stream_id) == active_streams_.end());

net_log().AddEvent(
NetLog::TYPE_SPDY_SESSION_SEND_RST_STREAM,
Expand Down Expand Up @@ -1394,7 +1395,11 @@ void SpdySession::DcheckClosed() const {
void SpdySession::StartGoingAway(SpdyStreamId last_good_stream_id,
Error status) {
DCHECK_GE(availability_state_, STATE_GOING_AWAY);

// The loops below are carefully written to avoid reentrancy problems.
//
// TODO(akalin): Any of the functions below can cause |this| to be
// deleted, so handle that below (and add tests for it).

for (int i = 0; i < NUM_PRIORITIES; ++i) {
PendingStreamRequestQueue queue;
Expand Down Expand Up @@ -1688,8 +1693,14 @@ void SpdySession::DeleteStream(scoped_ptr<SpdyStream> stream, int status) {

write_queue_.RemovePendingWritesForStream(stream->GetWeakPtr());

// |stream->OnClose()| may end up closing |this|, so detect that.
base::WeakPtr<SpdySession> weak_this = GetWeakPtr();

stream->OnClose(status);

if (!weak_this)
return;

switch (availability_state_) {
case STATE_AVAILABLE:
ProcessPendingStreamRequests();
Expand Down Expand Up @@ -1773,7 +1784,7 @@ void SpdySession::OnStreamError(SpdyStreamId stream_id,
if (it == active_streams_.end()) {
// We still want to send a frame to reset the stream even if we
// don't know anything about it.
SendResetStreamFrame(
EnqueueResetStreamFrame(
stream_id, IDLE, RST_STREAM_PROTOCOL_ERROR, description);
return;
}
Expand Down Expand Up @@ -1947,18 +1958,18 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
if (availability_state_ == STATE_GOING_AWAY) {
// TODO(akalin): This behavior isn't in the SPDY spec, although it
// probably should be.
SendResetStreamFrame(stream_id, request_priority,
RST_STREAM_REFUSED_STREAM,
"OnSyn received when going away");
EnqueueResetStreamFrame(stream_id, request_priority,
RST_STREAM_REFUSED_STREAM,
"OnSyn received when going away");
return;
}

if (associated_stream_id == 0) {
std::string description = base::StringPrintf(
"Received invalid OnSyn associated stream id %d for stream %d",
associated_stream_id, stream_id);
SendResetStreamFrame(stream_id, request_priority,
RST_STREAM_REFUSED_STREAM, description);
EnqueueResetStreamFrame(stream_id, request_priority,
RST_STREAM_REFUSED_STREAM, description);
return;
}

Expand All @@ -1969,16 +1980,17 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
// Verify that the response had a URL for us.
GURL gurl = GetUrlFromHeaderBlock(headers, GetProtocolVersion(), true);
if (!gurl.is_valid()) {
SendResetStreamFrame(stream_id, request_priority, RST_STREAM_PROTOCOL_ERROR,
"Pushed stream url was invalid: " + gurl.spec());
EnqueueResetStreamFrame(
stream_id, request_priority, RST_STREAM_PROTOCOL_ERROR,
"Pushed stream url was invalid: " + gurl.spec());
return;
}

// Verify we have a valid stream association.
ActiveStreamMap::iterator associated_it =
active_streams_.find(associated_stream_id);
if (associated_it == active_streams_.end()) {
SendResetStreamFrame(
EnqueueResetStreamFrame(
stream_id, request_priority, RST_STREAM_INVALID_STREAM,
base::StringPrintf(
"Received OnSyn with inactive associated stream %d",
Expand All @@ -1992,7 +2004,7 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
if (trusted_spdy_proxy_.Equals(host_port_pair())) {
// Disallow pushing of HTTPS content.
if (gurl.SchemeIs("https")) {
SendResetStreamFrame(
EnqueueResetStreamFrame(
stream_id, request_priority, RST_STREAM_REFUSED_STREAM,
base::StringPrintf(
"Rejected push of Cross Origin HTTPS content %d",
Expand All @@ -2001,7 +2013,7 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
} else {
GURL associated_url(associated_it->second.stream->GetUrlFromHeaders());
if (associated_url.GetOrigin() != gurl.GetOrigin()) {
SendResetStreamFrame(
EnqueueResetStreamFrame(
stream_id, request_priority, RST_STREAM_REFUSED_STREAM,
base::StringPrintf(
"Rejected Cross Origin Push Stream %d",
Expand All @@ -2015,9 +2027,10 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
unclaimed_pushed_streams_.lower_bound(gurl);
if (pushed_it != unclaimed_pushed_streams_.end() &&
pushed_it->first == gurl) {
SendResetStreamFrame(stream_id, request_priority, RST_STREAM_PROTOCOL_ERROR,
"Received duplicate pushed stream with url: " +
gurl.spec());
EnqueueResetStreamFrame(
stream_id, request_priority, RST_STREAM_PROTOCOL_ERROR,
"Received duplicate pushed stream with url: " +
gurl.spec());
return;
}

Expand Down
18 changes: 11 additions & 7 deletions net/spdy/spdy_session.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,17 +590,21 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
// stream may hold the last reference to the session.
void CloseCreatedStreamIterator(CreatedStreamSet::iterator it, int status);

// Calls CloseActiveStreamIterator() and then SendResetStreamFrame().
// Calls EnqueueResetStreamFrame() and then
// CloseActiveStreamIterator().
void ResetStreamIterator(ActiveStreamMap::iterator it,
SpdyRstStreamStatus status,
const std::string& description);

// Send a RST_STREAM frame with the given parameters. There must be
// no active stream with the given ID.
void SendResetStreamFrame(SpdyStreamId stream_id,
RequestPriority priority,
SpdyRstStreamStatus status,
const std::string& description);
// Send a RST_STREAM frame with the given parameters. There should
// either be no active stream with the given ID, or that active
// stream should be closed shortly after this function is called.
//
// TODO(akalin): Rename this to EnqueueResetStreamFrame().
void EnqueueResetStreamFrame(SpdyStreamId stream_id,
RequestPriority priority,
SpdyRstStreamStatus status,
const std::string& description);

// Calls DoReadLoop and then if |availability_state_| is
// STATE_CLOSED, calls RemoveFromPool().
Expand Down
77 changes: 77 additions & 0 deletions net/spdy/spdy_session_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1833,6 +1833,83 @@ TEST_P(SpdySessionTest, CloseSessionWithTwoActivatedMutuallyClosingStreams) {
EXPECT_TRUE(session == NULL);
}

// Delegate that closes a given session when the stream is closed.
class SessionClosingDelegate : public test::StreamDelegateDoNothing {
public:
SessionClosingDelegate(const base::WeakPtr<SpdyStream>& stream,
const base::WeakPtr<SpdySession>& session_to_close)
: StreamDelegateDoNothing(stream),
session_to_close_(session_to_close) {}

virtual ~SessionClosingDelegate() {}

virtual void OnClose(int status) OVERRIDE {
session_to_close_->CloseSessionOnError(ERR_ABORTED, "Aborted");
}

private:
base::WeakPtr<SpdySession> session_to_close_;
};

// Close an activated stream that closes its session. Nothing should
// blow up. This is a regression test for http://crbug.com/263691 .
TEST_P(SpdySessionTest, CloseActivatedStreamThatClosesSession) {
session_deps_.host_resolver->set_synchronous_mode(true);

MockConnect connect_data(SYNCHRONOUS, OK);

scoped_ptr<SpdyFrame> req(
spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, MEDIUM, true));
MockWrite writes[] = {
CreateMockWrite(*req, 0),
};

MockRead reads[] = {
MockRead(ASYNC, 0, 1) // EOF
};
DeterministicSocketData data(reads, arraysize(reads),
writes, arraysize(writes));
data.set_connect_data(connect_data);
session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);

SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);

CreateDeterministicNetworkSession();

base::WeakPtr<SpdySession> session =
CreateInsecureSpdySession(http_session_, key_, BoundNetLog());

GURL url("http://www.google.com");
base::WeakPtr<SpdyStream> spdy_stream =
CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
session, url, MEDIUM, BoundNetLog());
ASSERT_TRUE(spdy_stream.get() != NULL);
EXPECT_EQ(0u, spdy_stream->stream_id());

SessionClosingDelegate delegate(spdy_stream, session);
spdy_stream->SetDelegate(&delegate);

scoped_ptr<SpdyHeaderBlock> headers(
spdy_util_.ConstructGetHeaderBlock(url.spec()));
spdy_stream->SendRequestHeaders(headers.Pass(), NO_MORE_DATA_TO_SEND);
EXPECT_TRUE(spdy_stream->HasUrlFromHeaders());

EXPECT_EQ(0u, spdy_stream->stream_id());

data.RunFor(1);

EXPECT_EQ(1u, spdy_stream->stream_id());

// Ensure we don't crash while closing the stream (which closes the
// session).
spdy_stream->Cancel();

EXPECT_EQ(NULL, spdy_stream.get());
EXPECT_TRUE(delegate.StreamIsClosed());
EXPECT_TRUE(session == NULL);
}

TEST_P(SpdySessionTest, VerifyDomainAuthentication) {
session_deps_.host_resolver->set_synchronous_mode(true);

Expand Down
4 changes: 0 additions & 4 deletions net/spdy/spdy_stream.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,6 @@ void SpdyStream::SetDelegate(Delegate* delegate) {
}
}

SpdyStream::Delegate* SpdyStream::GetDelegate() {
return delegate_;
}

void SpdyStream::PushedStreamReplayData() {
DCHECK_EQ(type_, SPDY_PUSH_STREAM);
DCHECK_NE(stream_id_, 0u);
Expand Down
1 change: 0 additions & 1 deletion net/spdy/spdy_stream.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ class NET_EXPORT_PRIVATE SpdyStream {
// than once. For push streams, calling this may cause buffered data
// to be sent to the delegate (from a posted task).
void SetDelegate(Delegate* delegate);
Delegate* GetDelegate();

// Detach the delegate from the stream, which must not yet be
// closed, and cancel it.
Expand Down

0 comments on commit d8cef85

Please sign in to comment.