URLRequestJob: change ReadRawData contract

This CL is patched from ellyjones@ CL at crrev.com/1227893004.

Previously, the interface for URLRequestJob::ReadRawData was as follows:

  bool ReadRawData(IOBuffer*, int, int*)

Subclasses were expected to signal completion of the ReadRawData call by
calling NotifyDone, SetStatus, or maybe one of the other Notify* functions
on URLRequestJob, most of which do internal housekeeping and also drive the
URLRequest's state machine. This made it difficult to reason about the
URLRequestJob's state machine and needlessly complicated most of URLRequestJob.

The new interface is as follows:

  int ReadRawData(IOBuffer*, int)

Subclasses are required to either:

a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes in any way, or
b) Return a count of bytes read >= 0, indicating synchronous success, or
c) Return another error code < 0, indicating synchronous failure.

This substantially narrows the interface between URLRequestJob and its
subclasses and moves the logic for the URLRequest state machine largely into
URLRequestJob.

Also, the signature of URLRequestJob::ReadFilteredData and some other internal
URLRequestJob helpers changes to propagate detailed error codes instead of
coercing all errors to FAILED.

BUG=474859
BUG=329902

Review URL: https://codereview.chromium.org/1410643007

Cr-Commit-Position: refs/heads/master@{#358327}

6 files changed, 58 insertions, 83 deletions

diff --git a/storage/browser/blob/blob_url_request_job.cc b/storage/browser/blob/blob_url_request_job.cc
index 6ec2fe5..deea300 100644
--- a/storage/browser/blob/blob_url_request_job.cc
+++ b/storage/browser/blob/blob_url_request_job.cc
@@ -75,41 +75,37 @@ void BlobURLRequestJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-bool BlobURLRequestJob::ReadRawData(net::IOBuffer* dest,
-                                    int dest_size,
-                                    int* bytes_read) {
+int BlobURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) {
   TRACE_EVENT_ASYNC_BEGIN1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                            blob_handle_ ? blob_handle_->uuid() : "NotFound");
   DCHECK_NE(dest_size, 0);
-  DCHECK(bytes_read);
 
-  // Bail out immediately if we encounter an error.
-  if (error_) {
-    *bytes_read = 0;
-    return true;
-  }
+  // Bail out immediately if we encounter an error. This happens if a previous
+  // ReadRawData signalled an error to its caller but the caller called
+  // ReadRawData again anyway.
+  if (error_)
+    return 0;
 
+  int bytes_read = 0;
   BlobReader::Status read_status =
-      blob_reader_->Read(dest, dest_size, bytes_read,
+      blob_reader_->Read(dest, dest_size, &bytes_read,
                          base::Bind(&BlobURLRequestJob::DidReadRawData,
                                     weak_factory_.GetWeakPtr()));
 
   switch (read_status) {
     case BlobReader::Status::NET_ERROR:
-      NotifyFailure(blob_reader_->net_error());
       TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                              blob_handle_ ? blob_handle_->uuid() : "NotFound");
-      return false;
+      return blob_reader_->net_error();
     case BlobReader::Status::IO_PENDING:
-      SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
-      return false;
+      return net::ERR_IO_PENDING;
     case BlobReader::Status::DONE:
       TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                              blob_handle_ ? blob_handle_->uuid() : "NotFound");
-      return true;
+      return bytes_read;
   }
   NOTREACHED();
