Fixing possible read on URLRequest when there is no more data.

Fixing possible read on URLRequest when there is no more data.

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

[Shishir, 2013-01-03 22:00:26]

PTAL. I would like to merge this into M25.

[rvargas (doing something else), 2013-01-03 23:16:09]

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:170: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
I'm surprised that we keep reading and discarding the data, but that's beyond
this CL.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:221: if
(request->status().error()) {
This is a lot of code duplication with ReadFullResponse, and arguably the cause
of the bug was not having common code separated into a helper method.

I think ReadFullResponse should call something to test for conditions to finish
reading if Read() returns true, and OnReadCompleted should call the same method
to evaluate the result of the operation before attempting to read again.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:227: if (bytes_read <= 0) {
Note that this condition is not the same one used on ReadFullResponse.

[Shishir, 2013-01-04 00:29:04]

PTAL

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:170: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
On 2013/01/03 23:16:09, rvargas wrote:
> I'm surprised that we keep reading and discarding the data, but that's beyond
> this CL.

We dont need to use the data, only warm up the cache.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:221: if
(request->status().error()) {
On 2013/01/03 23:16:09, rvargas wrote:
> This is a lot of code duplication with ReadFullResponse, and arguably the
cause
> of the bug was not having common code separated into a helper method.
> 
> I think ReadFullResponse should call something to test for conditions to
finish
> reading if Read() returns true, and OnReadCompleted should call the same
method
> to evaluate the result of the operation before attempting to read again.

Not sure if I fully understand your comment. The condition is bytes_read == 0
and adding a function for it seems a bit overboard. I agree there is a bit of
code duplication but I dont think adding a condition to check for further read
will fix that.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:227: if (bytes_read <= 0) {
On 2013/01/03 23:16:09, rvargas wrote:
> Note that this condition is not the same one used on ReadFullResponse.

Fixed this to be == 0 as above.

[rvargas (doing something else), 2013-01-04 01:04:59]

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:170: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
On 2013/01/04 00:29:04, Shishir wrote:
> On 2013/01/03 23:16:09, rvargas wrote:
> > I'm surprised that we keep reading and discarding the data, but that's
beyond
> > this CL.
> 
> We dont need to use the data, only warm up the cache. 

yes, I'm surprised that we keep reading even if the data is already cached, or
not cacheable.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:221: if
(request->status().error()) {
I meant:
while (status) {
  status = req->Read();
  if (status)
    status = SeeIfWeHaveToKeepReading(req, bytesRead)
}

and:
if (SeeIfWeHaveToKeepReading(req, bytesRead))
  ReadFullResponse(req);

That makes very explicit what has to be done after a successful read. (I'm not
suggesting that name for real)

[Shishir, 2013-01-04 01:29:15]

Hi Ricardo,

I have attempted to do what you suggested, but it looks a bit trickier now. The
check is just one condition (bytes_read == 0) and we end up calling the
FinishRequest functions from within the new functions which deletes the request
ptrs.

Since I need to merge this into M25, I would prefer to take the path of the
previous patch.

PTAL.

Thanks.

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/1/chrome/browser/predictors/res...
chrome/browser/predictors/resource_prefetcher.cc:170: status =
request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
On 2013/01/04 01:04:59, rvargas wrote:
> On 2013/01/04 00:29:04, Shishir wrote:
> > On 2013/01/03 23:16:09, rvargas wrote:
> > > I'm surprised that we keep reading and discarding the data, but that's
> beyond
> > > this CL.
> > 
> > We dont need to use the data, only warm up the cache. 
> 
> yes, I'm surprised that we keep reading even if the data is already cached, or
> not cacheable.

Right! Will add a todo. Will fix this in an upcoming cl.

[rvargas (doing something else), 2013-01-04 02:02:40]

I don't fully follow what you're saying.

* The code is the same on trunk and M25 so there won't be any issues merging.

* The check is not one line (!bytes_read). It is looking at the request status,
bytes_read and was_cached and returning one of three possible status codes (or
nothing). Copy-paste of all that seems inappropriate to me, precisely because
the logic was wrong after the first attempt to copy paste (the current code on
M25)

* I don't see why calling FinishRequest from another method presents any
problem. If you really want to avoid that, you could return the desired status
from the new methods and do the call outside (I see no reason to complicate the
code that way)

* If you ever want to upgrade that logic (for instance to avoid reading
uncacheable content), changes will have to go in two places instead of one.

* We are not talking about a big refactoring here that would delay fixing the
branch.

https://codereview.chromium.org/11761022/diff/7001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/7001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.cc:172: if
(request->status().error()) {
The contract is not to inspect status() regardless of the return value. The
first thing to check is the return value of the function (as the old code does).

[Shishir, 2013-01-04 02:21:14]

PTAL.

https://codereview.chromium.org/11761022/diff/7001/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/7001/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.cc:172: if
(request->status().error()) {
On 2013/01/04 02:02:40, rvargas wrote:
> The contract is not to inspect status() regardless of the return value. The
> first thing to check is the return value of the function (as the old code
does).

From the comment on the Read function:
// If a read error occurs, Read returns false and the request->status
// will be set to an error.

So if read returns false, its either because its in async mode or there is an
error. Earlier we were checking error when status was true which would never
happen. This condition will rightly bail out in the case where there is an error
bit set and the read returns false.

[rvargas (doing something else), 2013-01-04 04:31:04]

oops

https://codereview.chromium.org/11761022/diff/2003/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/2003/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.cc:172: if
(request->status().error()) {
This is an interesting point. The issue is that on a synchronous failure the
callback is still delivered (unless the request is deleted before that). In
other words, the old code also works.

But you're right, looking for an error after a successful call and not a failed
one is confusing. But now we are looking at the error code without looking first
at the return value, and that's also confusing. I guess it should have been
located at the else of the if.

Now I see your point about the extra method... it is leaving half the check
outside, so it's not that nice :(. It could use a different logic to see if the
request is pending before looking at the result, but the "nice" thing to do is
to look at the return value first :(

I'm sorry for the detour.

I guess either we put all the logic into the new method (relying on the extra
callback to happen), or go back to your initial change (that means this line
should really be on the else branch). Or continue with this approach and move
this line to the else(status).

[Shishir, 2013-01-04 19:52:32]

PTAL

https://codereview.chromium.org/11761022/diff/2003/chrome/browser/predictors/...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/11761022/diff/2003/chrome/browser/predictors/...
chrome/browser/predictors/resource_prefetcher.cc:172: if
(request->status().error()) {
On 2013/01/04 04:31:04, rvargas wrote:
> This is an interesting point. The issue is that on a synchronous failure the
> callback is still delivered (unless the request is deleted before that). In
> other words, the old code also works.
> 
> But you're right, looking for an error after a successful call and not a
failed
> one is confusing. But now we are looking at the error code without looking
first
> at the return value, and that's also confusing. I guess it should have been
> located at the else of the if.
> 
> Now I see your point about the extra method... it is leaving half the check
> outside, so it's not that nice :(. It could use a different logic to see if
the
> request is pending before looking at the result, but the "nice" thing to do is
> to look at the return value first :(
> 
> I'm sorry for the detour.
> 
> I guess either we put all the logic into the new method (relying on the extra
> callback to happen), or go back to your initial change (that means this line
> should really be on the else branch). Or continue with this approach and move
> this line to the else(status).

Done.

[rvargas (doing something else), 2013-01-04 21:06:39]

lgtm

[commit-bot: I haz the power, 2013-01-04 22:42:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11761022/14001

[commit-bot: I haz the power, 2013-01-05 01:51:38]

Change committed as 175257