Rewrite guts of ResourceLoader and BufferedResourceHandler to suck less.

content/browser/renderer_host/resource_loader.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 "content/browser/renderer_host/resource_loader.h"
 
 #include "base/message_loop.h"
 #include "base/time.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/doomed_resource_handler.h"
@@ skipping 41 matching lines @@
 
 ResourceLoader::ResourceLoader(scoped_ptr<net::URLRequest> request,
                                scoped_ptr<ResourceHandler> handler,
                                ResourceLoaderDelegate* delegate)
     : deferred_stage_(DEFERRED_NONE),
       request_(request.Pass()),
       handler_(handler.Pass()),
       delegate_(delegate),
       last_upload_position_(0),
       waiting_for_upload_progress_ack_(false),
-      called_on_response_started_(false),
-      has_started_reading_(false),
-      is_paused_(false),
-      pause_count_(0),
-      paused_read_bytes_(0),
       is_transferring_(false) {
   request_->set_delegate(this);
   handler_->SetController(this);
 }
 
 ResourceLoader::~ResourceLoader() {
   if (login_delegate_)
     login_delegate_->OnRequestCancelled();
   if (ssl_client_auth_handler_)
     ssl_client_auth_handler_->OnRequestCancelled();
@@ skipping 129 matching lines @@
     CancelRequestInternal(net::ERR_UNKNOWN_URL_SCHEME, false);
     return;
   }
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse());
   PopulateResourceResponse(request_.get(), response);
 
   if (!handler_->OnRequestRedirected(info->GetRequestID(), new_url, response,
                                      defer)) {
     Cancel();
+  } else if (*defer) {
+    deferred_stage_ = DEFERRED_REDIRECT;  // Follow redirect when resumed.
   }
-
-  if (*defer)
-    deferred_stage_ = DEFERRED_REDIRECT;
 }
 
 void ResourceLoader::OnAuthRequired(net::URLRequest* unused,
                                     net::AuthChallengeInfo* auth_info) {
   DCHECK_EQ(request_.get(), unused);
 
   if (request_->load_flags() & net::LOAD_DO_NOT_PROMPT_FOR_LOGIN) {
     request_->CancelAuth();
     return;
   }
@@ skipping 55 matching lines @@
       render_view_id,
       ssl_info,
       fatal);
 }
 
 void ResourceLoader::OnResponseStarted(net::URLRequest* unused) {
   DCHECK_EQ(request_.get(), unused);
 
   VLOG(1) << "OnResponseStarted: " << request_->url().spec();
 
+  if (!request_->status().is_success()) {
+    ResponseCompleted();
+    return;
+  }
+
+  // We want to send a final upload progress message prior to sending the
+  // response complete message even if we're waiting for an ack to to a
+  // previous upload progress message.
+  waiting_for_upload_progress_ack_ = false;
+  ReportUploadProgress();
+
+  CompleteResponseStarted();
+
+  if (is_deferred())
+    return;
+
   if (request_->status().is_success()) {
-    if (PauseRequestIfNeeded()) {
-      VLOG(1) << "OnResponseStarted pausing: " << request_->url().spec();
-      return;
-    }
-
-    // We want to send a final upload progress message prior to sending the
-    // response complete message even if we're waiting for an ack to to a
-    // previous upload progress message.
-    waiting_for_upload_progress_ack_ = false;
-    ReportUploadProgress();
-
-    if (!CompleteResponseStarted()) {
-      Cancel();
-    } else {
-      // Check if the handler paused the request in their OnResponseStarted.
-      if (PauseRequestIfNeeded()) {
-        VLOG(1) << "OnResponseStarted pausing2: " << request_->url().spec();
-        return;
-      }
-
-      StartReading();
-    }
+    StartReading(false);  // Read the first chunk.
   } else {
     ResponseCompleted();
   }
 }
 
 void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
   DCHECK_EQ(request_.get(), unused);
   VLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
           << " bytes_read = " << bytes_read;
 
