Reland: URLRequestJob: change ReadRawData contract

net/url_request/url_request_job.cc

Index: net/url_request/url_request_job.cc
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index 2747f220a8724353c4461038b34687a217666da9..82499a999aef2ae84a11ec995770c57793e53437 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -101,45 +101,46 @@ void URLRequestJob::DetachRequest() {
   request_ = NULL;
 }
 
-// This function calls ReadData to get stream data. If a filter exists, passes
-// the data to the attached filter. Then returns the output from filter back to
-// the caller.
+// This function calls ReadRawData to get stream data. If a filter exists, it
+// passes the data to the attached filter. It then returns the output from
+// filter back to the caller.
 bool URLRequestJob::Read(IOBuffer* buf, int buf_size, int *bytes_read) {
-  bool rv = false;
-
   DCHECK_LT(buf_size, 1000000);  // Sanity check.
   DCHECK(buf);
   DCHECK(bytes_read);
   DCHECK(filtered_read_buffer_.get() == NULL);
   DCHECK_EQ(0, filtered_read_buffer_len_);
 
+  Error error = OK;
   *bytes_read = 0;
 
   // Skip Filter if not present.
-  if (!filter_.get()) {
-    rv = ReadRawDataHelper(buf, buf_size, bytes_read);
+  if (!filter_) {
+    error = ReadRawDataHelper(buf, buf_size, bytes_read);
   } else {
     // Save the caller's buffers while we do IO
     // in the filter's buffers.
     filtered_read_buffer_ = buf;
     filtered_read_buffer_len_ = buf_size;
 
-    if (ReadFilteredData(bytes_read)) {
-      rv = true;  // We have data to return.
+    error = ReadFilteredData(bytes_read);
 
-      // It is fine to call DoneReading even if ReadFilteredData receives 0
-      // bytes from the net, but we avoid making that call if we know for
-      // sure that's the case (ReadRawDataHelper path).
-      if (*bytes_read == 0)
-        DoneReading();
-    } else {
-      rv = false;  // Error, or a new IO is pending.
-    }
+    // Synchronous EOF from the filter.
+    if (error == OK && *bytes_read == 0)
+      DoneReading();
   }
 
-  if (rv && *bytes_read == 0)
-    NotifyDone(URLRequestStatus());
-  return rv;
+  if (error == OK) {
+    // If URLRequestJob read zero bytes, the job is at EOF.
+    if (*bytes_read == 0)
+      NotifyDone(URLRequestStatus());
+  } else if (error == ERR_IO_PENDING) {
+    SetStatus(URLRequestStatus::FromError(ERR_IO_PENDING));
+  } else {
+    NotifyDone(URLRequestStatus::FromError(error));
+    *bytes_read = -1;
+  }
+  return error == OK;
 }
 
 void URLRequestJob::StopCaching() {
@@ -480,11 +481,21 @@ void URLRequestJob::NotifyHeadersComplete() {
   request_->NotifyResponseStarted();
 }
 
-void URLRequestJob::NotifyReadComplete(int bytes_read) {
+void URLRequestJob::ConvertResultToError(int result, Error* error, int* count) {
+  if (result >= 0) {
+    *error = OK;
+    *count = result;
+  } else {
+    *error = static_cast<Error>(result);
+    *count = 0;
+  }
+}
+
+void URLRequestJob::ReadRawDataComplete(int result) {
   // TODO(cbentzel): Remove ScopedTracker below once crbug.com/475755 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "475755 URLRequestJob::NotifyReadComplete"));
+          "475755 URLRequestJob::RawReadCompleted"));
 
   if (!request_ || !request_->has_delegate())
     return;  // The request was destroyed, so there is no more work to do.
@@ -497,11 +508,52 @@ void URLRequestJob::NotifyReadComplete(int bytes_read) {
   // The headers should be complete before reads complete
   DCHECK(has_handled_response_);
 
-  OnRawReadComplete(bytes_read);
+  Error error;
+  int bytes_read;
+  ConvertResultToError(result, &error, &bytes_read);
 
-  // Don't notify if we had an error.
-  if (!request_->status().is_success())
-    return;
+  DCHECK_NE(ERR_IO_PENDING, error);
+
+  // Synchronize the URLRequest state machine with the URLRequestJob state
+  // machine. If this read succeeded, either the request is at EOF and the
+  // URLRequest state machine goes to 'finished', or it is not and the
+  // URLRequest state machine goes to 'success'. If the read failed, the
+  // URLRequest state machine goes directly to 'finished'.
+  //
+  // Update the URLRequest's status first, so that NotifyReadCompleted has an
+  // accurate view of the request.
+  if (error == OK && bytes_read > 0) {
+    SetStatus(URLRequestStatus());
+  } else {
+    NotifyDone(URLRequestStatus::FromError(error));
+  }
+
+  GatherRawReadStats(error, bytes_read);
+
+  if (filter_.get() && error == OK) {
+    int filter_bytes_read = 0;
+    // Tell the filter that it has more data.
+    PushInputToFilter(bytes_read);
+
+    // Filter the data.
+    error = ReadFilteredData(&filter_bytes_read);
+
+    if (!filter_bytes_read)
+      DoneReading();
+
+    DVLOG(1) << __FUNCTION__ << "() "
+             << "\"" << (request_ ? request_->url().spec() : "???") << "\""
+             << " pre bytes read = " << bytes_read
+             << " pre total = " << prefilter_bytes_read_
+             << " post total = " << postfilter_bytes_read_;
+    bytes_read = filter_bytes_read;
+  } else {
+    DVLOG(1) << __FUNCTION__ << "() "
+             << "\"" << (request_ ? request_->url().spec() : "???") << "\""
+             << " pre bytes read = " << bytes_read
+             << " pre total = " << prefilter_bytes_read_
+             << " post total = " << postfilter_bytes_read_;
+  }
 
   // When notifying the delegate, the delegate can release the request
   // (and thus release 'this').  After calling to the delegate, we must
@@ -510,25 +562,10 @@ void URLRequestJob::NotifyReadComplete(int bytes_read) {
   // survival until we can get out of this method.
   scoped_refptr<URLRequestJob> self_preservation(this);
 
-  if (filter_.get()) {
-    // Tell the filter that it has more data
-    FilteredDataRead(bytes_read);
-
-    // Filter the data.
-    int filter_bytes_read = 0;
-    if (ReadFilteredData(&filter_bytes_read)) {
-      if (!filter_bytes_read)
-        DoneReading();
-      request_->NotifyReadCompleted(filter_bytes_read);
-    }
-  } else {
+  // NotifyReadCompleted should be called after SetStatus or NotifyDone updates
+  // the status.
+  if (error == OK)
     request_->NotifyReadCompleted(bytes_read);
-  }
-  DVLOG(1) << __FUNCTION__ << "() "
-           << "\"" << (request_ ? request_->url().spec() : "???") << "\""
-           << " pre bytes read = " << bytes_read
-           << " pre total = " << prefilter_bytes_read_
-           << " post total = " << postfilter_bytes_read_;
 }
 
 void URLRequestJob::NotifyStartError(const URLRequestStatus &status) {
@@ -555,7 +592,7 @@ void URLRequestJob::NotifyDone(const URLRequestStatus &status) {
   // the response before getting here.
   DCHECK(has_handled_response_ || !status.is_success());
 
-  // As with NotifyReadComplete, we need to take care to notice if we were
+  // As with RawReadCompleted, we need to take care to notice if we were
   // destroyed during a delegate callback.
   if (request_) {
     request_->set_is_pending(false);
@@ -638,10 +675,8 @@ void URLRequestJob::OnCallToDelegateComplete() {
   request_->OnCallToDelegateComplete();
 }
 
-bool URLRequestJob::ReadRawData(IOBuffer* buf, int buf_size, int* bytes_read) {
-  DCHECK(bytes_read);
-  *bytes_read = 0;
-  return true;
+int URLRequestJob::ReadRawData(IOBuffer* buf, int buf_size) {
+  return 0;
 }
 
 void URLRequestJob::DoneReading() {
@@ -651,38 +686,34 @@ void URLRequestJob::DoneReading() {
 void URLRequestJob::DoneReadingRedirectResponse() {
 }
 
-void URLRequestJob::FilteredDataRead(int bytes_read) {
+void URLRequestJob::PushInputToFilter(int bytes_read) {
   DCHECK(filter_);
   filter_->FlushStreamBuffer(bytes_read);
 }
 
-bool URLRequestJob::ReadFilteredData(int* bytes_read) {
+Error URLRequestJob::ReadFilteredData(int* bytes_read) {
   DCHECK(filter_);
   DCHECK(filtered_read_buffer_.get());
   DCHECK_GT(filtered_read_buffer_len_, 0);
   DCHECK_LT(filtered_read_buffer_len_, 1000000);  // Sanity check.
-  DCHECK(!raw_read_buffer_.get());
+  DCHECK(!raw_read_buffer_);
 
   *bytes_read = 0;
-  bool rv = false;
+  Error error = ERR_FAILED;
 
   for (;;) {
     if (is_done())
-      return true;
+      return OK;
 
     if (!filter_needs_more_output_space_ && !filter_->stream_data_len()) {
       // We don't have any raw data to work with, so read from the transaction.
       int filtered_data_read;
-      if (ReadRawDataForFilter(&filtered_data_read)) {
-        if (filtered_data_read > 0) {
-          // Give data to filter.
-          filter_->FlushStreamBuffer(filtered_data_read);
-        } else {
-          return true;  // EOF.
-        }
-      } else {
-        return false;  // IO Pending (or error).
-      }
+      error = ReadRawDataForFilter(&filtered_data_read);
+      // If ReadRawDataForFilter returned some data, fall through to the case
+      // below; otherwise, return early.
+      if (error != OK || filtered_data_read == 0)
+        return error;
+      filter_->FlushStreamBuffer(filtered_data_read);
     }
 
     if ((filter_->stream_data_len() || filter_needs_more_output_space_) &&
@@ -708,7 +739,7 @@ bool URLRequestJob::ReadFilteredData(int* bytes_read) {
           filter_needs_more_output_space_ = false;
           *bytes_read = filtered_data_len;
           postfilter_bytes_read_ += filtered_data_len;
-          rv = true;
+          error = OK;
           break;
         }
         case Filter::FILTER_NEED_MORE_DATA: {
@@ -721,7 +752,7 @@ bool URLRequestJob::ReadFilteredData(int* bytes_read) {
           if (filtered_data_len > 0) {
             *bytes_read = filtered_data_len;
             postfilter_bytes_read_ += filtered_data_len;
-            rv = true;
+            error = OK;
           } else {
             // Read again since we haven't received enough data yet (e.g., we
             // may not have a complete gzip header yet).
@@ -732,7 +763,7 @@ bool URLRequestJob::ReadFilteredData(int* bytes_read) {
         case Filter::FILTER_OK: {
           *bytes_read = filtered_data_len;
           postfilter_bytes_read_ += filtered_data_len;
-          rv = true;
+          error = OK;
           break;
         }
         case Filter::FILTER_ERROR: {
@@ -740,21 +771,19 @@ bool URLRequestJob::ReadFilteredData(int* bytes_read) {
                    << "\"" << (request_ ? request_->url().spec() : "???")
                    << "\"" << " Filter Error";
           filter_needs_more_output_space_ = false;
-          NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                      ERR_CONTENT_DECODING_FAILED));
-          rv = false;
+          error = ERR_CONTENT_DECODING_FAILED;
           break;
         }
         default: {
           NOTREACHED();
           filter_needs_more_output_space_ = false;
-          rv = false;
+          error = ERR_FAILED;
           break;
         }
       }
 
       // If logging all bytes is enabled, log the filtered bytes read.
-      if (rv && request() && filtered_data_len > 0 &&
+      if (error == OK && request() && filtered_data_len > 0 &&
           request()->net_log().IsCapturing()) {
         request()->net_log().AddByteTransferEvent(
             NetLog::TYPE_URL_REQUEST_JOB_FILTERED_BYTES_READ, filtered_data_len,
@@ -762,18 +791,18 @@ bool URLRequestJob::ReadFilteredData(int* bytes_read) {
       }
     } else {
       // we are done, or there is no data left.
-      rv = true;
+      error = OK;
     }
     break;
   }
 
-  if (rv) {
+  if (error == OK) {
     // When we successfully finished a read, we no longer need to save the
     // caller's buffers. Release our reference.
     filtered_read_buffer_ = NULL;
     filtered_read_buffer_len_ = 0;
   }
-  return rv;
+  return error;
 }
 
 void URLRequestJob::DestroyFilters() {
@@ -806,9 +835,8 @@ void URLRequestJob::SetProxyServer(const HostPortPair& proxy_server) {
   request_->proxy_server_ = proxy_server;
 }
 
-bool URLRequestJob::ReadRawDataForFilter(int* bytes_read) {
-  bool rv = false;
-
+Error URLRequestJob::ReadRawDataForFilter(int* bytes_read) {
+  Error error = ERR_FAILED;
   DCHECK(bytes_read);
   DCHECK(filter_.get());
 
@@ -820,30 +848,28 @@ bool URLRequestJob::ReadRawDataForFilter(int* bytes_read) {
   if (!filter_->stream_data_len() && !is_done()) {
     IOBuffer* stream_buffer = filter_->stream_buffer();
     int stream_buffer_size = filter_->stream_buffer_size();
-    rv = ReadRawDataHelper(stream_buffer, stream_buffer_size, bytes_read);
+    error = ReadRawDataHelper(stream_buffer, stream_buffer_size, bytes_read);
   }
-  return rv;
+  return error;
 }
 
-bool URLRequestJob::ReadRawDataHelper(IOBuffer* buf,
-                                      int buf_size,
-                                      int* bytes_read) {
-  DCHECK(!request_->status().is_io_pending());
-  DCHECK(raw_read_buffer_.get() == NULL);
+Error URLRequestJob::ReadRawDataHelper(IOBuffer* buf,
+                                       int buf_size,
+                                       int* bytes_read) {
+  DCHECK(!raw_read_buffer_);
 
-  // Keep a pointer to the read buffer, so we have access to it in the
-  // OnRawReadComplete() callback in the event that the read completes
-  // asynchronously.
+  // Keep a pointer to the read buffer, so we have access to it in
+  // GatherRawReadStats() in the event that the read completes asynchronously.
   raw_read_buffer_ = buf;
-  bool rv = ReadRawData(buf, buf_size, bytes_read);
+  Error error;
+  ConvertResultToError(ReadRawData(buf, buf_size), &error, bytes_read);
 
-  if (!request_->status().is_io_pending()) {
-    // If the read completes synchronously, either success or failure,
-    // invoke the OnRawReadComplete callback so we can account for the
-    // completed read.
-    OnRawReadComplete(*bytes_read);
+  if (error != ERR_IO_PENDING) {
+    // If the read completes synchronously, either success or failure, invoke
+    // GatherRawReadStats so we can account for the completed read.
+    GatherRawReadStats(error, *bytes_read);
   }
-  return rv;
+  return error;
 }
 
 void URLRequestJob::FollowRedirect(const RedirectInfo& redirect_info) {
@@ -852,9 +878,16 @@ void URLRequestJob::FollowRedirect(const RedirectInfo& redirect_info) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
 }
 
-void URLRequestJob::OnRawReadComplete(int bytes_read) {
-  DCHECK(raw_read_buffer_.get());
-  // If |filter_| is non-NULL, bytes will be logged after it is applied instead.
+void URLRequestJob::GatherRawReadStats(Error error, int bytes_read) {
+  DCHECK(raw_read_buffer_ || bytes_read == 0);
+  DCHECK_NE(ERR_IO_PENDING, error);
+
+  if (error != OK) {
+    raw_read_buffer_ = nullptr;
+    return;
+  }
+  // If |filter_| is non-NULL, bytes will be logged after it is applied
+  // instead.
   if (!filter_.get() && request() && bytes_read > 0 &&
       request()->net_log().IsCapturing()) {
     request()->net_log().AddByteTransferEvent(
@@ -865,7 +898,7 @@ void URLRequestJob::OnRawReadComplete(int bytes_read) {
   if (bytes_read > 0) {
     RecordBytesRead(bytes_read);
   }
-  raw_read_buffer_ = NULL;
+  raw_read_buffer_ = nullptr;
 }
 
 void URLRequestJob::RecordBytesRead(int bytes_read) {