Enable SpdyStream's HALF_CLOSED_REMOTE state.

net/spdy/spdy_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/spdy/spdy_stream.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 385 matching lines @@
       // headers once we've finished sending the request headers.
       if (io_state_ == STATE_IDLE) {
         session_->ResetStream(stream_id_, RST_STREAM_PROTOCOL_ERROR,
                               "Response received before request sent");
         return ERR_SPDY_PROTOCOL_ERROR;
       }
       break;
 
     case SPDY_REQUEST_RESPONSE_STREAM:
       // For a request/response stream, we're ready for the response
-      // headers once we've finished sending the request headers and
-      // the request body (if we have one).
-      if (io_state_ != STATE_HALF_CLOSED_LOCAL) {
+      // headers once we've finished sending the request headers.
+      if (io_state_ == STATE_IDLE) {
         session_->ResetStream(stream_id_, RST_STREAM_PROTOCOL_ERROR,
                               "Response received before request sent");
         return ERR_SPDY_PROTOCOL_ERROR;
       }
       break;
 
     case SPDY_PUSH_STREAM:
       // Push streams transition to a locally half-closed state upon headers.
       // We must continue to buffer data while waiting for a call to
       // SetDelegate() (which may not ever happen).
@@ skipping 59 matching lines @@
     LogStreamError(ERR_SPDY_PROTOCOL_ERROR,
                    "Data received with incomplete headers.");
     session_->CloseActiveStream(stream_id_, ERR_SPDY_PROTOCOL_ERROR);
     return;
   }
 
   CHECK(!IsClosed());
 
   if (!buffer) {
     metrics_.StopStream();
-    // TODO(jgraettinger): Closing from OPEN preserves current SpdyStream
-    // behavior, but is incorrect HTTP/2 behavior. |io_state_| should be
-    // HALF_CLOSED_REMOTE. To be changed in a subsequent CL.
-    if (io_state_ == STATE_OPEN || io_state_ == STATE_HALF_CLOSED_LOCAL) {
+    if (io_state_ == STATE_OPEN) {
+      io_state_ = STATE_HALF_CLOSED_REMOTE;
+    } else if (io_state_ == STATE_HALF_CLOSED_LOCAL) {
       io_state_ = STATE_CLOSED;
       // Deletes |this|.
       session_->CloseActiveStream(stream_id_, OK);
-      return;
+    } else {
+      NOTREACHED() << io_state_;
     }
-    NOTREACHED() << io_state_;
     return;
   }
 
   size_t length = buffer->GetRemainingSize();
   DCHECK_LE(length, session_->GetDataFrameMaximumPayload());
   if (session_->flow_control_state() >= SpdySession::FLOW_CONTROL_STREAM) {
     DecreaseRecvWindowSize(static_cast<int32>(length));
     buffer->AddConsumeCallback(
         base::Bind(&SpdyStream::OnReadBufferConsumed, GetWeakPtr()));
   }
@@ skipping 20 matching lines @@
         frame_type == DATA) << frame_type;
 
   int result = (frame_type == SYN_STREAM) ?
       OnRequestHeadersSent() : OnDataSent(frame_size);
   if (result == ERR_IO_PENDING) {
     // The write operation hasn't completed yet.
     return;
   }
 
   if (pending_send_status_ == NO_MORE_DATA_TO_SEND) {
-    // TODO(jgraettinger): Once HALF_CLOSED_REMOTE is added,
-    // we'll need to handle transitions to fully closed here.
-    CHECK_EQ(io_state_, STATE_OPEN);
-    io_state_ = STATE_HALF_CLOSED_LOCAL;
+    if(io_state_ == STATE_OPEN) {
+      io_state_ = STATE_HALF_CLOSED_LOCAL;
+    } else if(io_state_ == STATE_HALF_CLOSED_REMOTE) {
+      io_state_ = STATE_CLOSED;
+    } else {
+      NOTREACHED() << io_state_;
+    }
   }
 
-  // Notify delegate of write completion. May destroy |this|.
+  // Notify delegate of write completion. Must not destroy |this|.

[Johnny, 2014/02/14 00:07:08]

This looks scary on the surface, but I mixed up this comment in the previous
"Remove SpdyStream::DoLoop()" CL. Prior to that, Fred had comments on the calls
to delegate_->OnRequestHeadersSent() and delegate_->OnDataSent(), pointing out
that they _couldn't_ destroy |this|.

[Ryan Hamilton, 2014/02/14 01:47:22]

Makes sense.  I wonder if we could have a member variable like:
  bool destruction_allowed_
which defaults to true, and which we set to false around this block of code. 
Then the destructor could CHECK(destruction_allowed_) to ensure that we don't
screw this up.  hopefully a use-after-free would be detected, so perhaps this is
not necessary?

[Johnny, 2014/02/14 16:27:30]