-  // bytes_read == -1 always implies an error, so we want to skip the pause
-  // checks and just call ResponseCompleted.
-  if (bytes_read == -1) {
-    DCHECK(!request_->status().is_success());
-
+  // bytes_read == -1 always implies an error.
+  if (bytes_read == -1 || !request_->status().is_success()) {
     ResponseCompleted();
     return;
   }
 
-  // OnReadCompleted can be called without Read (e.g., for chrome:// URLs).
-  // Make sure we know that a read has begun.
-  has_started_reading_ = true;
+  CompleteRead(bytes_read);
 
-  if (PauseRequestIfNeeded()) {
-    paused_read_bytes_ = bytes_read;
-    VLOG(1) << "OnReadCompleted pausing: \"" << request_->url().spec() << "\""
-            << " bytes_read = " << bytes_read;
+  if (is_deferred())
     return;
+
+  if (request_->status().is_success() && bytes_read > 0) {
+    StartReading(true);  // Read the next chunk.
+  } else {
+    ResponseCompleted();
   }
-
-  if (request_->status().is_success() && CompleteRead(&bytes_read)) {
-    // The request can be paused if we realize that the renderer is not
-    // servicing messages fast enough.
-    if (pause_count_ == 0 && ReadMore(&bytes_read) &&
-        request_->status().is_success()) {
-      if (bytes_read == 0) {
-        CompleteRead(&bytes_read);
-      } else {
-        // Force the next CompleteRead / Read pair to run as a separate task.
-        // This avoids a fast, large network request from monopolizing the IO
-        // thread and starving other IO operations from running.
-        VLOG(1) << "OnReadCompleted postponing: \""
-                << request_->url().spec() << "\""
-                << " bytes_read = " << bytes_read;
-
-        paused_read_bytes_ = bytes_read;
-        is_paused_ = true;
-
-        MessageLoop::current()->PostTask(
-            FROM_HERE, base::Bind(&ResourceLoader::ResumeRequest, AsWeakPtr()));
-        return;
-      }
-    }
-  }
-
-  if (PauseRequestIfNeeded()) {
-    paused_read_bytes_ = bytes_read;
-    VLOG(1) << "OnReadCompleted (CompleteRead) pausing: \""
-            << request_->url().spec() << "\""
-            << " bytes_read = " << bytes_read;
-    return;
-  }
-
-  // If the status is not IO pending then we've either finished (success) or we
-  // had an error.  Either way, we're done!
-  if (!request_->status().is_io_pending())
-    ResponseCompleted();
 }
 
 void ResourceLoader::CancelSSLRequest(const GlobalRequestID& id,
                                       int error,
                                       const net::SSLInfo* ssl_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // The request can be NULL if it was cancelled by the renderer (as the
   // request of the user navigating to a new page from the location bar).
   if (!request_->is_pending())
@@ skipping 23 matching lines @@
   switch (stage) {
     case DEFERRED_NONE:
       NOTREACHED();
       break;
     case DEFERRED_START:
       StartRequestInternal();
       break;
     case DEFERRED_REDIRECT:
       request_->FollowDeferredRedirect();
       break;
-    case DEFERRED_RESPONSE:
     case DEFERRED_READ:
-      PauseRequest(false);
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&ResourceLoader::ResumeReading, AsWeakPtr()));
       break;
     case DEFERRED_FINISH:
       // Delay self-destruction since we don't know how we were reached.
       MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(&ResourceLoader::CallDidFinishLoading, AsWeakPtr()));
       break;
   }
 }
 
@@ skipping 12 matching lines @@
   VLOG(1) << "CancelRequestInternal: " << request_->url().spec();
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
   // WebKit will send us a cancel for downloads since it no longer handles
   // them.  In this case, ignore the cancel since we handle downloads in the
   // browser.
   if (from_renderer && info->is_download())
     return;
 
+  // XXX Perhaps we should really be looking to see if the status is IO_PENDING?

[darin (slow to review), 2012/06/28 21:49:53]