-  return true;
+  return 0;
 }
 
 bool BlobURLRequestJob::GetMimeType(std::string* mime_type) const {
@@ -222,13 +218,7 @@ void BlobURLRequestJob::DidCalculateSize(int result) {
 void BlobURLRequestJob::DidReadRawData(int result) {
   TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                          blob_handle_ ? blob_handle_->uuid() : "NotFound");
-  if (result < 0) {
-    NotifyFailure(result);
-    return;
-  }
-  // Clear the IO_PENDING status
-  SetStatus(net::URLRequestStatus());
-  NotifyReadComplete(result);
+  ReadRawDataComplete(result);
 }
 
 void BlobURLRequestJob::NotifyFailure(int error_code) {
@@ -236,11 +226,7 @@ void BlobURLRequestJob::NotifyFailure(int error_code) {
 
   // If we already return the headers on success, we can't change the headers
   // now. Instead, we just error out.
-  if (response_info_) {
-    NotifyDone(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, error_code));
-    return;
-  }
+  DCHECK(!response_info_) << "Cannot NotifyFailure after headers.";
 
   net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR;
   switch (error_code) {
diff --git a/storage/browser/blob/blob_url_request_job.h b/storage/browser/blob/blob_url_request_job.h
index 21baa2c..1f0b9fb 100644
--- a/storage/browser/blob/blob_url_request_job.h
+++ b/storage/browser/blob/blob_url_request_job.h
@@ -44,7 +44,7 @@ class STORAGE_EXPORT BlobURLRequestJob
   // net::URLRequestJob methods.
   void Start() override;
   void Kill() override;
-  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
+  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
   bool GetMimeType(std::string* mime_type) const override;
   void GetResponseInfo(net::HttpResponseInfo* info) override;
   int GetResponseCode() const override;
diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.cc b/storage/browser/fileapi/file_system_dir_url_request_job.cc
index 35b6c1d..2a0984b7 100644
--- a/storage/browser/fileapi/file_system_dir_url_request_job.cc
+++ b/storage/browser/fileapi/file_system_dir_url_request_job.cc
@@ -14,7 +14,6 @@
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/io_buffer.h"
-#include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/url_request/url_request.h"
 #include "storage/browser/fileapi/file_system_context.h"
@@ -44,15 +43,14 @@ FileSystemDirURLRequestJob::FileSystemDirURLRequestJob(
 FileSystemDirURLRequestJob::~FileSystemDirURLRequestJob() {
 }
 
-bool FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size,
-                                             int *bytes_read) {
-  int count = std::min(dest_size, static_cast<int>(data_.size()));
+int FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest,
+                                            int dest_size) {
+  int count = std::min(dest_size, base::checked_cast<int>(data_.size()));
   if (count > 0) {
     memcpy(dest->data(), data_.data(), count);
     data_.erase(0, count);
   }
-  *bytes_read = count;
-  return true;
+  return count;
 }
 
 void FileSystemDirURLRequestJob::Start() {
@@ -98,8 +96,7 @@ void FileSystemDirURLRequestJob::StartAsync() {
                        false);
       return;
     }
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                net::ERR_FILE_NOT_FOUND));
+    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
     return;
   }
   file_system_context_->operation_runner()->ReadDirectory(
@@ -112,8 +109,7 @@ void FileSystemDirURLRequestJob::DidAttemptAutoMount(base::File::Error result) {
       file_system_context_->CrackURL(request_->url()).is_valid()) {
     StartAsync();
   } else {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                net::ERR_FILE_NOT_FOUND));
+    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
   }
 }
 
@@ -125,7 +121,7 @@ void FileSystemDirURLRequestJob::DidReadDirectory(
     int rv = net::ERR_FILE_NOT_FOUND;
     if (result == base::File::FILE_ERROR_INVALID_URL)
       rv = net::ERR_INVALID_URL;
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
+    NotifyStartError(URLRequestStatus::FromError(rv));
     return;
   }
 
diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.h b/storage/browser/fileapi/file_system_dir_url_request_job.h
index 8715c96..56b13ff 100644
--- a/storage/browser/fileapi/file_system_dir_url_request_job.h
+++ b/storage/browser/fileapi/file_system_dir_url_request_job.h
@@ -33,7 +33,7 @@ class STORAGE_EXPORT_PRIVATE FileSystemDirURLRequestJob
   // URLRequestJob methods:
   void Start() override;
   void Kill() override;
-  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
+  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
   bool GetCharset(std::string* charset) override;
 
   // FilterContext methods (via URLRequestJob):
