net/spdy/spdy_session.cc - Issue 17004007: [SPDY] Fix SpdySession's handling of SYN_REPLY frames

Unified Diff: net/spdy/spdy_session.cc

Issue 17004007: [SPDY] Fix SpdySession's handling of SYN_REPLY frames (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Created 7 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: net/spdy/spdy_session.cc

diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc

index 9b10d2afc45b5d624a3e6e9253b5b4ed4c22775a..ba8018ca767be4ae1f73d0168e4ec0034f82f192 100644

--- a/net/spdy/spdy_session.cc

+++ b/net/spdy/spdy_session.cc

@@ -311,6 +311,26 @@ void SpdyStreamRequest::Reset() {

callback_.Reset();

}

+SpdySession::ActiveStreamInfo::ActiveStreamInfo()

+ : stream(NULL),

+ waiting_for_syn_reply(false) {}

+SpdySession::ActiveStreamInfo::ActiveStreamInfo(SpdyStream* stream)

+ : stream(stream),

+ waiting_for_syn_reply(stream->type() != SPDY_PUSH_STREAM) {}

+SpdySession::ActiveStreamInfo::~ActiveStreamInfo() {}

+SpdySession::PushedStreamInfo::PushedStreamInfo() : stream_id(0) {}

+SpdySession::PushedStreamInfo::PushedStreamInfo(

+ SpdyStreamId stream_id,

+ base::TimeTicks creation_time)

+ : stream_id(stream_id),

+ creation_time(creation_time) {}

+SpdySession::PushedStreamInfo::~PushedStreamInfo() {}

