Fix out of order SYN_STEAM frames.

net/spdy/spdy_stream.cc

Index: net/spdy/spdy_stream.cc
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 2698603fe78e1007cc7d9d19ecfd70d89d46c27a..a8ab51c63759bcbd9a8238cc7b44bb5effe8a015 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -72,11 +72,10 @@ bool ContainsUpperAscii(const std::string& str) {
 }  // namespace
 
 SpdyStream::SpdyStream(SpdySession* session,
-                       SpdyStreamId stream_id,
                        bool pushed,
                        const BoundNetLog& net_log)
     : continue_buffering_data_(true),
-      stream_id_(stream_id),
+      stream_id_(0),
       priority_(HIGHEST),
       slot_(0),
       stalled_by_flow_control_(false),
@@ -99,8 +98,54 @@ SpdyStream::SpdyStream(SpdySession* session,
       domain_bound_cert_type_(CLIENT_CERT_INVALID_TYPE) {
 }
 
+RequestPriority SpdyStream::GetPriority() const {
+  return priority();
+}
+
+SpdyFrame* SpdyStream::ProduceNextFrame() {
+  if (io_state_ == STATE_SEND_DOMAIN_BOUND_CERT_COMPLETE) {
+    CHECK(request_.get());
+    std::string origin = GetUrl().GetOrigin().spec();
+    origin.erase(origin.length() - 1);  // trim trailing slash
+    SpdyCredentialControlFrame* frame = session_->CreateCredentialFrame(
+        origin, domain_bound_cert_type_, domain_bound_private_key_,
+        domain_bound_cert_, priority_);

[ramant (doing other things), 2012/06/20 01:50:40]

nit: consider adding a CHECK_GT(stream_id_, 0u)?

[Ryan Hamilton, 2012/06/21 18:14:58]

Done.

+    return frame;
+  } else if (io_state_ == STATE_SEND_HEADERS_COMPLETE) {
+    SpdyControlFlags flags =
+        has_upload_data_ ? CONTROL_FLAG_NONE : CONTROL_FLAG_FIN;
+
+    CHECK(request_.get());
+    SpdySynStreamControlFrame* frame = session_->CreateSynStream(
+        stream_id_, priority_, slot_, flags, request_);
+    set_stream_id(frame->stream_id());

[ramant (doing other things), 2012/06/20 01:50:40]

nit: can we do CHECK_EQ(stream_id_, 0u)? Or do we want to overwrite the
stream_id_ with frame's stream_id? or should we assert stream_id_ is same as
frame->stream_id()? thanks.

[Ryan Hamilton, 2012/06/21 18:14:58]

Sorry, this line of code was a bug.  We should not be doing the set_stream_id().
 I can add a check there, but that seems a bit silly since we dont' do that for
priority, flags, slot, or the request headers.

+    send_time_ = base::TimeTicks::Now();
+    return frame;
+  } else {
+    CHECK(!cancelled());
+    // We must need to write stream data.
+    // Until the headers have been completely sent, we can not be sure
+    // that our stream_id is correct.
+    DCHECK_GT(io_state_, STATE_SEND_HEADERS_COMPLETE);
+    DCHECK_GT(stream_id_, 0u);
+    DCHECK(!pending_data_frames_.empty());
+    SpdyFrame* frame = pending_data_frames_.front();
+    pending_data_frames_.pop_front();
+    return frame;
+  }
+}
+
+SpdyStream* SpdyStream::GetSpdyStream() const {
+  return const_cast<SpdyStream*>(this);
+}
+
 SpdyStream::~SpdyStream() {
   UpdateHistograms();
+  while (!pending_data_frames_.empty()) {
+    SpdyFrame* frame = pending_data_frames_.back();
+    pending_data_frames_.pop_back();
+    delete frame;
+  }
 }
 
 void SpdyStream::SetDelegate(Delegate* delegate) {
@@ -495,7 +540,10 @@ void SpdyStream::Cancel() {
 }
 
 void SpdyStream::Close() {
-  session_->CloseStream(stream_id_, net::OK);
+  if (stream_id_ != 0)
+    session_->CloseStream(stream_id_, net::OK);
+  else
+    session_->CloseCreatedStream(this, OK);
 }
 
 int SpdyStream::SendRequest(bool has_upload_data) {
@@ -522,7 +570,13 @@ int SpdyStream::WriteStreamData(IOBuffer* data, int length,
   // Until the headers have been completely sent, we can not be sure
   // that our stream_id is correct.
   DCHECK_GT(io_state_, STATE_SEND_HEADERS_COMPLETE);
-  return session_->WriteStreamData(stream_id_, data, length, flags);
+  DCHECK_GT(stream_id_, 0u);

[ramant (doing other things), 2012/06/20 01:50:40]

nit: can we change it to CHECK_GT to assert that set_stream_id was called before
WriteStreamData in non-debug builds also?

[Ryan Hamilton, 2012/06/21 18:14:58]

Done, but I feel like I don't have a good sense for when we want to use DCHECK
and when we want to use CHECK.  I'd love it if the style guide had some advice
here.  Perhaps I'll send a message to chromium dev.

+
+  pending_data_frames_.push_back(
+      session_->CreateDataFrame(stream_id_, data, length, flags));
+
+  session_->SetStreamHasWriteAvailable(this);
+  return ERR_IO_PENDING;
 }
 
 bool SpdyStream::GetSSLInfo(SSLInfo* ssl_info,
@@ -668,12 +722,12 @@ int SpdyStream::DoSendDomainBoundCert() {
   CHECK(request_.get());
   std::string origin = GetUrl().GetOrigin().spec();
   origin.erase(origin.length() - 1);  // trim trailing slash
-  int rv =  session_->WriteCredentialFrame(
+  SpdyCredentialControlFrame* frame = session_->CreateCredentialFrame(
       origin, domain_bound_cert_type_, domain_bound_private_key_,

[ramant (doing other things), 2012/06/20 01:50:40]

nit: SpdyStream::ProduceNextFrame also seems to be doing CreateCredentialFrame.
Looks like we don't need to create CredentialFrame here.

[Ryan Hamilton, 2012/06/21 18:14:58]

Done.

       domain_bound_cert_, priority_);
-  if (rv != ERR_IO_PENDING)
-    return rv;
-  return OK;
+  frame->length();
+  session_->SetStreamHasWriteAvailable(this);
+  return ERR_IO_PENDING;
 }
 
 int SpdyStream::DoSendDomainBoundCertComplete(int result) {
@@ -687,18 +741,7 @@ int SpdyStream::DoSendDomainBoundCertComplete(int result) {
 int SpdyStream::DoSendHeaders() {
   CHECK(!cancelled_);
 
-  SpdyControlFlags flags = CONTROL_FLAG_NONE;
-  if (!has_upload_data_)
-    flags = CONTROL_FLAG_FIN;
-
-  CHECK(request_.get());
-  int result = session_->WriteSynStream(
-      stream_id_, priority_, slot_, flags,
-      request_);
-  if (result != ERR_IO_PENDING)
-    return result;
-
-  send_time_ = base::TimeTicks::Now();
+  session_->SetStreamHasWriteAvailable(this);
   io_state_ = STATE_SEND_HEADERS_COMPLETE;
   return ERR_IO_PENDING;
 }