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

net/spdy/spdy_http_stream.cc

Index: net/spdy/spdy_http_stream.cc
diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 259581a5229173cc2e7fcbc84963396f357c0d80..465dfd5a2dd93fa64b13564de5762b13b85969ce 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -302,8 +302,13 @@ void SpdyHttpStream::Cancel() {
 }
 
 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.
@@ -448,13 +453,28 @@ void SpdyHttpStream::ReadAndSendRequestBodyData() {
                         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();