Use defer out-params instead of ResourceDispatcherHostImpl::PauseRequest(...true)

content/browser/renderer_host/buffered_resource_handler.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/buffered_resource_handler.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 54 matching lines @@
     ResourceHandler* handler,
     ResourceDispatcherHostImpl* host,
     net::URLRequest* request)
     : LayeredResourceHandler(handler),
       host_(host),
       request_(request),
       read_buffer_size_(0),
       bytes_read_(0),
       sniff_content_(false),
       wait_for_plugins_(false),
+      deferred_waiting_for_plugins_(false),
       buffering_(false),
       next_handler_needs_response_started_(false),
       next_handler_needs_will_read_(false),
       finished_(false) {
 }
 
 bool BufferedResourceHandler::OnResponseStarted(
     int request_id,
-    ResourceResponse* response) {
+    ResourceResponse* response,
+    bool* defer) {
   response_ = response;
+
   if (!DelayResponse())
-    return CompleteResponseStarted(request_id);
+    return CompleteResponseStarted(request_id, defer);
+
+  if (wait_for_plugins_) {
+    deferred_waiting_for_plugins_ = true;
+    *defer = true;
+  }
   return true;
 }
 
 void BufferedResourceHandler::OnRequestClosed() {
   request_ = NULL;
   next_handler_->OnRequestClosed();
 }
 
 // We'll let the original event handler provide a buffer, and reuse it for
 // subsequent reads until we're done buffering.
 bool BufferedResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf,
                                          int* buf_size, int min_size) {
   if (buffering_) {
     DCHECK(!my_buffer_.get());
     my_buffer_ = new net::IOBuffer(net::kMaxBytesToSniff);
     *buf = my_buffer_.get();
     *buf_size = net::kMaxBytesToSniff;
     return true;
   }
 
   if (finished_)
     return false;
 
-  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size)) {
+  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size))
     return false;
-  }
+
   read_buffer_ = *buf;
   read_buffer_size_ = *buf_size;
   DCHECK_GE(read_buffer_size_, net::kMaxBytesToSniff * 2);
   bytes_read_ = 0;
   return true;
 }
 
-bool BufferedResourceHandler::OnReadCompleted(int request_id, int* bytes_read) {
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-
+bool BufferedResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
+                                              bool* defer) {
   if (sniff_content_) {
-    if (KeepBuffering(*bytes_read))
+    if (KeepBuffering(*bytes_read)) {
+      if (wait_for_plugins_) {
+        deferred_waiting_for_plugins_ = true;
+        *defer = true;
+      }
       return true;
+    }
 
     *bytes_read = bytes_read_;
 
     // Done buffering, send the pending ResponseStarted event.
-    if (!CompleteResponseStarted(request_id))
+    if (!CompleteResponseStarted(request_id, defer))
       return false;
-
-    // The next handler might have paused the request in OnResponseStarted.
-    if (info->pause_count())
+    if (*defer)
       return true;
   } else if (wait_for_plugins_) {
+    deferred_waiting_for_plugins_ = true;
+    *defer = true;
     return true;
   }
 
-  if (!ForwardPendingEventsToNextHandler(request_id))
+  if (!ForwardPendingEventsToNextHandler(request_id, defer))
     return false;
-  if (info->pause_count())
+  if (*defer)
     return true;
 
   // Release the reference that we acquired at OnWillRead.
   read_buffer_ = NULL;
-  return next_handler_->OnReadCompleted(request_id, bytes_read);
+  return next_handler_->OnReadCompleted(request_id, bytes_read, defer);
 }
 
 BufferedResourceHandler::~BufferedResourceHandler() {}
 
 bool BufferedResourceHandler::DelayResponse() {
   std::string mime_type;
   request_->GetMimeType(&mime_type);
 
   std::string content_type_options;
   request_->GetResponseHeaderByName("x-content-type-options",
@@ skipping 77 matching lines @@
   }
 
   buffering_ = false;
 
   if (wait_for_plugins_)
     return true;
 
   return false;
 }
 
