Avoid issuing a Read() after draining a request.

Avoid issuing a Read() after draining a request.

A URLRequest can signal the end of a stream by calling OnReadCompleted()
with a byte count of zero. No further reads should be issued after this.
However, ResourceLoader could issue a second Read() if the
ResourceHandler::OnReadCompleted() handler sets defer to true. I.e.

When a read completes:
  URLRequest calls
    -> ResourceLoader::OnReadCompleted(request, 0)
    -> ResourceHandler::OnReadCompleted(id, 0, &defer)
       (ResourceHandler sets defer to true)

Prior to this CL, the behavior on resumption was:

  ResourceHandler calls
    -> ResourceLoader::Resume()
    -> ResourceLoader::ResumeReading()
    -> ResourceLoader::StartReading()
    -> ResourceLoader::ReadMore()
    -> URLRequest::Read()

The new behavior is:

  ResourceHandler calls
    -> ResourceLoader::Resume()
    -> ResourceLoader::ResponseCompleted()

The new behavior is parallel to what happens if the ResourceHandler
didn't defer.

BUG=320394
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269518

[davidben, 2014-05-08 21:51:59]

LGTM.

Were you able to figure out why the tests don't tickle the crash itself and just
the DCHECKs? (Or do they and I'm just confused?)

[asanka, 2014-05-09 05:13:27]

On 2014/05/08 21:51:59, David Benjamin wrote:
> LGTM.
> 
> Were you able to figure out why the tests don't tickle the crash itself and
just
> the DCHECKs? (Or do they and I'm just confused?)

Yeah. The crash happens in HttpCache::Transaction::DoCacheReadData when Read()
is called after EOF. I added a reliable repro to
chrome/browser/download/download_browsertest just to make sure we've covered
that base. To repro, I had to download the same resource twice so that the
second time it will be served out of cache and we hit the
HttpCache::Transaction::ReadComplete()/DoCacheReadData() path.

[davidben, 2014-05-09 16:04:24]

Ah. New test LGTM.

apply_issue seems to be unhappy at the empty file. Maybe rename to empty.txt? Or
perhaps gzip it so it's not empty? (And matches the Content-Encoding header.) I
would expect it to still cover the DCHECKs, and the original repro was an empty
gzipped file, so it probably still triggers the crash?

We probably should also file a bug with infra...

https://codereview.chromium.org/267563004/diff/60001/chrome/browser/download/...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/267563004/diff/60001/chrome/browser/download/...
chrome/browser/download/download_browsertest.cc:3223: // cached (with a high
probability). This test verifies that that doesn't
Nit: s/that that doesn't happen/that it doesn't crash/

[asanka, 2014-05-09 16:16:02]

Thanks!

On 2014/05/09 16:04:24, David Benjamin wrote:
> Ah. New test LGTM.
> 
> apply_issue seems to be unhappy at the empty file. Maybe rename to empty.txt?
Or
> perhaps gzip it so it's not empty? (And matches the Content-Encoding header.)
I
> would expect it to still cover the DCHECKs, and the original repro was an
empty
> gzipped file, so it probably still triggers the crash?

> 
> We probably should also file a bug with infra...

empty.bin is a 20 byte GZip stream that expands to a zero length file. The try
bots still aren't happy. The issue seems to be the handling of binary files on
the try bots.

If a reupload doesn't fix it, I'll land the binary files separately with
https://codereview.chromium.org/272933002/ then try/CQ this CL.

>
https://codereview.chromium.org/267563004/diff/60001/chrome/browser/download/...
> File chrome/browser/download/download_browsertest.cc (right):
> 
>
https://codereview.chromium.org/267563004/diff/60001/chrome/browser/download/...
> chrome/browser/download/download_browsertest.cc:3223: // cached (with a high
> probability). This test verifies that that doesn't
> Nit: s/that that doesn't happen/that it doesn't crash/

[asanka, 2014-05-09 18:22:47]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-05-09 18:23:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asanka@chromium.org/267563004/100001

[commit-bot: I haz the power, 2014-05-10 04:30:04]

Change committed as 269518