SpdySession::SpdySession(const SpdySessionKey& spdy_session_key,

SpdySessionPool* spdy_session_pool,

HttpServerProperties* http_server_properties,

@@ -705,7 +725,7 @@ scoped_ptr<SpdyFrame> SpdySession::CreateSynStream(

const SpdyHeaderBlock& headers) {

ActiveStreamMap::const_iterator it = active_streams_.find(stream_id);

CHECK(it != active_streams_.end());

- CHECK_EQ(it->second->stream_id(), stream_id);

+ CHECK_EQ(it->second.stream->stream_id(), stream_id);

SendPrefacePingIfNoneInFlight();

@@ -772,7 +792,7 @@ scoped_ptr<SpdyBuffer> SpdySession::CreateDataBuffer(SpdyStreamId stream_id,

SpdyDataFlags flags) {

ActiveStreamMap::const_iterator it = active_streams_.find(stream_id);

CHECK(it != active_streams_.end());

- SpdyStream* stream = it->second;

+ SpdyStream* stream = it->second.stream;

CHECK_EQ(stream->stream_id(), stream_id);

if (len < 0) {

@@ -900,7 +920,7 @@ void SpdySession::CloseActiveStream(SpdyStreamId stream_id, int status) {

if (status != OK) {

for (PushedStreamMap::iterator it = unclaimed_pushed_streams_.begin();

it != unclaimed_pushed_streams_.end(); ++it) {

- if (stream_id == it->second.first->stream_id()) {

+ if (stream_id == it->second.stream_id) {

unclaimed_pushed_streams_.erase(it);

break;

}

@@ -912,7 +932,7 @@ void SpdySession::CloseActiveStream(SpdyStreamId stream_id, int status) {

if (it == active_streams_.end())

return;

- scoped_ptr<SpdyStream> owned_stream(it->second);

+ scoped_ptr<SpdyStream> owned_stream(it->second.stream);

active_streams_.erase(it);

DeleteStream(owned_stream.Pass(), status);

@@ -1240,7 +1260,7 @@ void SpdySession::CloseAllStreamsAfter(SpdyStreamId last_good_stream_id,

break;

SpdyStreamId stream_id = it->first;

streams_abandoned_count_++;

- LogAbandonedStream(it->second, status);

+ LogAbandonedStream(it->second.stream, status);

CloseActiveStream(stream_id, status);

}

@@ -1436,7 +1456,8 @@ void SpdySession::InsertActivatedStream(scoped_ptr<SpdyStream> stream) {

SpdyStreamId stream_id = stream->stream_id();

DCHECK_NE(stream_id, 0u);

std::pair<ActiveStreamMap::iterator, bool> result =

- active_streams_.insert(std::make_pair(stream_id, stream.get()));

+ active_streams_.insert(

+ std::make_pair(stream_id, ActiveStreamInfo(stream.get())));

if (result.second) {

ignore_result(stream.release());

} else {

@@ -1475,14 +1496,21 @@ base::WeakPtr<SpdyStream> SpdySession::GetActivePushStream(

base::StatsCounter used_push_streams("spdy.claimed_push_streams");

PushedStreamMap::iterator it = unclaimed_pushed_streams_.find(path);

- if (it != unclaimed_pushed_streams_.end()) {

- net_log_.AddEvent(NetLog::TYPE_SPDY_STREAM_ADOPTED_PUSH_STREAM);

- base::WeakPtr<SpdyStream> stream = it->second.first->GetWeakPtr();

- unclaimed_pushed_streams_.erase(it);

- used_push_streams.Increment();

- return stream;

+ if (it == unclaimed_pushed_streams_.end())

+ return base::WeakPtr<SpdyStream>();

+ SpdyStreamId stream_id = it->second.stream_id;

+ unclaimed_pushed_streams_.erase(it);

+ ActiveStreamMap::iterator it2 = active_streams_.find(stream_id);

+ if (it2 == active_streams_.end()) {

+ NOTREACHED();

+ return base::WeakPtr<SpdyStream>();

}

- return base::WeakPtr<SpdyStream>();

+ net_log_.AddEvent(NetLog::TYPE_SPDY_STREAM_ADOPTED_PUSH_STREAM);

+ used_push_streams.Increment();

+ return it2->second.stream->GetWeakPtr();

}

bool SpdySession::GetSSLInfo(SSLInfo* ssl_info,

@@ -1521,7 +1549,7 @@ void SpdySession::OnStreamError(SpdyStreamId stream_id,

// We still want to reset the stream even if we don't know anything

// about it.

RequestPriority priority =

- (it == active_streams_.end()) ? IDLE : it->second->priority();

+ (it == active_streams_.end()) ? IDLE : it->second.stream->priority();

ResetStream(stream_id, priority, RST_STREAM_PROTOCOL_ERROR, description);

}

@@ -1542,6 +1570,17 @@ void SpdySession::OnStreamFrameData(SpdyStreamId stream_id,

if (it == active_streams_.end())

return;

+ SpdyStream* stream = it->second.stream;

+ CHECK_EQ(stream->stream_id(), stream_id);

+ if (it->second.waiting_for_syn_reply) {

+ const std::string& error = "Data received before SYN_REPLY.";

+ stream->LogStreamError(ERR_SPDY_PROTOCOL_ERROR, error);

+ ResetStream(stream_id, stream->priority(),

+ RST_STREAM_PROTOCOL_ERROR, error);

+ return;

+ }

scoped_ptr<SpdyBuffer> buffer;

if (data) {

DCHECK_GT(len, 0u);

@@ -1556,7 +1595,7 @@ void SpdySession::OnStreamFrameData(SpdyStreamId stream_id,

} else {

DCHECK_EQ(len, 0u);

}

- it->second->OnDataReceived(buffer.Pass());

+ stream->OnDataReceived(buffer.Pass());

}

void SpdySession::OnSettings(bool clear_persisted) {

@@ -1686,7 +1725,7 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,

associated_stream_id));

}

} else {

- GURL associated_url(associated_it->second->GetUrl());

+ GURL associated_url(associated_it->second.stream->GetUrl());

if (associated_url.GetOrigin() != gurl.GetOrigin()) {

ResetStream(stream_id, request_priority, RST_STREAM_REFUSED_STREAM,

base::StringPrintf(

@@ -1712,8 +1751,7 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,

stream->set_stream_id(stream_id);

DeleteExpiredPushedStreams();

- unclaimed_pushed_streams_[url] =

- std::pair<SpdyStream*, base::TimeTicks>(stream.get(), time_func_());

+ unclaimed_pushed_streams_[url] = PushedStreamInfo(stream_id, time_func_());

stream->set_response_received();

InsertActivatedStream(stream.Pass());

@@ -1725,7 +1763,7 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,

}

// Parse the headers.

- if (!Respond(headers, it->second))

+ if (!Respond(headers, it->second.stream))

return;

base::StatsCounter push_requests("spdy.pushed_streams");

@@ -1740,27 +1778,29 @@ void SpdySession::DeleteExpiredPushedStreams() {

if (time_func_() < next_unclaimed_push_stream_sweep_time_)

return;

- // Delete old streams.

+ // Gather old streams to delete.

base::TimeTicks minimum_freshness = time_func_() -

base::TimeDelta::FromSeconds(kMinPushedStreamLifetimeSeconds);

- PushedStreamMap::iterator it;

- for (it = unclaimed_pushed_streams_.begin();

- it != unclaimed_pushed_streams_.end(); ) {

- SpdyStream* stream = it->second.first;

- base::TimeTicks creation_time = it->second.second;

- // CloseActiveStream() will invalidate the current iterator, so

- // move to next.

- ++it;

- if (minimum_freshness > creation_time) {

- base::StatsCounter abandoned_push_streams(

- "spdy.abandoned_push_streams");

- base::StatsCounter abandoned_streams("spdy.abandoned_streams");

- abandoned_push_streams.Increment();

- abandoned_streams.Increment();

- streams_abandoned_count_++;

- CloseActiveStream(stream->stream_id(), ERR_INVALID_SPDY_STREAM);

- }

+ std::vector<SpdyStreamId> streams_to_close;

+ for (PushedStreamMap::iterator it = unclaimed_pushed_streams_.begin();

+ it != unclaimed_pushed_streams_.end(); ++it) {

+ if (minimum_freshness > it->second.creation_time)

+ streams_to_close.push_back(it->second.stream_id);

+ }

+ for (std::vector<SpdyStreamId>::const_iterator it = streams_to_close.begin();

+ it != streams_to_close.end(); ++it) {

+ base::StatsCounter abandoned_push_streams(

+ "spdy.abandoned_push_streams");

+ base::StatsCounter abandoned_streams("spdy.abandoned_streams");

+ abandoned_push_streams.Increment();

+ abandoned_streams.Increment();

+ streams_abandoned_count_++;

+ // CloseActiveStream() will remove the stream from

+ // |unclaimed_pushed_streams_|.

+ CloseActiveStream(*it, ERR_INVALID_SPDY_STREAM);

}

next_unclaimed_push_stream_sweep_time_ = time_func_() +

base::TimeDelta::FromSeconds(kMinPushedStreamLifetimeSeconds);

}

@@ -1782,17 +1822,19 @@ void SpdySession::OnSynReply(SpdyStreamId stream_id,

return;

}

- SpdyStream* stream = it->second;

+ SpdyStream* stream = it->second.stream;

CHECK_EQ(stream->stream_id(), stream_id);

- if (stream->response_received()) {

- stream->LogStreamError(ERR_SYN_REPLY_NOT_RECEIVED,

- "Received duplicate SYN_REPLY for stream.");

- RecordProtocolErrorHistogram(PROTOCOL_ERROR_SYN_REPLY_NOT_RECEIVED);

Ryan Hamilton 2013/06/21 15:35:37 Did this get lost or is it called by LogStreamErro

akalin 2013/06/21 18:30:44 Called by ResetStream

- CloseActiveStream(stream->stream_id(), ERR_SPDY_PROTOCOL_ERROR);

+ if (!it->second.waiting_for_syn_reply) {

+ const std::string& error =

+ "Received duplicate SYN_REPLY for stream.";

+ stream->LogStreamError(ERR_SPDY_PROTOCOL_ERROR, error);

+ ResetStream(stream_id, stream->priority(),

+ RST_STREAM_STREAM_IN_USE, error);

return;

}

stream->set_response_received();

+ it->second.waiting_for_syn_reply = false;

Respond(headers, stream);

}

@@ -1815,9 +1857,10 @@ void SpdySession::OnHeaders(SpdyStreamId stream_id,

return;

}

- CHECK_EQ(it->second->stream_id(), stream_id);

+ SpdyStream* stream = it->second.stream;

+ CHECK_EQ(stream->stream_id(), stream_id);

- int rv = it->second->OnHeaders(headers);

+ int rv = stream->OnHeaders(headers);

if (rv < 0) {

DCHECK_NE(rv, ERR_IO_PENDING);

CloseActiveStream(stream_id, rv);

@@ -1839,16 +1882,16 @@ void SpdySession::OnRstStream(SpdyStreamId stream_id,

return;

}

- CHECK_EQ(it->second->stream_id(), stream_id);

+ CHECK_EQ(it->second.stream->stream_id(), stream_id);

if (status == 0) {

- it->second->OnDataReceived(scoped_ptr<SpdyBuffer>());

+ it->second.stream->OnDataReceived(scoped_ptr<SpdyBuffer>());

} else if (status == RST_STREAM_REFUSED_STREAM) {

CloseActiveStream(stream_id, ERR_SPDY_SERVER_REFUSED_STREAM);

} else {

RecordProtocolErrorHistogram(

PROTOCOL_ERROR_RST_STREAM_FOR_NON_ACTIVE_STREAM);

- it->second->LogStreamError(

+ it->second.stream->LogStreamError(

ERR_SPDY_PROTOCOL_ERROR,

base::StringPrintf("SPDY stream closed with status: %d", status));

// TODO(mbelshe): Map from Spdy-protocol errors to something sensical.

@@ -1937,13 +1980,16 @@ void SpdySession::OnWindowUpdate(SpdyStreamId stream_id,

ActiveStreamMap::const_iterator it = active_streams_.find(stream_id);

if (it == active_streams_.end()) {

- // TODO(akalin): Record an error and close the session.

+ // NOTE: it may just be that the stream was cancelled.

LOG(WARNING) << "Received WINDOW_UPDATE for invalid stream " << stream_id;

return;

}

+ SpdyStream* stream = it->second.stream;

+ CHECK_EQ(stream->stream_id(), stream_id);

if (delta_window_size < 1u) {

- ResetStream(stream_id, it->second->priority(),

+ ResetStream(stream_id, it->second.stream->priority(),

RST_STREAM_FLOW_CONTROL_ERROR,

base::StringPrintf(

"Received WINDOW_UPDATE with an invalid "

@@ -1951,8 +1997,9 @@ void SpdySession::OnWindowUpdate(SpdyStreamId stream_id,

return;

}

- CHECK_EQ(it->second->stream_id(), stream_id);

- it->second->IncreaseSendWindowSize(static_cast<int32>(delta_window_size));

+ CHECK_EQ(it->second.stream->stream_id(), stream_id);

+ it->second.stream->IncreaseSendWindowSize(

+ static_cast<int32>(delta_window_size));

}

@@ -1961,8 +2008,9 @@ void SpdySession::SendStreamWindowUpdate(SpdyStreamId stream_id,

CHECK_GE(flow_control_state_, FLOW_CONTROL_STREAM);

ActiveStreamMap::const_iterator it = active_streams_.find(stream_id);

CHECK(it != active_streams_.end());

- CHECK_EQ(it->second->stream_id(), stream_id);

- SendWindowUpdateFrame(stream_id, delta_window_size, it->second->priority());

+ CHECK_EQ(it->second.stream->stream_id(), stream_id);

+ SendWindowUpdateFrame(

+ stream_id, delta_window_size, it->second.stream->priority());

}

void SpdySession::SendInitialSettings() {

@@ -2061,7 +2109,7 @@ void SpdySession::UpdateStreamsSendWindowSize(int32 delta_window_size) {

DCHECK_GE(flow_control_state_, FLOW_CONTROL_STREAM);

for (ActiveStreamMap::iterator it = active_streams_.begin();

it != active_streams_.end(); ++it) {

- it->second->AdjustSendWindowSize(delta_window_size);

+ it->second.stream->AdjustSendWindowSize(delta_window_size);

}

for (CreatedStreamSet::const_iterator it = created_streams_.begin();

@@ -2090,7 +2138,7 @@ void SpdySession::SendWindowUpdateFrame(SpdyStreamId stream_id,

CHECK_GE(flow_control_state_, FLOW_CONTROL_STREAM);

ActiveStreamMap::const_iterator it = active_streams_.find(stream_id);

if (it != active_streams_.end()) {

- CHECK_EQ(it->second->stream_id(), stream_id);

+ CHECK_EQ(it->second.stream->stream_id(), stream_id);

} else {

CHECK_EQ(flow_control_state_, FLOW_CONTROL_STREAM_AND_SESSION);

CHECK_EQ(stream_id, kSessionFlowControlStreamId);

@@ -2444,7 +2492,7 @@ void SpdySession::ResumeSendStalledStreams() {

// to its own send window) but that's okay -- it'll then be

// resumed once its send window increases.

if (it != active_streams_.end())

- it->second->PossiblyResumeIfSendStalled();

+ it->second.stream->PossiblyResumeIfSendStalled();

// The size should decrease unless we got send-stalled again.

if (!IsSendStalled())

« net/spdy/spdy_session.h ('K') | « net/spdy/spdy_session.h ('k') | net/spdy/spdy_stream.h » ('j') | no next file with comments »