-bool BufferedResourceHandler::CompleteResponseStarted(int request_id) {
+bool BufferedResourceHandler::CompleteResponseStarted(int request_id,
+                                                      bool* defer) {
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request_);
   std::string mime_type;
   request_->GetMimeType(&mime_type);
 
   // Check if this is an X.509 certificate, if yes, let it be handled
   // by X509UserCertResourceHandler.
   if (mime_type == "application/x-x509-user-cert") {
     // This is entirely similar to how DownloadResourceThrottle works except we
     // are doing it for an X.509 client certificates.
     // TODO(darin): This does not belong here!
 
     if (response_->headers &&  // Can be NULL if FTP.
         response_->headers->response_code() / 100 != 2) {
       // The response code indicates that this is an error page, but we are
       // expecting an X.509 user certificate. We follow Firefox here and show
       // our own error page instead of handling the error page as a
       // certificate.
       // TODO(abarth): We should abstract the response_code test, but this kind
       //               of check is scattered throughout our codebase.
       request_->CancelWithError(net::ERR_FILE_NOT_FOUND);
       return false;
     }
 
     X509UserCertResourceHandler* x509_cert_handler =
         new X509UserCertResourceHandler(request_,
                                         info->GetChildID(),
                                         info->GetRouteID());
-    if (!UseAlternateResourceHandler(request_id, x509_cert_handler))
+    if (!UseAlternateResourceHandler(request_id, x509_cert_handler, defer))
       return false;
   }
-
   // Check to see if we should forward the data from this request to the
   // download thread.
   // TODO(paulg): Only download if the context from the renderer allows it.
-  if (info->allow_download() && ShouldDownload(NULL)) {
+  else if (info->allow_download() && ShouldDownload(NULL)) {

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

Suggestion (as I can't find any support for my position in the style guide): I
find the separation of the else from its previous curly brace jarring; any
chance that it could be put on that line and the comment moved underneath the if
without it being confusing?

[darin (slow to review), 2012/05/18 23:09:05]

Yes, that can probably be done.

     if (response_->headers &&  // Can be NULL if FTP.
         response_->headers->response_code() / 100 != 2) {
       // The response code indicates that this is an error page, but we don't
       // know how to display the content.  We follow Firefox here and show our
       // own error page instead of triggering a download.
       // TODO(abarth): We should abstract the response_code test, but this kind
       //               of check is scattered throughout our codebase.
       request_->CancelWithError(net::ERR_FILE_NOT_FOUND);
       return false;
     }
 
     info->set_is_download(true);
 
     scoped_refptr<ResourceHandler> handler(
         host_->CreateResourceHandlerForDownload(
             request_,
             info->GetContext(),
             info->GetChildID(),
             info->GetRouteID(),
             info->GetRequestID(),
             DownloadSaveInfo(),
             DownloadResourceHandler::OnStartedCallback()));
 
-    if (!UseAlternateResourceHandler(request_id, handler))
+    if (!UseAlternateResourceHandler(request_id, handler, defer))
       return false;
   }
 
-  if (info->pause_count())
+  if (*defer)
     return true;
 
-  return next_handler_->OnResponseStarted(request_id, response_);
+  return next_handler_->OnResponseStarted(request_id, response_, defer);
 }
 
 bool BufferedResourceHandler::ShouldWaitForPlugins() {
   bool need_plugin_list;
   if (!ShouldDownload(&need_plugin_list) || !need_plugin_list)
     return false;
 
-  // We don't want to keep buffering as our buffer will fill up.
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-  host_->PauseRequest(info->GetChildID(), info->GetRequestID(), true);
-
   // Get the plugins asynchronously.
   PluginServiceImpl::GetInstance()->GetPlugins(
       base::Bind(&BufferedResourceHandler::OnPluginsLoaded, this));
+
+  // We don't want to keep buffering as our buffer will fill up.

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

nit, suggestion: This comment feels a little strange to me, disassociated from
the pause.  I think it'd make more sense at the places where we translate the
return value from ShouldWaitForPlugins into a *defer = true;

[darin (slow to review), 2012/05/18 23:09:05]

Agreed.

   return true;
 }
 
 // This test mirrors the decision that WebKit makes in
 // WebFrameLoaderClient::dispatchDecidePolicyForMIMEType.
 bool BufferedResourceHandler::ShouldDownload(bool* need_plugin_list) {
   if (need_plugin_list)
     *need_plugin_list = false;
   std::string type = StringToLowerASCII(response_->mime_type);
 
@@ skipping 32 matching lines @@
     }
   } else {
     DCHECK(!stale);
   }
 
   return !found;
 }
 
 bool BufferedResourceHandler::UseAlternateResourceHandler(
     int request_id,
-    ResourceHandler* handler) {
+    ResourceHandler* handler,
+    bool* defer) {
   // Inform the original ResourceHandler that this will be handled entirely by
   // the new ResourceHandler.
   // TODO(darin): We should probably check the return values of these.
-  next_handler_->OnResponseStarted(request_id, response_);
+  bool defer_ignored = false;
+  next_handler_->OnResponseStarted(request_id, response_, &defer_ignored);
+  DCHECK(!defer_ignored);

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

What's the rational for this?  

[darin (slow to review), 2012/05/18 23:09:05]

The old code fails to handle pausing done by the old handler.  I think it should
probably be fixed.  This DCHECK will complain if the handler does try to pause,
and then we can notice that and fix this code to handle it properly.

[Randy Smith (Not in Mondays), 2012/05/19 00:42:14]

SG.

   net::URLRequestStatus status(net::URLRequestStatus::HANDLED_EXTERNALLY, 0);
   next_handler_->OnResponseCompleted(request_id, status, std::string());
 
   // Remove the non-owning pointer to the CrossSiteResourceHandler, if any,
   // from the extra request info because the CrossSiteResourceHandler (part of
   // the original ResourceHandler chain) will be deleted by the next statement.
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request_);
   info->set_cross_site_handler(NULL);
 
   // This is handled entirely within the new ResourceHandler, so just reset the
   // original ResourceHandler.
   next_handler_ = handler;
 
   next_handler_needs_response_started_ = true;
   next_handler_needs_will_read_ = true;
 
