Create blobs from Disk Cache entries.

storage/browser/blob/blob_url_request_job.cc

Index: storage/browser/blob/blob_url_request_job.cc
diff --git a/storage/browser/blob/blob_url_request_job.cc b/storage/browser/blob/blob_url_request_job.cc
index 26607cd3e5c5db4b7cca2b66b0b69ec1883a3857..ce199cbec8e0df8e5b5717121101527f10d7bc16 100644
--- a/storage/browser/blob/blob_url_request_job.cc
+++ b/storage/browser/blob/blob_url_request_job.cc
@@ -22,6 +22,7 @@
 #include "base/trace_event/trace_event.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
+#include "net/disk_cache/disk_cache.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
@@ -356,6 +357,8 @@ bool BlobURLRequestJob::ReadItem() {
   const BlobDataItem& item = *items.at(current_item_index_);
   if (item.type() == DataElement::TYPE_BYTES)
     return ReadBytesItem(item, bytes_to_read);
+  if (item.type() == DataElement::TYPE_DISK_CACHE_ENTRY)
+    return ReadDiskCacheEntryItem(item, bytes_to_read);
   if (!IsFileType(item.type())) {
     NOTREACHED();
     return false;
@@ -420,7 +423,7 @@ bool BlobURLRequestJob::ReadFileItem(FileStreamReader* reader,
   const int result =
       reader->Read(read_buf_.get(), bytes_to_read,
                    base::Bind(&BlobURLRequestJob::DidReadFile,
-                              base::Unretained(this), chunk_number));
+                              weak_factory_.GetWeakPtr(), chunk_number));
   if (result >= 0) {
     // Data is immediately available.
     if (GetStatus().is_io_pending())
@@ -468,6 +471,49 @@ void BlobURLRequestJob::DeleteCurrentFileReader() {
   }
 }
 
+bool BlobURLRequestJob::ReadDiskCacheEntryItem(const BlobDataItem& item,

[mmenke, 2015/06/17 19:11:16]

Are there tests that exercise this code?

[michaeln, 2015/06/17 20:49:35]

good question, there should be tests for this in
blob_url_request_job_unittest.cc

+                                               int bytes_to_read) {
+  DCHECK_GE(read_buf_->BytesRemaining(), bytes_to_read);
+
+  const int result = item.disk_cache_entry()->ReadData(
+      item.disk_cache_stream_index(), current_item_offset_, read_buf_.get(),
+      bytes_to_read, base::Bind(&BlobURLRequestJob::DidReadDiskCacheEntry,
+                                weak_factory_.GetWeakPtr()));
+  if (result >= 0) {
+    // Data is immediately available.
+    if (GetStatus().is_io_pending())

[mmenke, 2015/06/17 19:11:16]

I'm going to refrain from commenting on this.  Mostly.  Already done elsewhere
in this file...must...not...comment....

[mmenke, 2015/06/17 19:20:05]

This looks like a bug, actually.  In the async case, io will be pending, but
we'll also be calling this from the ReadLoop call on line 513, so they'll *both*
call NotifyReadComplete, if the first read is asynchronous but the next read
completes synchronously, right?

This is presumably also a bug in the other place that does it.

[dmurph, 2015/06/17 20:18:07]

This class is super old, crufty, and the current state is....not friendly for
the reader.  It will get love at the last state of the refactor.  We can add
TODO(dmurph)s at some places.

Any insights on the bad parts of this file is appreciated, I've had trouble
figuring out what parts are normal for network request jobs and what parts are
bad.

Other than that, I'm not sure if there is a bug or not, but we haven't noticed
anything.  We have layout tests that exercise this system as well.

[mmenke, 2015/06/17 20:29:25]

My guess is that's because reads generally always happen asynchronously, or
greedily (Or both?).  However, I'm pretty sure it's a bug that we could easily
repro in a unit test, with a mocked out disk_cache::Entry.

Something else I'm wondering:  UploadElementsReaders and this code need similar
logic to handle multiple concatenated blobs of different types.  Could we make
them share some sort of blob reader internally instead, and get rid of both this
class and simplify UploadElementsReaders (To each reader only having one type of
data - we'd still want to support in-memory and on-disk uploads, of course)

Oh, and I'm happy to consult on the refactoring.

[michaeln, 2015/06/17 20:49:36]

Hmmm, in practice this might not be a bug with the filestream reading case
because that class always operates asyncly.

[mmenke, 2015/06/17 20:57:12]

Unless, of course, you can concatenate blobs of different types, some of which
are async, and some sync...

[michaeln, 2015/06/17 21:27:54]

That would be ideal and might help eliminate a bug or two that causes
urlreqeusts to outlive their contexts, provided internal consumers of
bloburlrequests were retrofitted to use the internal blobreader class instead.

[gavinp, 2015/06/18 18:50:55]

Can you be more explicit about which function is double called? I just read
through the code and I'm not convinced there's a bug here.

In this case, we've performed a read on the underlying disk cache, and it's come
back synchronously. If we've already performed an async read earlier in
servicing the read of the BlobUrlRequestJob and thus already decided to response
asynchronously, we toss this in to that pile and concatenate reads as needed by
calling DidReadDiskCacheEntry() on this result, which indeed chains another read
until we're done.

If the underlying read on the disk cache returns asynchronously, we chain
through this callback anyway, so these code paths are identical, right?

In the case that we got back a synchronous result on the underlying disk cache
item, AND we have not already decided we're stuck going synchronous, we continue
the synchronous story, advancing our read pointer in the hopes that we won't
have a false return from the ReadLoop().

Isn't this good? (if crufty and very likely to be improved by the refactorings
you and others have suggested)

[mmenke, 2015/06/18 19:01:55]

Sorry, I missed the SetStatus call in BlobURLRequestJob.

So...erm...Is it even possible for the status to be is_io_pending() when this
method is invoked?  It looks like in both the places this class sets it, it's
cleared in the next callback, except in the case of failure, and this shouldn't
be called again until after the previous read completes.

[gavinp, 2015/06/18 19:49:42]

I think this is possible.

Imagine a read made of a BlobURLRequestJob (the "high level read") which spans
two underlying disk cache or file entries that are expected to return
asynchronously. I think we'd expect:

1. A read is made of the first disk cache resource (the "first read"). This
first read to the underlying disk cache entry returns ERR_IO_PENDING; SetStatus
means we now are is_io_pending(), and BlobURLRequestJob::ReadRawData() returns
false.

2. The first read completes, and BlobURLRequestJob::DidReadFooEntry() is called.
The read was successful, and so we tail-call ReadLoop() from our
DidRead...Entry(). This immediately issues a read of the second disk cache
resource (the "second read"). ***BINGOBINGOBINGO GetStatus().is_io_pending() is
expected to be true here BINGOBINGOBINGO**

3. This second read returns ERR_IO_PENDING. Not much happens until...

4. The second read completes, and BloblURLRequestJob::DidReadFooEntry() is
called. When ReadLoop() is called, there's no more bytes to read, and so
NotifyReadComplete() gets called and the high level read completes per the usual
URLRequestJob logic.

[mmenke, 2015/06/18 19:58:22]

Why is is_io_pending true there?  We set it to ok again in
BlobURLRequestJob::DidReadFooEntry.

If it were true, we'd actually be in trouble on sync success, which is what I
was worried about.  IF it were true, we'd call DidReadDiskCacheEntry(result);
here, which would call into NotifyReadComplete(), if we had nothing more to do. 
And then we'd return true.  That true would be passed back up to ReadLoop(),
which (Again if we had nothing left to read), would return true.  And then we'd
be back up to the outer call to DidReadDiskCacheEntry, which would *also* call
NotifyReadComplete(), which would be bad.  Right?

[gavinp, 2015/06/18 20:19:52]

Right.

I've removed the dead code, and added a few fairly explicitly documented DCHECKs
that guard against this. While I was in DidReadFileEntry, I removed the dead
code you identified in my cut and paste.

+      DidReadDiskCacheEntry(result);
+    else
+      AdvanceBytesRead(result);
+    return true;
+  }
+  if (result == net::ERR_IO_PENDING)
+    SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+  else
+    NotifyFailure(result);
+  return false;
+}
+
+void BlobURLRequestJob::DidReadDiskCacheEntry(int result) {
+  if (result <= 0) {

[mmenke, 2015/06/17 19:11:16]

This is weird:  If ReadData returns an error synchronously, we fail with that
error.  But if it returns an error asynchronously, we always return ERR_FAILED,
rather than the actual error.

[gavinp, 2015/06/18 18:50:55]

Fixed here, and in the case for files.

+    NotifyFailure(net::ERR_FAILED);
+    return;
+  }
+  SetStatus(net::URLRequestStatus());
+
+  AdvanceBytesRead(result);
+
+  if (!read_buf_->BytesRemaining()) {

[mmenke, 2015/06/17 19:11:16]

Is this block needed?  The ReadLoop call below already does exactly this.

[gavinp, 2015/06/18 18:50:55]

Done.

+    int bytes_read = BytesReadCompleted();
+    NotifyReadComplete(bytes_read);
+    return;
+  }
+
+  int bytes_read = 0;
+  if (ReadLoop(&bytes_read))
+    NotifyReadComplete(bytes_read);
+}
+
 int BlobURLRequestJob::BytesReadCompleted() {
   int bytes_read = read_buf_->BytesConsumed();
   read_buf_ = NULL;