URLRequestJob: change ReadRawData contract

net/url_request/url_request_job.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/url_request/url_request_job.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 83 matching lines @@
   // Make sure the request is notified that we are done.  We assume that the
   // request took care of setting its error status before calling Kill.
   if (request_)
     NotifyCanceled();
 }
 
 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() {
   // Nothing to do here.
 }
 
 bool URLRequestJob::GetFullRequestHeaders(HttpRequestHeaders* headers) const {
   // Most job types don't send request headers.
   return false;
 }
@@ skipping 320 matching lines @@
       base::StringToInt64(content_length, &expected_content_size_);
   } else {
     request_->net_log().AddEvent(
         NetLog::TYPE_URL_REQUEST_FILTERS_SET,
         base::Bind(&FiltersSetCallback, base::Unretained(filter_.get())));
   }
 
   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"));
+
+  Error error;
+  int bytes_read;
+  ConvertResultToError(result, &error, &bytes_read);
 
   if (!request_ || !request_->has_delegate())
     return;  // The request was destroyed, so there is no more work to do.
 
   // TODO(darin): Bug 1004233. Re-enable this test once all of the chrome
   // unit_tests have been fixed to not trip this.
 #if 0
   DCHECK(!request_->status().is_io_pending());
 #endif
   // The headers should be complete before reads complete
   DCHECK(has_handled_response_);
 
-  OnRawReadComplete(bytes_read);
+  GatherRawReadStats(error, bytes_read);
 
-  // Don't notify if we had an error.
-  if (!request_->status().is_success())
-    return;
+  bool notify = false;
+  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();
+    // Notify when |error| is OK.
+    notify = true;
+
+    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 {
+    // Notify when |error| is OK.
+    if (error == OK)
+      notify = true;

[mmenke, 2015/10/27 19:41:08]

I think this can be moved out of this if statement? The other branch
unconditionally sets notify to try, but is only run when error starts out as OK,
too.

[xunjieli, 2015/10/27 21:24:07]

Done.

+
+    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
   // check the request pointer to see if it still exists, and return
   // immediately if it has been destroyed.  self_preservation ensures our
   // 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);
+  // 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));
+  }
 
