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

chrome/browser/predictors/resource_prefetcher.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.
 
 #include <iterator>
 
 #include "chrome/browser/predictors/resource_prefetcher.h"
 
 #include "base/stl_util.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 149 matching lines @@
 
   TryToLaunchPrefetchRequests();
 }
 
 void ResourcePrefetcher::ReadFullResponse(net::URLRequest* request) {
   bool status = true;
   while (status) {
     int bytes_read = 0;
     scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(
         kResourceBufferSizeBytes));
     status = request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);

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

I'm surprised that we keep reading and discarding the data, but that's beyond
this CL.

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

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

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

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

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

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

 
     if (status) {
       if (request->status().error()) {
         FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
         return;
       } else if (bytes_read == 0) {
         if (request->was_cached())
           FinishRequest(request, Request::PREFETCH_STATUS_FROM_CACHE);
         else
           FinishRequest(request, Request::PREFETCH_STATUS_FROM_NETWORK);
@@ skipping 30 matching lines @@
   if (request->status().error()) {
     FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
     return;
   }
 
   ReadFullResponse(request);
 }
 
 void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request,
                                          int bytes_read) {
   if (request->status().error()) {

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

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.

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

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.

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

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)

     FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
     return;
   }
 
+  // If the bytes_read is <= 0, there is no more data to read.
+  if (bytes_read <= 0) {

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

Note that this condition is not the same one used on ReadFullResponse.

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

Fixed this to be == 0 as above.

+    if (request->was_cached())
+      FinishRequest(request, Request::PREFETCH_STATUS_FROM_CACHE);
+    else
+      FinishRequest(request, Request::PREFETCH_STATUS_FROM_NETWORK);
+    return;
+  }
+
   ReadFullResponse(request);
 }
 
 }  // namespace predictors