Reland: URLRequestJob: change ReadRawData contract

This CL is to reland crrev.com/1410643007 which was
reverted in crrev.com/1437523002.

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.

TBR=michaeln@chromium.org,mtomasz@chromium.org,jianli@chromium.org,zork@chromium.org

BUG=474859
BUG=329902

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

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

2 files changed, 31 insertions, 39 deletions

diff --git a/android_webview/browser/net/android_stream_reader_url_request_job.cc b/android_webview/browser/net/android_stream_reader_url_request_job.cc
index 173bdbb..dc288fa 100644
--- a/android_webview/browser/net/android_stream_reader_url_request_job.cc
+++ b/android_webview/browser/net/android_stream_reader_url_request_job.cc
@@ -21,7 +21,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mime_util.h"
-#include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
@@ -90,6 +89,7 @@ AndroidStreamReaderURLRequestJob::AndroidStreamReaderURLRequestJob(
     net::NetworkDelegate* network_delegate,
     scoped_ptr<Delegate> delegate)
     : URLRequestJob(request, network_delegate),
+      range_parse_result_(net::OK),
       delegate_(delegate.Pass()),
       weak_factory_(this) {
   DCHECK(delegate_);
@@ -101,6 +101,7 @@ AndroidStreamReaderURLRequestJob::AndroidStreamReaderURLRequestJob(
     scoped_ptr<DelegateObtainer> delegate_obtainer,
     bool)
     : URLRequestJob(request, network_delegate),
+      range_parse_result_(net::OK),
       delegate_obtainer_(delegate_obtainer.Pass()),
       weak_factory_(this) {
   DCHECK(delegate_obtainer_);
@@ -141,7 +142,10 @@ void AndroidStreamReaderURLRequestJob::Start() {
         base::Bind(&AndroidStreamReaderURLRequestJob::DelegateObtained,
                    weak_factory_.GetWeakPtr()));
   } else {
-    DoStart();
+    // Run DoStart asynchronously to avoid re-entering the delegate.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&AndroidStreamReaderURLRequestJob::DoStart,
+                              weak_factory_.GetWeakPtr()));
   }
 }
 
@@ -202,45 +206,29 @@ void AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted(int result) {
     set_expected_content_size(result);
     HeadersComplete(kHTTPOk, kHTTPOkText);
   } else {
-    NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
+    NotifyStartError(
+        net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
   }
 }
 
 void AndroidStreamReaderURLRequestJob::OnReaderReadCompleted(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  // The URLRequest API contract requires that:
-  // * NotifyDone be called once, to set the status code, indicate the job is
-  //   finished (there will be no further IO),
-  // * NotifyReadComplete be called if false is returned from ReadRawData to
-  //   indicate that the IOBuffer will not be used by the job anymore.
-  // There might be multiple calls to ReadRawData (and thus multiple calls to
-  // NotifyReadComplete), which is why NotifyDone is called only on errors
-  // (result < 0) and end of data (result == 0).
-  if (result == 0) {
-    NotifyDone(net::URLRequestStatus());
-  } else if (result < 0) {
-    NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
-  } else {
-    // Clear the IO_PENDING status.
-    SetStatus(net::URLRequestStatus());
-  }
-  NotifyReadComplete(result);
+
+  ReadRawDataComplete(result);
 }
 
 base::TaskRunner* AndroidStreamReaderURLRequestJob::GetWorkerThreadRunner() {
   return static_cast<base::TaskRunner*>(BrowserThread::GetBlockingPool());
 }
 
-bool AndroidStreamReaderURLRequestJob::ReadRawData(net::IOBuffer* dest,
-                                                   int dest_size,
-                                                   int* bytes_read) {
+int AndroidStreamReaderURLRequestJob::ReadRawData(net::IOBuffer* dest,
+                                                  int dest_size) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!input_stream_reader_wrapper_.get()) {
     // This will happen if opening the InputStream fails in which case the
     // error is communicated by setting the HTTP response status header rather
     // than failing the request during the header fetch phase.
-    *bytes_read = 0;
-    return true;
+    return 0;
   }
 
   PostTaskAndReplyWithResult(
@@ -251,9 +239,7 @@ bool AndroidStreamReaderURLRequestJob::ReadRawData(net::IOBuffer* dest,
       base::Bind(&AndroidStreamReaderURLRequestJob::OnReaderReadCompleted,
                  weak_factory_.GetWeakPtr()));
 
-  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING,
-                                  net::ERR_IO_PENDING));
-  return false;
+  return net::ERR_IO_PENDING;
 }
 
 bool AndroidStreamReaderURLRequestJob::GetMimeType(
@@ -303,6 +289,11 @@ void AndroidStreamReaderURLRequestJob::DelegateObtained(
 
 void AndroidStreamReaderURLRequestJob::DoStart() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (range_parse_result_ != net::OK) {
+    NotifyStartError(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                           range_parse_result_));
+    return;
+  }
   // Start reading asynchronously so that all error reporting and data
   // callbacks happen as they would for network requests.
   SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING,
@@ -383,19 +374,18 @@ void AndroidStreamReaderURLRequestJob::SetExtraRequestHeaders(
     const net::HttpRequestHeaders& headers) {
   std::string range_header;
   if (headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header)) {
-    // We only extract the "Range" header so that we know how many bytes in the
-    // stream to skip and how many to read after that.
+    // This job only cares about the Range header so that we know how many bytes
+    // in the stream to skip and how many to read after that. Note that
+    // validation is deferred to DoStart(), because NotifyStartError() is not
+    // legal to call since the job has not started.
     std::vector<net::HttpByteRange> ranges;
     if (net::HttpUtil::ParseRangeHeader(range_header, &ranges)) {
-      if (ranges.size() == 1) {
+      if (ranges.size() == 1)
         byte_range_ = ranges[0];
-      } else {
-        // We don't support multiple range requests in one single URL request,
-        // because we need to do multipart encoding here.
-        NotifyDone(
-            net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                  net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
-      }
+    } else {
+      // We don't support multiple range requests in one single URL request,
+      // because we need to do multipart encoding here.
+      range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
     }
   }
 }
diff --git a/android_webview/browser/net/android_stream_reader_url_request_job.h b/android_webview/browser/net/android_stream_reader_url_request_job.h
index e21d765..645be13 100644
--- a/android_webview/browser/net/android_stream_reader_url_request_job.h
+++ b/android_webview/browser/net/android_stream_reader_url_request_job.h
@@ -15,6 +15,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "net/base/net_errors.h"
 #include "net/http/http_byte_range.h"
 #include "net/url_request/url_request_job.h"
 
@@ -96,7 +97,7 @@ class AndroidStreamReaderURLRequestJob : public net::URLRequestJob {
   // URLRequestJob:
   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;
   void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers) override;
   bool GetMimeType(std::string* mime_type) const override;
   bool GetCharset(std::string* charset) override;
@@ -131,6 +132,7 @@ class AndroidStreamReaderURLRequestJob : public net::URLRequestJob {
   void OnReaderReadCompleted(int bytes_read);
 
   net::HttpByteRange byte_range_;
+  net::Error range_parse_result_;
   scoped_ptr<net::HttpResponseInfo> response_info_;
   scoped_ptr<Delegate> delegate_;
   scoped_ptr<DelegateObtainer> delegate_obtainer_;