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 151 matching lines @@
 }
 
 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);
 
-    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) {
   FinishRequest(request, Request::PREFETCH_STATUS_REDIRECTED);
 }
 
 void ResourcePrefetcher::OnAuthRequired(net::URLRequest* request,
                                         net::AuthChallengeInfo* auth_info) {
   FinishRequest(request, Request::PREFETCH_STATUS_AUTH_REQUIRED);
 }
 
 void ResourcePrefetcher::OnCertificateRequested(
     net::URLRequest* request,
     net::SSLCertRequestInfo* cert_request_info) {
   FinishRequest(request, Request::PREFETCH_STATUS_CERT_REQUIRED);
 }
 
 void ResourcePrefetcher::OnSSLCertificateError(net::URLRequest* request,
                                                const net::SSLInfo& ssl_info,
                                                bool fatal) {
   FinishRequest(request, Request::PREFETCH_STATUS_CERT_ERROR);
 }
 
 void ResourcePrefetcher::OnResponseStarted(net::URLRequest* request) {
   if (request->status().error()) {
     FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
     return;
   }
 
+  // TODO(shishir): Do not read cached entries, or ones that are not cacheable.
   ReadFullResponse(request);
 }
 
 void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request,
                                          int bytes_read) {
   if (request->status().error()) {
     FinishRequest(request, Request::PREFETCH_STATUS_FAILED);
     return;
   }
 
-  ReadFullResponse(request);
+  if (ShouldContinueReadingRequest(request, bytes_read))
+    ReadFullResponse(request);
 }
 
 }  // namespace predictors