-    // Filter the data.
-    int filter_bytes_read = 0;
-    if (ReadFilteredData(&filter_bytes_read)) {
-      if (!filter_bytes_read)
-        DoneReading();
-      request_->NotifyReadCompleted(filter_bytes_read);
-    }
-  } else {
+  // TODO(ellyjones): why does this method only call NotifyReadComplete when
+  // there isn't a filter error? How do filter errors get notified?

[mmenke, 2015/10/27 19:41:08]

Need to fix this comment:  The issue is that we *do* call it on filter errors,
but not on read errors.  I actually suspect we should call it on no errors (Note
that NotifyDone calls CompleteNotifyDone which calls NotifyReadCompleted with -1
- this looks like a real bug to me, as we're calling NotifyReadCompleted twice)

[xunjieli, 2015/10/27 21:24:07]

If that's the case, can I get rid of |notify| and move
request_->NotifyReadCompleted inside the if statement on line 565?

[mmenke, 2015/10/27 21:30:50]

We can....But I'm paranoid about making more behavior changes than strictly
necessary in an already huge CL.

[xunjieli, 2015/10/27 21:44:22]

Done. Thanks for explaining. I have moved NotifyReadCompleted and conditioned it
on |error| == OK.

+  if (notify)
     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) {
   DCHECK(!has_handled_response_);
   has_handled_response_ = true;
   if (request_) {
     // There may be relevant information in the response info even in the
     // error case.
     GetResponseInfo(&request_->response_info_);
 
     request_->set_status(status);
     request_->NotifyResponseStarted();
     // We may have been deleted.
   }
 }
 
 void URLRequestJob::NotifyDone(const URLRequestStatus &status) {
   DCHECK(!done_) << "Job sending done notification twice";
   if (done_)
     return;
   done_ = true;
 
   // Unless there was an error, we should have at least tried to handle
   // 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);
     // With async IO, it's quite possible to have a few outstanding
     // requests.  We could receive a request to Cancel, followed shortly
     // by a successful IO.  For tracking the status(), once there is
     // an error, we do not change the status back to success.  To
     // enforce this, only set the status if the job is so far
     // successful.
     if (request_->status().is_success()) {
@@ skipping 62 matching lines @@
 }
 
 void URLRequestJob::OnCallToDelegate() {
   request_->OnCallToDelegate();
 }
 
 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() {
   // Do nothing.
 }
 
 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_) &&
         !is_done()) {
       // Get filtered data.
       int filtered_data_len = filtered_read_buffer_len_;
       int output_buffer_size = filtered_data_len;
       Filter::FilterStatus status =
           filter_->ReadData(filtered_read_buffer_->data(), &filtered_data_len);
 
       if (filter_needs_more_output_space_ && !filtered_data_len) {
         // filter_needs_more_output_space_ was mistaken... there are no more
         // bytes and we should have at least tried to fill up the filter's input
         // buffer. Correct the state, and try again.
         filter_needs_more_output_space_ = false;
         continue;
       }
       filter_needs_more_output_space_ =
           (filtered_data_len == output_buffer_size);
 
       switch (status) {
         case Filter::FILTER_DONE: {
           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: {
           // We have finished filtering all data currently in the buffer.
           // There might be some space left in the output buffer. One can
           // consider reading more data from the stream to feed the filter
           // and filling up the output buffer. This leads to more complicated
           // buffer management and data notification mechanisms.
           // We can revisit this issue if there is a real perf need.
           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).
             continue;
           }
           break;
         }
         case Filter::FILTER_OK: {
           *bytes_read = filtered_data_len;
           postfilter_bytes_read_ += filtered_data_len;
-          rv = true;
+          error = OK;
           break;
         }
         case Filter::FILTER_ERROR: {
           DVLOG(1) << __FUNCTION__ << "() "
                    << "\"" << (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,
             filtered_read_buffer_->data());
       }
     } 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() {
   filter_.reset();
 }
 
 const URLRequestStatus URLRequestJob::GetStatus() {
   if (request_)
     return request_->status();
   // If the request is gone, we must be cancelled.
@@ skipping 12 matching lines @@
           request_->status().is_success() ||
           (!status.is_success() && !status.is_io_pending()));
     request_->set_status(status);
   }
 }
 
 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());
 
   *bytes_read = 0;
 
   // Get more pre-filtered data if needed.
   // TODO(mbelshe): is it possible that the filter needs *MORE* data
   //    when there is some data already in the buffer?
   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) {
   int rv = request_->Redirect(redirect_info);
   if (rv != OK)
     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(
-        NetLog::TYPE_URL_REQUEST_JOB_BYTES_READ,
-        bytes_read, raw_read_buffer_->data());
+        NetLog::TYPE_URL_REQUEST_JOB_BYTES_READ, bytes_read,
+        raw_read_buffer_->data());
   }
 
   if (bytes_read > 0) {
     RecordBytesRead(bytes_read);
   }
-  raw_read_buffer_ = NULL;
+  raw_read_buffer_ = nullptr;
 }
 
 void URLRequestJob::RecordBytesRead(int bytes_read) {
   DCHECK_GT(bytes_read, 0);
   prefilter_bytes_read_ += bytes_read;
 
   // On first read, notify NetworkQualityEstimator that response headers have
   // been received.
   // TODO(tbansal): Move this to url_request_http_job.cc. This may catch
   // Service Worker jobs twice.
@@ skipping 91 matching lines @@
   int64_t total_sent_bytes = GetTotalSentBytes();
   DCHECK_GE(total_sent_bytes, last_notified_total_sent_bytes_);
   if (total_sent_bytes > last_notified_total_sent_bytes_) {
     network_delegate_->NotifyNetworkBytesSent(
         *request_, total_sent_bytes - last_notified_total_sent_bytes_);
   }
   last_notified_total_sent_bytes_ = total_sent_bytes;
 }
 
 }  // namespace net