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

net/http/http_stream_parser.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/http/http_stream_parser.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 255 matching lines @@
       // large enough to hold the encoded chunk.
       request_body_read_buf_ =
           new SeekableIOBuffer(kRequestBodyBufferSize - kChunkHeaderFooterSize);
     } else {
       // No need to encode request body, just send the raw data.
       request_body_read_buf_ = request_body_send_buf_;
     }
   }
 
   io_state_ = STATE_SEND_HEADERS;
-
   // If we have a small request body, then we'll merge with the headers into a
   // single write.
   bool did_merge = false;
   if (ShouldMergeRequestHeadersAndBody(request, request_->upload_data_stream)) {
     int merged_size = static_cast<int>(
         request_headers_length_ + request_->upload_data_stream->size());
     scoped_refptr<IOBuffer> merged_request_headers_and_body(
         new IOBuffer(merged_size));
     // We'll repurpose |request_headers_| to store the merged headers and
     // body.
     request_headers_ = new DrainableIOBuffer(
         merged_request_headers_and_body.get(), merged_size);
-
     memcpy(request_headers_->data(), request.data(), request_headers_length_);
     request_headers_->DidConsume(request_headers_length_);
 
     uint64_t todo = request_->upload_data_stream->size();
     while (todo) {
-      int consumed = request_->upload_data_stream->Read(
+      int result = request_->upload_data_stream->Read(
           request_headers_.get(), static_cast<int>(todo), CompletionCallback());
-      DCHECK_GT(consumed, 0);  // Read() won't fail if not chunked.
-      request_headers_->DidConsume(consumed);
-      todo -= consumed;
+      if (result == ERR_FILE_READ_FAIL)
+        return result;

[mmenke, 2016/05/24 17:13:47]

I think we can actually just keep the DCHECK here, instead - if it's in memory,
reads can't fail.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+      request_headers_->DidConsume(result);
+      todo -= result;
     }
     DCHECK(request_->upload_data_stream->IsEOF());
     // Reset the offset, so the buffer can be read from the beginning.
     request_headers_->SetOffset(0);
     did_merge = true;
 
     net_log_.AddEvent(
         NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST_BODY,
         base::Bind(&NetLogSendRequestBodyCallback,
                    request_->upload_data_stream->size(),
                    false, /* not chunked */
                    true /* merged */));
   }
 
   if (!did_merge) {
     // If we didn't merge the body with the headers, then |request_headers_|
     // contains just the HTTP headers.
     scoped_refptr<StringIOBuffer> headers_io_buf(new StringIOBuffer(request));
     request_headers_ =
         new DrainableIOBuffer(headers_io_buf.get(), headers_io_buf->size());
   }
-
   result = DoLoop(OK);

[mmenke, 2016/05/24 17:13:47]

Should keep these line break, for readability.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

   if (result == ERR_IO_PENDING)
     callback_ = callback;
-
   return result > 0 ? OK : result;
 }
 
 int HttpStreamParser::ReadResponseHeaders(const CompletionCallback& callback) {
   DCHECK(io_state_ == STATE_NONE || io_state_ == STATE_DONE);
   DCHECK(callback_.is_null());
   DCHECK(!callback.is_null());
   DCHECK_EQ(0, read_buf_unused_offset_);
 
   // This function can be called with io_state_ == STATE_DONE if the
@@ skipping 206 matching lines @@
   sent_bytes_ += result;
   request_body_send_buf_->DidConsume(result);
 
   io_state_ = STATE_SEND_BODY;
   return OK;
 }
 
 int HttpStreamParser::DoSendRequestReadBodyComplete(int result) {
   // |result| is the result of read from the request body from the last call to
   // DoSendBody().
-  DCHECK_GE(result, 0);  // There won't be errors.
+  if (result == ERR_FILE_READ_FAIL)

[mmenke, 2016/05/24 17:13:47]

This should be "if (result < 0)" - if we see any read failure, we should fail,
it doesn't matter which one it is.  Also note that we can't get ERR_IO_PENDING
here.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done. Or should it be DCHECK_NE(result, ERR_IO_PENDING) ?

[mmenke, 2016/05/25 15:13:30]

We already have that DCHECK in HttpStreamParser::DoLoop, before calling this,
and other comparable methods in this file don't, so probably best just to keep
things as-is.  However, that wouldn't be unreasonable here.

+    return result;
 
   // Chunked data needs to be encoded.
   if (request_->upload_data_stream->is_chunked()) {
     if (result == 0) {  // Reached the end.
       DCHECK(request_->upload_data_stream->IsEOF());
       sent_last_chunk_ = true;
     }
     // Encode the buffer as 1 chunk.
     const base::StringPiece payload(request_body_read_buf_->data(), result);
     request_body_send_buf_->Clear();
@@ skipping 597 matching lines @@
 }
 
 void HttpStreamParser::ValidateStatusLine(const std::string& status_line) {
   HttpStatusLineValidator::StatusLineStatus status =
       HttpStatusLineValidator::ValidateStatusLine(status_line);
   UMA_HISTOGRAM_ENUMERATION("Net.HttpStatusLineStatus", status,
                             HttpStatusLineValidator::STATUS_LINE_MAX);
 }
 
 }  // namespace net