Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure.

net/spdy/spdy_http_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_http_stream.h"
 
 #include <algorithm>
 #include <list>
 #include <utility>
 
@@ skipping 284 matching lines @@
 void SpdyHttpStream::Cancel() {
   request_callback_.Reset();
   response_callback_.Reset();
   if (stream_.get()) {
     stream_->Cancel();
     DCHECK(!stream_.get());
   }
 }
 
 void SpdyHttpStream::OnRequestHeadersSent() {
-  if (!request_callback_.is_null())
+  if (!request_callback_.is_null()) {
+    // Avoid reseting the callback as long as it is used on errors in
+    // OnRequestBodyReadCompleted() method.

[mmenke, 2016/06/01 17:24:07]

We should not be invoking the callback twice, under any circumstances.  We
should actually change the code to only run the callback after the body has been
sent.  That's what the HttpStreamParser does as well.  Weird that the behavior
diverges here.

Also unfortunate that our usual net/ callback test infrastructure doesn't catch
unexpected callback invocations.  :(

[maksims (do not use this acc), 2016/06/03 11:39:08]

Done.

+    const CompletionCallback cb = request_callback_;
     DoRequestCallback(OK);
+    request_callback_ = cb;
+  }
 
   // TODO(akalin): Do this immediately after sending the request
   // headers.
   if (HasUploadData())
     ReadAndSendRequestBodyData();
 }
 
 SpdyResponseHeadersStatus SpdyHttpStream::OnResponseHeadersUpdated(
     const SpdyHeaderBlock& response_headers) {
   CHECK_EQ(response_headers_status_, RESPONSE_HEADERS_ARE_INCOMPLETE);
@@ skipping 124 matching lines @@
     return;
 
   // Read the data from the request body stream.
   const int rv = request_info_->upload_data_stream
       ->Read(request_body_buf_.get(),
              request_body_buf_->size(),
              base::Bind(&SpdyHttpStream::OnRequestBodyReadCompleted,
                         weak_factory_.GetWeakPtr()));
 
   if (rv != ERR_IO_PENDING) {
-    // ERR_IO_PENDING is the only possible error.
-    CHECK_GE(rv, 0);
+    // UploadDataStream::Read() can fail reading data
     OnRequestBodyReadCompleted(rv);
   }
 }
 
+void SpdyHttpStream::ResetStreamInternal() {
+  spdy_session_->ResetStream(stream()->stream_id(), RST_STREAM_INTERNAL_ERROR,
+                             std::string());
+}
+
 void SpdyHttpStream::OnRequestBodyReadCompleted(int status) {
+  if (status < 0) {
+    DCHECK_NE(ERR_IO_PENDING, status);
+    if (!request_callback_.is_null()) {
+      DoRequestCallback(status);
+    }
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&SpdyHttpStream::ResetStreamInternal,
+                              weak_factory_.GetWeakPtr()));

[mmenke, 2016/06/01 17:24:07]

Should do this first - invoking |request_callback_| may actually delete |this|. 
Could add a test for that case.  (Didn't suggest it for HttpStreamParser because
it shares code to get that behavior, while for this class, the paths have to
invoke the callbacks themselves, since there's no DoLoop)

[maksims (do not use this acc), 2016/06/02 12:43:12]

Do you mean -
base::WeakPtr<SpdyHttpStream> weak_ptr = weak_factory_.GetWeakPtr();
if (!request_callback_.is_null()) {
..
}
CHECK(weak_ptr);
?

[mmenke, 2016/06/02 15:02:10]

No, I mean do the PostTask to reset the stream, and then invoke the callback.

[maksims (do not use this acc), 2016/06/03 11:39:08]

Done.

+    return;
+  }
+
   CHECK_GE(status, 0);
   request_body_buf_size_ = status;
   const bool eof = request_info_->upload_data_stream->IsEOF();
   // Only the final fame may have a length of 0.
   if (eof) {
     CHECK_GE(request_body_buf_size_, 0);
   } else {
     CHECK_GT(request_body_buf_size_, 0);
   }
   stream_->SendData(request_body_buf_.get(),
@@ skipping 116 matching lines @@
   details->connection_info = HttpResponseInfo::CONNECTION_INFO_HTTP2;
   return;
 }
 
 void SpdyHttpStream::SetPriority(RequestPriority priority) {
   // TODO(akalin): Plumb this through to |stream_request_| and
   // |stream_|.
 }
 
 }  // namespace net