URLRequestJob: change ReadRawData contract

net/url_request/url_request_file_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.
 
 // For loading files, we make use of overlapped i/o to ensure that reading from
 // the filesystem (e.g., a network filesystem) does not block the calling
 // thread.  An alternative approach would be to use a background thread or pool
 // of threads, but it seems better to leverage the operating system's ability
 // to do background file reads for us.
 //
@@ skipping 45 matching lines @@
 URLRequestFileJob::URLRequestFileJob(
     URLRequest* request,
     NetworkDelegate* network_delegate,
     const base::FilePath& file_path,
     const scoped_refptr<base::TaskRunner>& file_task_runner)
     : URLRequestJob(request, network_delegate),
       file_path_(file_path),
       stream_(new FileStream(file_task_runner)),
       file_task_runner_(file_task_runner),
       remaining_bytes_(0),
+      range_parse_result_(net::OK),
       weak_ptr_factory_(this) {}
 
 void URLRequestFileJob::Start() {
   FileMetaInfo* meta_info = new FileMetaInfo();
   file_task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::Bind(&URLRequestFileJob::FetchMetaInfo, file_path_,
                  base::Unretained(meta_info)),
       base::Bind(&URLRequestFileJob::DidFetchMetaInfo,
                  weak_ptr_factory_.GetWeakPtr(),
                  base::Owned(meta_info)));
 }
 
 void URLRequestFileJob::Kill() {
   stream_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   URLRequestJob::Kill();
 }
 
-bool URLRequestFileJob::ReadRawData(IOBuffer* dest,
-                                    int dest_size,
-                                    int* bytes_read) {
+int URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size) {
   DCHECK_NE(dest_size, 0);
-  DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
   if (remaining_bytes_ < dest_size)
-    dest_size = static_cast<int>(remaining_bytes_);
+    dest_size = remaining_bytes_;
 
   // If we should copy zero bytes because |remaining_bytes_| is zero, short
   // circuit here.
-  if (!dest_size) {
-    *bytes_read = 0;
-    return true;
-  }
+  if (!dest_size)
+    return 0;
 
   int rv = stream_->Read(dest,
                          dest_size,
                          base::Bind(&URLRequestFileJob::DidRead,
                                     weak_ptr_factory_.GetWeakPtr(),
                                     make_scoped_refptr(dest)));
   if (rv >= 0) {
-    // Data is immediately available.
-    *bytes_read = rv;
     remaining_bytes_ -= rv;
     DCHECK_GE(remaining_bytes_, 0);
-    return true;
   }
 
-  // Otherwise, a read error occured.  We may just need to wait...
-  if (rv == ERR_IO_PENDING) {
-    SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
-  } else {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
-  }
-  return false;
+  return rv;
 }
 
 bool URLRequestFileJob::IsRedirectResponse(GURL* location,
                                            int* http_status_code) {
   if (meta_info_.is_directory) {
     // This happens when we discovered the file is a directory, so needs a
     // slash at the end of the path.
     std::string new_path = request_->url().path();
     new_path.push_back('/');
     GURL::Replacements replacements;
@@ skipping 35 matching lines @@
 
 bool URLRequestFileJob::GetMimeType(std::string* mime_type) const {
   DCHECK(request_);
   if (meta_info_.mime_type_result) {
     *mime_type = meta_info_.mime_type;
     return true;
   }
   return false;
 }
 
+// Extracts headers that this job cares about from the supplied request headers.
+// Currently this job only cares about the Range header. Note that validation is
+// deferred to DidOpen, because this method may be called with a
+// URLRequest::Delegate call still on the stack, which means NotifyStartError is
+// not safe to call (it may reenter the delegate).

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

This comment is useful, but there are several other jobs in the same position
(store range error in SetExtraRequestHeaders, report it later) which could use
the same comment; would you be willing to copy it to those other jobs?

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

Done.

I have a question though. Since SetExtraRequestHeaders is called before Start.
Why do we have a URLRequest::Delegate call on the stack?
My understanding is that NotifyStartError should not be called here, because the
request has not started.

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

Hmmm.  Good point; I think you're right.  Specifically, it seems really wrong
that URLRequest::NotifyResponseStarted() can be called before
URLRequest::Start() is called.  I'd suggest checking with Elly (in case there's
something I'm missing) and then changing the comment over to indicating that
NotifyStartError() isn't legal to call before Start() has been called (might be
worthwhile making that explicit in url_request_job.h as well).

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

Done. I've checked with Elly too.

 void URLRequestFileJob::SetExtraRequestHeaders(
     const HttpRequestHeaders& headers) {
   std::string range_header;
   if (headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
-    // We only care about "Range" header here.
+    // We only care about "Range" header here. This method stashes the value for
+    // later use in DidRead(), which is responsible for some of the range

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

Should this refer to DidOpen() rather than DidRead()?

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

Done.

+    // validation as well.
     std::vector<HttpByteRange> ranges;
     if (HttpUtil::ParseRangeHeader(range_header, &ranges)) {
       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.
         // TODO(hclam): decide whether we want to support multiple range
         // requests.
-        NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                    ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+        range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
       }
     }
   }
 }
 
 void URLRequestFileJob::OnSeekComplete(int64 result) {
 }
 
 void URLRequestFileJob::OnReadComplete(IOBuffer* buf, int result) {
 }
