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

chrome/browser/predictors/resource_prefetcher.cc

Index: chrome/browser/predictors/resource_prefetcher.cc
diff --git a/chrome/browser/predictors/resource_prefetcher.cc b/chrome/browser/predictors/resource_prefetcher.cc
index 44b1057c4165839429cda432b446087e27efc25b..fd8cfa38e4b39d4657396a37a722a2ed001ad5ae 100644
--- a/chrome/browser/predictors/resource_prefetcher.cc
+++ b/chrome/browser/predictors/resource_prefetcher.cc
@@ -169,21 +169,29 @@ void ResourcePrefetcher::ReadFullResponse(net::URLRequest* request) {
         kResourceBufferSizeBytes));
     status = request->Read(buffer, kResourceBufferSizeBytes, &bytes_read);
 
-    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);
-        return;
-      }
+    if (request->status().error()) {

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

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]

Done.

+      FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
+      return;
     }
+
+    if (status)
+      status = ShouldContinueReadingRequest(request, bytes_read);
   }
 }
 
+bool ResourcePrefetcher::ShouldContinueReadingRequest(net::URLRequest* request,
+                                                      int bytes_read) {
+  if (bytes_read == 0) {  // When bytes_read == 0, no more data.
+    if (request->was_cached())
+      FinishRequest(request, Request::PREFETCH_STATUS_FROM_CACHE);
+    else
+      FinishRequest(request, Request::PREFETCH_STATUS_FROM_NETWORK);
+    return false;
+  }
+
+  return true;
+}
+
 void ResourcePrefetcher::OnReceivedRedirect(net::URLRequest* request,
                                             const GURL& new_url,
                                             bool* defer_redirect) {
@@ -213,6 +221,7 @@ void ResourcePrefetcher::OnResponseStarted(net::URLRequest* request) {
     return;
   }
 
+  // TODO(shishir): Do not read cached entries, or ones that are not cacheable.
   ReadFullResponse(request);
 }
 
@@ -223,7 +232,8 @@ void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request,
     return;
   }
 
-  ReadFullResponse(request);
+  if (ShouldContinueReadingRequest(request, bytes_read))
+    ReadFullResponse(request);
 }
 
 }  // namespace predictors