Sure. I've added a |weak_this| guard instead. Same effect without the new
member.

On 2014/02/14 01:47:22, Ryan Hamilton wrote:

   CHECK(delegate_);
   if (frame_type == SYN_STREAM) {
     delegate_->OnRequestHeadersSent();
   } else {
     delegate_->OnDataSent();
   }
+
+  if (io_state_ == STATE_CLOSED) {
+    // Deletes |this|.
+    session_->CloseActiveStream(stream_id_, OK);
+  }
 }
 
 int SpdyStream::OnRequestHeadersSent() {
   CHECK_EQ(io_state_, STATE_IDLE);
   CHECK_NE(stream_id_, 0u);
 
   io_state_ = STATE_OPEN;
   return OK;
 }
 
 int SpdyStream::OnDataSent(size_t frame_size) {
-  CHECK_EQ(io_state_, STATE_OPEN);
+  CHECK(io_state_ == STATE_OPEN ||
+        io_state_ == STATE_HALF_CLOSED_REMOTE) << io_state_;
 
   size_t frame_payload_size = frame_size - session_->GetDataFrameMinimumSize();
 
   CHECK_GE(frame_size, session_->GetDataFrameMinimumSize());
   CHECK_LE(frame_payload_size, session_->GetDataFrameMaximumPayload());
 
   send_bytes_ += frame_payload_size;
 
   // If more data is available to send, dispatch it and
   // return that the write operation is still ongoing.
@@ skipping 74 matching lines @@
       scoped_ptr<SpdyBufferProducer>(
           new SynStreamBufferProducer(GetWeakPtr())));
   return ERR_IO_PENDING;
 }
 
 void SpdyStream::SendData(IOBuffer* data,
                           int length,
                           SpdySendStatus send_status) {
   CHECK_NE(type_, SPDY_PUSH_STREAM);
   CHECK_EQ(pending_send_status_, MORE_DATA_TO_SEND);
-  CHECK_EQ(io_state_, STATE_OPEN);
+  CHECK(io_state_ == STATE_OPEN ||
+        io_state_ == STATE_HALF_CLOSED_REMOTE) << io_state_;
   CHECK(!pending_send_data_.get());
   pending_send_data_ = new DrainableIOBuffer(data, length);
   pending_send_status_ = send_status;
   QueueNextDataFrame();
 }
 
 bool SpdyStream::GetSSLInfo(SSLInfo* ssl_info,
                             bool* was_npn_negotiated,
                             NextProto* protocol_negotiated) {
   return session_->GetSSLInfo(
@@ skipping 21 matching lines @@
 bool SpdyStream::IsClosed() const {
   return io_state_ == STATE_CLOSED;
 }
 
 bool SpdyStream::IsLocallyClosed() const {
   return io_state_ == STATE_HALF_CLOSED_LOCAL_UNCLAIMED ||
       io_state_ == STATE_HALF_CLOSED_LOCAL ||
       io_state_ == STATE_CLOSED;
 }
 
-bool SpdyStream::IsIdleTemporaryRename() const {
+bool SpdyStream::IsIdle() const {
   return io_state_ == STATE_IDLE;
 }
 
 bool SpdyStream::IsOpen() const {
   return io_state_ == STATE_OPEN;
 }
 
 NextProto SpdyStream::GetProtocol() const {
   return session_->protocol();
 }
@@ skipping 45 matching lines @@
   UMA_HISTOGRAM_TIMES("Net.SpdyStreamTime",
                       recv_last_byte_time_ - effective_send_time);
 
   UMA_HISTOGRAM_COUNTS("Net.SpdySendBytes", send_bytes_);
   UMA_HISTOGRAM_COUNTS("Net.SpdyRecvBytes", recv_bytes_);
 }
 
 void SpdyStream::QueueNextDataFrame() {
   // Until the request has been completely sent, we cannot be sure
   // that our stream_id is correct.
-  CHECK_EQ(io_state_, STATE_OPEN);
+  CHECK(io_state_ == STATE_OPEN ||
+        io_state_ == STATE_HALF_CLOSED_REMOTE) << io_state_;
   CHECK_GT(stream_id_, 0u);
   CHECK(pending_send_data_.get());
   CHECK_GT(pending_send_data_->BytesRemaining(), 0);
 
   SpdyDataFlags flags =
       (pending_send_status_ == NO_MORE_DATA_TO_SEND) ?
       DATA_FLAG_FIN : DATA_FLAG_NONE;
   scoped_ptr<SpdyBuffer> data_buffer(
       session_->CreateDataBuffer(stream_id_,
                                  pending_send_data_.get(),
@@ skipping 98 matching lines @@
       description = base::StringPrintf("Unknown state 0x%08X (%u)", state,
                                        state);
       break;
   }
   return description;
 }
 
 #undef STATE_CASE
 
 }  // namespace net