Whoops, I need to change this to a TODO(darin).

   bool was_pending = request_->is_pending();
 
   if (login_delegate_) {
     login_delegate_->OnRequestCancelled();
     login_delegate_ = NULL;
   }
   if (ssl_client_auth_handler_) {
     ssl_client_auth_handler_->OnRequestCancelled();
     ssl_client_auth_handler_ = NULL;
   }
 
   request_->CancelWithError(error);
 
   if (!was_pending) {
     // If the request isn't in flight, then we won't get an asynchronous
     // notification from the request, so we have to signal ourselves to finish
     // this request.
     MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(&ResourceLoader::ResponseCompleted, AsWeakPtr()));
   }
 }
 
-bool ResourceLoader::CompleteResponseStarted() {
+void ResourceLoader::CompleteResponseStarted() {
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse());
   PopulateResourceResponse(request_.get(), response);
 
   if (request_->ssl_info().cert) {
     int cert_id =
         CertStore::GetInstance()->StoreCert(request_->ssl_info().cert,
                                             info->GetChildID());
     response->head.security_info = SerializeSecurityInfo(
         cert_id,
         request_->ssl_info().cert_status,
         request_->ssl_info().security_bits,
         request_->ssl_info().connection_status);
   } else {
     // We should not have any SSL state.
     DCHECK(!request_->ssl_info().cert_status &&
            request_->ssl_info().security_bits == -1 &&
            !request_->ssl_info().connection_status);
   }
 
   delegate_->DidReceiveResponse(this);
-  called_on_response_started_ = true;
 
   bool defer = false;
-  if (!handler_->OnResponseStarted(info->GetRequestID(), response, &defer))
-    return false;
-
-  if (defer) {
-    deferred_stage_ = DEFERRED_RESPONSE;
-    PauseRequest(true);
+  if (!handler_->OnResponseStarted(info->GetRequestID(), response, &defer)) {
+    Cancel();
+  } else if (defer) {
+    deferred_stage_ = DEFERRED_READ;  // Read first chunk when resumed.
   }
-
-  return true;
 }
 
-void ResourceLoader::StartReading() {
+void ResourceLoader::StartReading(bool is_continuation) {

[darin (slow to review), 2012/06/28 21:49:53]

NOTE: The old code tried to avoid going back to the MessageLoop in some cases:
1) When reading the first chunk of data, and 2) when reading EOF.  This
is_continuation flag and the bytes_read <= 0 check down below are used to
preserve that behavior.  I'm not sure yet if it is the best behavior, so I just
decided to preserve it.

   int bytes_read = 0;
-  if (ReadMore(&bytes_read)) {
+  ReadMore(&bytes_read);
+
+  // If IO is pending, wait for the URLRequest to call OnReadCompleted.
+  if (request_->status().is_io_pending())
+    return;
+
+  if (!is_continuation || bytes_read <= 0) {
     OnReadCompleted(request_.get(), bytes_read);
-  } else if (!request_->status().is_io_pending()) {
-    DCHECK(!is_paused_);
-    // If the error is not an IO pending, then we're done reading.
+  } else {
+    // Else, trigger OnReadCompleted asynchronously to avoid starving the IO
+    // thread in case the URLRequest can provide data synchronously.
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&ResourceLoader::OnReadCompleted, AsWeakPtr(),
+                   request_.get(), bytes_read));
+  }
+}
+
+void ResourceLoader::ResumeReading() {
+  DCHECK(!is_deferred());
+
+  if (request_->status().is_success()) {
+    StartReading(false);  // Read the next chunk (OK to complete synchronously).
+  } else {
     ResponseCompleted();
   }
 }
 