diff --git a/storage/browser/fileapi/file_system_url_request_job.cc b/storage/browser/fileapi/file_system_url_request_job.cc
index 9572721..6c33fc8 100644
--- a/storage/browser/fileapi/file_system_url_request_job.cc
+++ b/storage/browser/fileapi/file_system_url_request_job.cc
@@ -62,8 +62,8 @@ FileSystemURLRequestJob::FileSystemURLRequestJob(
       file_system_context_(file_system_context),
       is_directory_(false),
       remaining_bytes_(0),
-      weak_factory_(this) {
-}
+      range_parse_result_(net::OK),
+      weak_factory_(this) {}
 
 FileSystemURLRequestJob::~FileSystemURLRequestJob() {}
 
@@ -80,39 +80,28 @@ void FileSystemURLRequestJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest,
-                                          int dest_size,
-                                          int* bytes_read) {
+int FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) {
   DCHECK_NE(dest_size, 0);
-  DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
   if (reader_.get() == NULL)
-    return false;
+    return net::ERR_FAILED;
 
   if (remaining_bytes_ < dest_size)
-    dest_size = static_cast<int>(remaining_bytes_);
+    dest_size = remaining_bytes_;
 
-  if (!dest_size) {
-    *bytes_read = 0;
-    return true;
-  }
+  if (!dest_size)
+    return 0;
 
   const int rv = reader_->Read(dest, dest_size,
                                base::Bind(&FileSystemURLRequestJob::DidRead,
                                           weak_factory_.GetWeakPtr()));
   if (rv >= 0) {
-    // Data is immediately available.
-    *bytes_read = rv;
     remaining_bytes_ -= rv;
     DCHECK_GE(remaining_bytes_, 0);
-    return true;
   }
-  if (rv == net::ERR_IO_PENDING)
-    SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
-  else
-    NotifyFailed(rv);
-  return false;
+
+  return rv;
 }
 
 bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const {
@@ -127,8 +116,12 @@ bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const {
 void FileSystemURLRequestJob::SetExtraRequestHeaders(
     const net::HttpRequestHeaders& headers) {
   std::string range_header;
+  // Currently this job only cares about the Range header. Note that validation
+  // is deferred to DidGetMetaData(), because NotifyStartError is not legal to
+  // call since the job has not started.
   if (headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header)) {
     std::vector<net::HttpByteRange> ranges;
+
     if (net::HttpUtil::ParseRangeHeader(range_header, &ranges)) {
       if (ranges.size() == 1) {
         byte_range_ = ranges[0];
@@ -136,7 +129,7 @@ void FileSystemURLRequestJob::SetExtraRequestHeaders(
         // We don't support multiple range requests in one single URL request.
         // TODO(adamk): decide whether we want to support multiple range
         // requests.
-        NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
+        range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
       }
     }
   }
@@ -168,7 +161,7 @@ void FileSystemURLRequestJob::StartAsync() {
   }
   if (!file_system_context_->CanServeURLRequest(url_)) {
     // In incognito mode the API is not usable and there should be no data.
-    NotifyFailed(net::ERR_FILE_NOT_FOUND);
+    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
     return;
   }
   file_system_context_->operation_runner()->GetMetadata(
@@ -182,7 +175,7 @@ void FileSystemURLRequestJob::DidAttemptAutoMount(base::File::Error result) {
       file_system_context_->CrackURL(request_->url()).is_valid()) {
     StartAsync();
   } else {
-    NotifyFailed(net::ERR_FILE_NOT_FOUND);
+    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
   }
 }
 
@@ -190,8 +183,10 @@ void FileSystemURLRequestJob::DidGetMetadata(
     base::File::Error error_code,
     const base::File::Info& file_info) {
   if (error_code != base::File::FILE_OK) {
-    NotifyFailed(error_code == base::File::FILE_ERROR_INVALID_URL ?
-                 net::ERR_INVALID_URL : net::ERR_FILE_NOT_FOUND);
+    NotifyStartError(URLRequestStatus::FromError(
+        error_code == base::File::FILE_ERROR_INVALID_URL
+            ? net::ERR_INVALID_URL
+            : net::ERR_FILE_NOT_FOUND));
     return;
   }
 
@@ -201,8 +196,14 @@ void FileSystemURLRequestJob::DidGetMetadata(
 
   is_directory_ = file_info.is_directory;
 
+  if (range_parse_result_ != net::OK) {
+    NotifyStartError(URLRequestStatus::FromError(range_parse_result_));
+    return;
+  }
+
   if (!byte_range_.ComputeBounds(file_info.size)) {
-    NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
+    NotifyStartError(
+        URLRequestStatus::FromError(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
@@ -226,17 +227,12 @@ void FileSystemURLRequestJob::DidGetMetadata(
 }
 
 void FileSystemURLRequestJob::DidRead(int result) {
-  if (result > 0)
-    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
-  else if (result == 0)
-    NotifyDone(URLRequestStatus());
-  else
-    NotifyFailed(result);
-
-  remaining_bytes_ -= result;
-  DCHECK_GE(remaining_bytes_, 0);
+  if (result >= 0) {
+    remaining_bytes_ -= result;
+    DCHECK_GE(remaining_bytes_, 0);
+  }
 
-  NotifyReadComplete(result);
+  ReadRawDataComplete(result);
 }
 
 bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location,
@@ -256,8 +252,4 @@ bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location,
   return false;
 }
 
-void FileSystemURLRequestJob::NotifyFailed(int rv) {
-  NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
-}
-
 }  // namespace storage
diff --git a/storage/browser/fileapi/file_system_url_request_job.h b/storage/browser/fileapi/file_system_url_request_job.h
index 0021210..09271f5 100644
--- a/storage/browser/fileapi/file_system_url_request_job.h
+++ b/storage/browser/fileapi/file_system_url_request_job.h
@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "net/base/net_errors.h"
 #include "net/http/http_byte_range.h"
 #include "net/url_request/url_request_job.h"
 #include "storage/browser/fileapi/file_system_url.h"
@@ -42,7 +43,7 @@ class STORAGE_EXPORT_PRIVATE FileSystemURLRequestJob
   // URLRequestJob methods:
   void Start() override;
   void Kill() override;
-  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
+  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
   bool IsRedirectResponse(GURL* location, int* http_status_code) override;
   void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers) override;
   void GetResponseInfo(net::HttpResponseInfo* info) override;
@@ -61,7 +62,6 @@ class STORAGE_EXPORT_PRIVATE FileSystemURLRequestJob
   void DidGetMetadata(base::File::Error error_code,
                       const base::File::Info& file_info);
   void DidRead(int result);
-  void NotifyFailed(int rv);
 
   const std::string storage_domain_;
   FileSystemContext* file_system_context_;
@@ -70,6 +70,7 @@ class STORAGE_EXPORT_PRIVATE FileSystemURLRequestJob
   bool is_directory_;
   scoped_ptr<net::HttpResponseInfo> response_info_;
   int64 remaining_bytes_;
+  net::Error range_parse_result_;
   net::HttpByteRange byte_range_;
   base::WeakPtrFactory<FileSystemURLRequestJob> weak_factory_;