@@ skipping 40 matching lines @@
               base::File::FLAG_ASYNC;
   int rv = stream_->Open(file_path_, flags,
                          base::Bind(&URLRequestFileJob::DidOpen,
                                     weak_ptr_factory_.GetWeakPtr()));
   if (rv != ERR_IO_PENDING)
     DidOpen(rv);
 }
 
 void URLRequestFileJob::DidOpen(int result) {
   if (result != OK) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, result));
+    return;
+  }
+
+  if (range_parse_result_ != net::OK) {
+    NotifyStartError(
+        URLRequestStatus(URLRequestStatus::FAILED, range_parse_result_));
     return;
   }
 
   if (!byte_range_.ComputeBounds(meta_info_.file_size)) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-               ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED,
+                                      net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
   remaining_bytes_ = byte_range_.last_byte_position() -
                      byte_range_.first_byte_position() + 1;
   DCHECK_GE(remaining_bytes_, 0);
 
   if (remaining_bytes_ > 0 && byte_range_.first_byte_position() != 0) {
     int rv = stream_->Seek(byte_range_.first_byte_position(),
                            base::Bind(&URLRequestFileJob::DidSeek,
                                       weak_ptr_factory_.GetWeakPtr()));
     if (rv != ERR_IO_PENDING) {
       // stream_->Seek() failed, so pass an intentionally erroneous value
       // into DidSeek().
       DidSeek(-1);
     }
   } else {
     // We didn't need to call stream_->Seek() at all, so we pass to DidSeek()
     // the value that would mean seek success. This way we skip the code
     // handling seek failure.
     DidSeek(byte_range_.first_byte_position());
   }
 }
 
 void URLRequestFileJob::DidSeek(int64 result) {
   OnSeekComplete(result);
   if (result != byte_range_.first_byte_position()) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED,
+                                      ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
   set_expected_content_size(remaining_bytes_);
   NotifyHeadersComplete();
 }
 
 void URLRequestFileJob::DidRead(scoped_refptr<IOBuffer> buf, int result) {
-  if (result > 0) {
-    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
+  if (result >= 0) {
     remaining_bytes_ -= result;
     DCHECK_GE(remaining_bytes_, 0);
   }
 
   OnReadComplete(buf.get(), result);
   buf = NULL;
 
-  if (result == 0) {
-    NotifyDone(URLRequestStatus());
-  } else if (result < 0) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
-  }
-
-  NotifyReadComplete(result);
+  ReadRawDataComplete(result);
 }
 
 }  // namespace net