Add experiment to measure time to hash extension content as we read it

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 81 matching lines @@
   if (remaining_bytes_ < dest_size)
     dest_size = static_cast<int>(remaining_bytes_);
 
   // If we should copy zero bytes because |remaining_bytes_| is zero, short
   // circuit here.
   if (!dest_size) {
     *bytes_read = 0;
     return true;
   }
 
-  int rv = stream_->Read(dest, dest_size,
+  int rv = stream_->Read(dest,
+                         dest_size,
                          base::Bind(&URLRequestFileJob::DidRead,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                                    weak_ptr_factory_.GetWeakPtr(),
+                                    base::Unretained(dest)));

[rvargas (doing something else), 2014/04/08 16:53:45]

This cannot be unretained because the stream is allowed to release its reference
before calling the callback (in fact, it must), so the buffer has to be
protected somehow by this code.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Ok, I guess I was confused by both the code flow and this statement in the
io_buffer.h comments: 

"
// Whenever you call an asynchronous operation that takes an IOBuffer,
// ownership is implicitly transferred to the called function, until the
// operation has completed (at which point it transfers back to the caller).
"

In this case, the part of the logical call chain of interest is:

... 
(1) URLRequestJob::ReadRawDataHelper
(2) URLRequestFileJob::ReadRawData
(3) FileStream::Read
(4) (async reading happens and posts a task to run the callback when finished)
(5) URLRequestFileJob::DidRead
    ... URLRequestFileJob::OnReadComplete
(6) URLRequestJob::NotifyReadComplete
(7) URLRequestJob::OnRawReadComplete


At step (2) URLRequestJob::ReadRawDataHelper, the URLRequestJob sets it's
|raw_read_buffer_| scoped_refptr member variable to hold a reference to the
buffer,  and doesn't clear it until URLRequestJob::OnRawReadComplete (or
automatically in the destructor).

The callback for URLRequestFileJob::DidRead that we pass to FileStream::Read is
bound with a WeakPtr, so unless I missed something it seemed like there isn't
currently any way that we could end up getting URLRequestFileJob::DidRead called
where the buffer isn't still held by the raw_read_buffer_ member variable.

I guess future refactoring in URLRequestFileJob could break this assumption
though? I've changed the interface so that URLRequestFileJob::DidRead takes a
scoped_refptr instead of a naked pointer to the IOBuffer, and then drops its
reference once it's called URLRequestFileJob::OnReadComplete. Let me know if
this seems better to you. 

[rvargas (doing something else), 2014/04/09 23:48:50]

yeah, the comment definitely can be improved.
Yes, that's true.

The issue is that then this code relies on an implementation detail of the
caller (not _that_ bad given that it is the parent class). The normal pattern is
that callers of Foo(IOBuffer*, size, callback) may or may not keep a reference
after the call returns, even if it returns IO_PENDING. On the other hand, the
callee receives a naked pointer, and if it returns synchronously doesn't have to
do anything... if it needs the buffer after that point (returning IO_PENDING),
then it has to store the pointer somewhere and add a ref.

The last detail is that the implementation of Foo must release any reference
before invoking the callback, because when the callback runs the caller should
be able to do whatever it wants to with its buffer. (the same way that the
caller is not allowed to touch the buffer at all while the operation is pending,
which is what that comment above is trying to state).
It is better. It may be a little confusing to have a callback receiving a
scoped_refptr (if I forget that the method is a completion callback as opposed
to something that operates on the buffer... I mean, the callback cannot store
the pointer for later). To be honest, that was the first solution I considered
when I wrote the comment. Another option would be to have a member variable with
the buffer.

But I'm happy enough as it is.

   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) {
@@ skipping 70 matching lines @@
         // 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));
       }
     }
   }
 }
 
+void URLRequestFileJob::OnSeekComplete(int64 result) {}

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: use {
}
and an empty line between the methods.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done. 

(Note that 'git cl format' does {} without newline separation, but I'm happy to
conform to local style here and override it). 

+void URLRequestFileJob::OnReadComplete(net::IOBuffer* buf, int result) {}
+
 URLRequestFileJob::~URLRequestFileJob() {
 }
 
 void URLRequestFileJob::FetchMetaInfo(const base::FilePath& file_path,
                                       FileMetaInfo* meta_info) {
   base::File::Info file_info;
   meta_info->file_exists = base::GetFileInfo(file_path, &file_info);
   if (meta_info->file_exists) {
     meta_info->file_size = file_info.size;
     meta_info->is_directory = file_info.is_directory;
@@ skipping 61 matching lines @@
     }
   } 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));
     return;
   }
 
   set_expected_content_size(remaining_bytes_);
   NotifyHeadersComplete();
 }
 
-void URLRequestFileJob::DidRead(int result) {
+void URLRequestFileJob::DidRead(net::IOBuffer* buf, int result) {
+  OnReadComplete(buf, result);
   if (result > 0) {
     SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
   } else if (result == 0) {
     NotifyDone(URLRequestStatus());
   } else {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
   }
 
   remaining_bytes_ -= result;
   DCHECK_GE(remaining_bytes_, 0);
 
   NotifyReadComplete(result);
 }
 
 }  // namespace net