-  return ForwardPendingEventsToNextHandler(request_id);
+  return ForwardPendingEventsToNextHandler(request_id, defer);
 }
 
 bool BufferedResourceHandler::ForwardPendingEventsToNextHandler(
-    int request_id) {
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-  if (info->pause_count())
-    return true;
-
+    int request_id,
+    bool* defer) {

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

nit: Why two lines?

[darin (slow to review), 2012/05/18 23:09:05]

Looks like the first parameter actually fits on the previous line.

   if (next_handler_needs_response_started_) {
-    if (!next_handler_->OnResponseStarted(request_id, response_))
+    if (!next_handler_->OnResponseStarted(request_id, response_, defer))
       return false;
-    // If the request was paused during OnResponseStarted, we need to avoid
+    // If the request was deferred during OnResponseStarted, we need to avoid
     // calling OnResponseStarted again.
     next_handler_needs_response_started_ = false;
-    if (info->pause_count())
+    if (*defer)
       return true;
   }
 
   if (next_handler_needs_will_read_) {
     CopyReadBufferToNextHandler(request_id);
-    // If the request was paused during OnWillRead, we need to be sure to try
-    // calling OnWillRead again.
-    if (info->pause_count())
-      return true;
     next_handler_needs_will_read_ = false;
   }
   return true;
 }
 
 void BufferedResourceHandler::CopyReadBufferToNextHandler(int request_id) {
   if (!bytes_read_)
     return;
 
   net::IOBuffer* buf = NULL;
   int buf_len = 0;
   if (next_handler_->OnWillRead(request_id, &buf, &buf_len, bytes_read_)) {
     CHECK((buf_len >= bytes_read_) && (bytes_read_ >= 0));
     memcpy(buf->data(), read_buffer_->data(), bytes_read_);
   }
 }
 
 void BufferedResourceHandler::OnPluginsLoaded(
     const std::vector<webkit::WebPluginInfo>& plugins) {
+  bool needs_resume = deferred_waiting_for_plugins_;
+  deferred_waiting_for_plugins_ = false;
+
   wait_for_plugins_ = false;
   if (!request_)
     return;
 
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request_);
   int child_id = info->GetChildID();
   int request_id = info->GetRequestID();
 
-  host_->PauseRequest(child_id, request_id, false);
-  if (!CompleteResponseStarted(request_id))
+  bool defer = false;
+  if (!CompleteResponseStarted(request_id, &defer)) {
     host_->CancelRequest(child_id, request_id, false);
+  } else if (!defer && needs_resume) {
+    host_->ResumeDeferredRequest(child_id, request_id);
+  }
 }
 
 }  // namespace content