-bool ResourceLoader::ReadMore(int* bytes_read) {
+void ResourceLoader::ReadMore(int* bytes_read) {
   ResourceRequestInfoImpl* info = GetRequestInfo();
-  DCHECK(!is_paused_);
+  DCHECK(!is_deferred());
 
   net::IOBuffer* buf;
   int buf_size;
-  if (!handler_->OnWillRead(info->GetRequestID(), &buf, &buf_size, -1))
-    return false;
+  if (!handler_->OnWillRead(info->GetRequestID(), &buf, &buf_size, -1)) {
+    Cancel();
+    return;
+  }
 
   DCHECK(buf);
   DCHECK(buf_size > 0);
 
-  has_started_reading_ = true;
-  return request_->Read(buf, buf_size, bytes_read);
+  request_->Read(buf, buf_size, bytes_read);
+
+  // No need to check the return value here as we'll detect errors by
+  // inspecting the URLRequest's status.
 }
 
-bool ResourceLoader::CompleteRead(int* bytes_read) {
-  if (!request_.get() || !request_->status().is_success()) {
-    NOTREACHED();
-    return false;
-  }
+void ResourceLoader::CompleteRead(int bytes_read) {
+  DCHECK(bytes_read >= 0);
+  DCHECK(request_->status().is_success());
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
   bool defer = false;
   if (!handler_->OnReadCompleted(info->GetRequestID(), bytes_read, &defer)) {
     Cancel();
-    return false;
+  } else if (defer) {
+    deferred_stage_ = DEFERRED_READ;  // Read next chunk when resumed.
   }
-
-  if (defer) {
-    deferred_stage_ = DEFERRED_READ;
-    PauseRequest(true);
-  }
-
-  return *bytes_read != 0;
 }
 
 void ResourceLoader::ResponseCompleted() {
   VLOG(1) << "ResponseCompleted: " << request_->url().spec();
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
   std::string security_info;
   const net::SSLInfo& ssl_info = request_->ssl_info();
   if (ssl_info.cert != NULL) {
     int cert_id = CertStore::GetInstance()->StoreCert(ssl_info.cert,
                                                       info->GetChildID());
     security_info = SerializeSecurityInfo(
         cert_id, ssl_info.cert_status, ssl_info.security_bits,
         ssl_info.connection_status);
   }
 
   if (handler_->OnResponseCompleted(info->GetRequestID(), request_->status(),
                                     security_info)) {
     // This will result in our destruction.
     CallDidFinishLoading();
   } else {
     // The handler is not ready to die yet.  We will call DidFinishLoading when
     // we resume.
     deferred_stage_ = DEFERRED_FINISH;
   }
 }
 
-bool ResourceLoader::PauseRequestIfNeeded() {
-  if (pause_count_ > 0)
-    is_paused_ = true;
-  return is_paused_;
-}
-
-void ResourceLoader::PauseRequest(bool pause) {
-  int pause_count = pause_count_ + (pause ? 1 : -1);
-  if (pause_count < 0) {
-    NOTREACHED();  // Unbalanced call to pause.
-    return;
-  }
-  pause_count_ = pause_count;
-
-  VLOG(1) << "To pause (" << pause << "): " << request_->url().spec();
-
-  // If we're resuming, kick the request to start reading again. Run the read
-  // asynchronously to avoid recursion problems.
-  if (pause_count_ == 0) {
-    MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(&ResourceLoader::ResumeRequest, AsWeakPtr()));
-  }
-}
-
-void ResourceLoader::ResumeRequest() {
-  // We may already be unpaused, or the pause count may have increased since we
-  // posted the task to call ResumeRequest.
-  if (!is_paused_)
-    return;
-  is_paused_ = false;
-  if (PauseRequestIfNeeded())
-    return;
-
-  VLOG(1) << "Resuming: \"" << request_->url().spec() << "\""
-          << " paused_read_bytes = " << paused_read_bytes_
-          << " called response started = " << called_on_response_started_
-          << " started reading = " << has_started_reading_;
-
-  if (called_on_response_started_) {
-    if (has_started_reading_) {
-      OnReadCompleted(request_.get(), paused_read_bytes_);
-    } else {
-      StartReading();
-    }
-  } else {
-    OnResponseStarted(request_.get());
-  }
-}
-
 void ResourceLoader::CallDidFinishLoading() {
   delegate_->DidFinishLoading(this);
 }
 
 }  // namespace content