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();

[mmenke, 2015/10/22 18:58:10]

Randy:  Did we ever figure out why this isn't called (Here or in
ReadRawDataComplete) when there's no filter?

[Randy Smith (Not in Mondays), 2015/10/22 20:38:45]

So the comment in url_request_job.h says:

  // Called to tell the job that a filter has successfully reached the end of
  // the stream.

So this usage is in accordance with that comment.  Having said that,
URLRequestHttpJob uses it to decide when to call transaction_->DoneReading(),
which seems like something that you want to do whether or not you have a filter
chain.  I think URLRequestHttpJob's use of it is the only use of it, so if we
don't need it (which seems likely--I presume URLRHJ is implicitly relying on
destruction for tearing down the transaction in most cases), we should probably
just remove it.  WDYT?

[mmenke, 2015/10/22 20:42:46]

I thought transaction_->DoneReading() was used to tell the cache that the entire
response was successfully received?  Maybe this is so that we successfully cache
responses in the case content-length is actually the uncompressed content
length?  IF so, I'm fine with not caching those responses.  I don't think that
would affect caching under any other circumstances.  Should probably talk to
Gavin before removing this, but if we can reasonably remove it, I'd like to do
so.  This state machine has too must stuff.  :(

[Randy Smith (Not in Mondays), 2015/10/26 21:38:03]

So the comment in HttpTransaction suggests why we can get away with not calling
it in the filter case:

  // Called to tell the transaction that we have successfully reached the end
  // of the stream. This is equivalent to performing an extra Read() at the end
  // that should return 0 bytes. 

My presumption is that if we don't have a filter, we always do the final read,
since that's how we figure out that we're at EOF.  So that means we do need to
keep this in the filter case (IIUC).  I wouldn't at all mind making it happen
for both filter and non-filter cases though, for consistency so that some future
reviewers don't have to have this conversation again.

   }
 
-  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

[mmenke, 2015/10/22 18:58:10]

nit:  +.

[xunjieli, 2015/10/23 13:43:08]

Done.

+    PushInputToFilter(bytes_read);
+
+    // Filter the data.
+    error = ReadFilteredData(&filter_bytes_read);
+    if (error == OK) {
+      if (!filter_bytes_read)
+        DoneReading();
+      notify = true;

[mmenke, 2015/10/22 18:58:11]

Keep comment from before?  ("Don't notify if we had an error.")

[xunjieli, 2015/10/23 13:43:08]

Done.

+    }
+    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 = true;
+    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) {
+    if (bytes_read == 0)
+      NotifyDone(URLRequestStatus());
+    else
+      SetStatus(URLRequestStatus());
+  } else {
+    NotifyDone(URLRequestStatus::FromError(error));
+  }

[mmenke, 2015/10/22 18:58:10]

Maybe:

if (error == OK && bytes_read > 0) {
  SetStatus(URLRequetsStatus());
} else {
  NotifyDone(URLRequestStatus::FromError(error));
}

[xunjieli, 2015/10/23 13:43:08]

Done.

 
-    // 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?
+  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) {
+Error URLRequestJob::ReadRawDataHelper(IOBuffer* buf,
+                                       int buf_size,
+                                       int* bytes_read) {
   DCHECK(!request_->status().is_io_pending());
-  DCHECK(raw_read_buffer_.get() == NULL);
+  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());
+void URLRequestJob::GatherRawReadStats(Error error, int bytes_read) {
+  DCHECK(raw_read_buffer_ || bytes_read == 0);
+  DCHECK_NE(ERR_IO_PENDING, error);
+  if (error != OK)
+    return;

[Randy Smith (Not in Mondays), 2015/10/22 20:38:45]

It looks to me (you should verify) like this will result in a leak of the
reference taken in ReadRawDataHelper above where we assign buf to
raw_read_buffer_; in the old code, OnRawReadComplete was always called on
synchronous return, and the entire function traversed (resulting in the rrb_ =
NULL always being executed) but now that only happens when there isn't an error.
 This isn't a big deal (the reference will be released when URLRequestJob is
destroyed) but it's a minor glitch.

[xunjieli, 2015/10/23 13:43:08]

Done. I believe you are right.

[Randy Smith (Not in Mondays), 2015/10/26 21:38:03]

nit, suggestion: I think the code would be a bit clearer (even if one line
longer) with the original "return on error/main code not indented" structure + a
duplication of the "read_raw_buffer_ = NULL;" line.  Up to you, though.

[xunjieli, 2015/10/27 14:17:21]

Done.

   // 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());
   }
 
   if (bytes_read > 0) {
     RecordBytesRead(bytes_read);
@@ skipping 103 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