Refactor ResourceHandler API.

content/browser/loader/mime_sniffing_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/loader/mime_sniffing_resource_handler.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/mime_util/mime_util.h"
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/loader/intercepting_resource_handler.h"
+#include "content/browser/loader/resource_controller.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/loader/stream_resource_handler.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_save_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
@@ skipping 27 matching lines @@
       : net::WrappedIOBuffer(buf->data() + offset), buf_(buf) {}
 
  private:
   ~DependentIOBuffer() override {}
 
   scoped_refptr<net::IOBuffer> buf_;
 };
 
 }  // namespace
 
+class MimeSniffingResourceHandler::Controller : public ResourceController {

[Charlie Harrison, 2017/01/25 20:22:59]

This looks just like a couple of other controllers, I would support merging
logic. The only issue is "ResumeInternal" though. Still, even with a base class
that implements everything else but makes Resume() virtual would be good. 

[mmenke, 2017/01/25 22:07:59]

I think there are only two classes like this?  ResourceLoader's is kinda like
this, but it's not a ResourceHandler, and having a class implement two
interfaces with the same method names is just not good.  I'd have to make 4
virtual methods in the parent class, not one.  Given that, I'm not sure it's
worth it...Particularly as it's not a pattern I want to encourage, and the
InterceptingResourceHandler should ideally go away, some day, assuming all the
Mojo stuff goes as planned.

[Charlie Harrison, 2017/01/26 00:25:50]

Yeah, I probably just looked at this one 5 times and misattributed. SGTM

+ public:
+  explicit Controller(MimeSniffingResourceHandler* mime_handler)
+      : mime_handler_(mime_handler) {}
+
+  void Resume() override {
+    DCHECK(!used_);
+    used_ = true;
+    mime_handler_->ResumeInternal();
+  }
+
+  void Cancel() override {
+    DCHECK(!used_);
+    used_ = true;
+    mime_handler_->Cancel();
+  }
+
+  void CancelAndIgnore() override {
+    DCHECK(!used_);
+    used_ = true;
+    mime_handler_->CancelAndIgnore();
+  }
+
+  void CancelWithError(int error_code) override {
+    DCHECK(!used_);
+    used_ = true;
+    mime_handler_->CancelWithError(error_code);
+  }
+
+ private:
+  MimeSniffingResourceHandler* mime_handler_;
+  bool used_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(Controller);
+};
+
 MimeSniffingResourceHandler::MimeSniffingResourceHandler(
     std::unique_ptr<ResourceHandler> next_handler,
     ResourceDispatcherHostImpl* host,
     PluginService* plugin_service,
     InterceptingResourceHandler* intercepting_handler,
     net::URLRequest* request,
     RequestContextType request_context_type)
     : LayeredResourceHandler(request, std::move(next_handler)),
       state_(STATE_STARTING),
       host_(host),
 #if BUILDFLAG(ENABLE_PLUGINS)
       plugin_service_(plugin_service),
 #endif
       must_download_(false),
       must_download_is_set_(false),
       read_buffer_size_(0),
       bytes_read_(0),
       intercepting_handler_(intercepting_handler),
       request_context_type_(request_context_type),
+      in_state_loop_(false),
+      advance_state_(false),
       weak_ptr_factory_(this) {
 }
 
 MimeSniffingResourceHandler::~MimeSniffingResourceHandler() {}
 
-void MimeSniffingResourceHandler::SetController(
-    ResourceController* controller) {
-  ResourceHandler::SetController(controller);
+void MimeSniffingResourceHandler::OnWillStart(
+    const GURL& url,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
 
-  // Downstream handlers see the MimeSniffingResourceHandler as their
-  // ResourceController, which allows it to consume part or all of the resource
-  // response, and then later replay it to downstream handler.
-  DCHECK(next_handler_.get());
-  next_handler_->SetController(this);
-}
-
-bool MimeSniffingResourceHandler::OnWillStart(const GURL& url, bool* defer) {
   const char* accept_value = nullptr;
   switch (GetRequestInfo()->GetResourceType()) {
     case RESOURCE_TYPE_MAIN_FRAME:
     case RESOURCE_TYPE_SUB_FRAME:
       accept_value = kFrameAcceptHeader;
       break;
     case RESOURCE_TYPE_STYLESHEET:
       accept_value = kStylesheetAcceptHeader;
       break;
     case RESOURCE_TYPE_FAVICON:
@@ skipping 16 matching lines @@
       accept_value = kDefaultAcceptHeader;
       break;
     case RESOURCE_TYPE_LAST_TYPE:
       NOTREACHED();
       break;
   }
 
   // The false parameter prevents overwriting an existing accept header value,
   // which is needed because JS can manually set an accept header on an XHR.
   request()->SetExtraRequestHeaderByName(kAcceptHeader, accept_value, false);
-  return next_handler_->OnWillStart(url, defer);
+  next_handler_->OnWillStart(url, std::move(controller));
 }
 
-bool MimeSniffingResourceHandler::OnResponseStarted(ResourceResponse* response,
-                                                    bool* defer) {
+void MimeSniffingResourceHandler::OnResponseStarted(
+    ResourceResponse* response,
+    std::unique_ptr<ResourceController> controller) {
   DCHECK_EQ(STATE_STARTING, state_);
+  DCHECK(!has_controller());
+
   response_ = response;
 
   state_ = STATE_BUFFERING;
   // A 304 response should not contain a Content-Type header (RFC 7232 section
   // 4.1). The following code may incorrectly attempt to add a Content-Type to
   // the response, and so must be skipped for 304 responses.
   if (!(response_->head.headers.get() &&
         response_->head.headers->response_code() == 304)) {
-    if (ShouldSniffContent())
-      return true;
+    if (ShouldSniffContent()) {
+      controller->Resume();
+      return;
+    }
 
     if (response_->head.mime_type.empty()) {
       // Ugg.  The server told us not to sniff the content but didn't give us a
       // mime type.  What's a browser to do?  Turns out, we're supposed to
       // treat the response as "text/plain".  This is the most secure option.
       response_->head.mime_type.assign("text/plain");
     }
 
     // Treat feed types as text/plain.
     if (response_->head.mime_type == "application/rss+xml" ||
         response_->head.mime_type == "application/atom+xml") {
       response_->head.mime_type.assign("text/plain");
     }
   }
 
-  return ProcessState(defer);
+  HoldController(std::move(controller));
+  AdvanceState();
 }
 
 bool MimeSniffingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                                              int* buf_size,
                                              int min_size) {
   if (state_ == STATE_STREAMING)
     return next_handler_->OnWillRead(buf, buf_size, min_size);
 
   DCHECK_EQ(-1, min_size);
 
   if (read_buffer_.get()) {
     CHECK_LT(bytes_read_, read_buffer_size_);
     *buf = new DependentIOBuffer(read_buffer_.get(), bytes_read_);
     *buf_size = read_buffer_size_ - bytes_read_;
   } else {
     if (!next_handler_->OnWillRead(buf, buf_size, min_size))
       return false;
 
     read_buffer_ = *buf;
     read_buffer_size_ = *buf_size;
     DCHECK_GE(read_buffer_size_, net::kMaxBytesToSniff * 2);
   }
   return true;
 }
 
-bool MimeSniffingResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
-  if (state_ == STATE_STREAMING)
-    return next_handler_->OnReadCompleted(bytes_read, defer);
+void MimeSniffingResourceHandler::OnReadCompleted(
+    int bytes_read,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
+
+  if (state_ == STATE_STREAMING) {
+    next_handler_->OnReadCompleted(bytes_read, std::move(controller));
+    return;
+  }
 
   DCHECK_EQ(state_, STATE_BUFFERING);
   bytes_read_ += bytes_read;
 
   const std::string& type_hint = response_->head.mime_type;
 
   std::string new_type;
   bool made_final_decision =
       net::SniffMimeType(read_buffer_->data(), bytes_read_, request()->url(),
                          type_hint, &new_type);
 
   // SniffMimeType() returns false if there is not enough data to determine
   // the mime type. However, even if it returns false, it returns a new type
   // that is probably better than the current one.
   response_->head.mime_type.assign(new_type);
 
-  if (!made_final_decision && (bytes_read > 0))
-    return true;
+  if (!made_final_decision && (bytes_read > 0)) {
+    controller->Resume();
+    return;
+  }
 
-  return ProcessState(defer);
+  HoldController(std::move(controller));
+  AdvanceState();
 }
 
 void MimeSniffingResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
-    bool* defer) {
+    std::unique_ptr<ResourceController> resource_controller) {
   // Upon completion, act like a pass-through handler in case the downstream
   // handler defers OnResponseCompleted.
   state_ = STATE_STREAMING;
 
-  next_handler_->OnResponseCompleted(status, defer);
+  next_handler_->OnResponseCompleted(status, std::move(resource_controller));
 }
 
-void MimeSniffingResourceHandler::Resume() {
+void MimeSniffingResourceHandler::ResumeInternal() {
+  DCHECK_NE(state_, STATE_BUFFERING);
+  DCHECK(!advance_state_);
+
   // If no information is currently being transmitted to downstream handlers,
   // they should not attempt to resume the request.
   if (state_ == STATE_BUFFERING) {
     NOTREACHED();

[Charlie Harrison, 2017/01/25 20:22:59]

Unnecessary NOTREACHED() with the DCHECK above. It is against chromium style to
need the early return for this "unreachable" code so I would also like that
removed (or turn the DCHECK into a CHECK if we aren't sure).

[mmenke, 2017/01/25 22:07:59]

I left this as it is mostly because of the sheer size of this CL.  If we want to
clean up the state machine, which I'm fine with, I think we want to do it in
another CL.  I really don't want want to have to re-land this CL.

[Charlie Harrison, 2017/01/26 00:25:50]

Acknowledged.

     return;
   }
 
-  // If the BufferingHandler is acting as a pass-through handler, just ask the
-  // upwards ResourceController to resume the request.
-  if (state_ == STATE_STARTING || state_ == STATE_STREAMING) {
-    controller()->Resume();
+  if (in_state_loop_) {
+    advance_state_ = true;
     return;
   }
 
   // Otherwise proceed with the replay of the response. If it is successful,
   // it will resume the request. Posted as a task to avoid re-entrancy into
   // the calling class.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&MimeSniffingResourceHandler::AdvanceState,
                             weak_ptr_factory_.GetWeakPtr()));
 }
 
-void MimeSniffingResourceHandler::Cancel() {
-  controller()->Cancel();
-}
+void MimeSniffingResourceHandler::AdvanceState() {
+  DCHECK(!in_state_loop_);
+  DCHECK(!advance_state_);
 
-void MimeSniffingResourceHandler::CancelAndIgnore() {
-  controller()->CancelAndIgnore();
-}
+  in_state_loop_ = true;
+  advance_state_ = true;
+  while (advance_state_) {
+    advance_state_ = false;
 
-void MimeSniffingResourceHandler::CancelWithError(int error_code) {
-  controller()->CancelWithError(error_code);
-}
-
-void MimeSniffingResourceHandler::AdvanceState() {
-  bool defer = false;
-  if (!ProcessState(&defer)) {
-    Cancel();
-  } else if (!defer) {
-    DCHECK_EQ(STATE_STREAMING, state_);
-    controller()->Resume();
-  }
-}
-
-bool MimeSniffingResourceHandler::ProcessState(bool* defer) {
-  bool return_value = true;
-  while (!*defer && return_value && state_ != STATE_STREAMING) {
     switch (state_) {
       case STATE_BUFFERING:
-        return_value = MaybeIntercept(defer);
+        MaybeIntercept();
         break;
       case STATE_INTERCEPTION_CHECK_DONE:
-        return_value = ReplayResponseReceived(defer);
+        ReplayResponseReceived();
         break;
       case STATE_REPLAYING_RESPONSE_RECEIVED:
-        return_value = ReplayReadCompleted(defer);
+        ReplayReadCompleted();
         break;
+      case STATE_STARTING:
+      case STATE_STREAMING:
+        in_state_loop_ = false;
+        Resume();
+        return;
       default:
         NOTREACHED();
         break;
     }
   }
-  return return_value;
+
+  DCHECK(in_state_loop_);
+  in_state_loop_ = false;
 }
 
-bool MimeSniffingResourceHandler::MaybeIntercept(bool* defer) {
+void MimeSniffingResourceHandler::MaybeIntercept() {
   DCHECK_EQ(STATE_BUFFERING, state_);
   // If a request that can be intercepted failed the check for interception
   // step, it should be canceled.
-  if (!MaybeStartInterception(defer))
-    return false;
+  if (!MaybeStartInterception())
+    return;
 
-  if (!*defer)
-    state_ = STATE_INTERCEPTION_CHECK_DONE;
-
-  return true;
+  state_ = STATE_INTERCEPTION_CHECK_DONE;
+  ResumeInternal();
 }
 
-bool MimeSniffingResourceHandler::ReplayResponseReceived(bool* defer) {
+void MimeSniffingResourceHandler::ReplayResponseReceived() {
   DCHECK_EQ(STATE_INTERCEPTION_CHECK_DONE, state_);
   state_ = STATE_REPLAYING_RESPONSE_RECEIVED;
-  return next_handler_->OnResponseStarted(response_.get(), defer);
+  next_handler_->OnResponseStarted(response_.get(),
+                                   base::MakeUnique<Controller>(this));
 }
 
-bool MimeSniffingResourceHandler::ReplayReadCompleted(bool* defer) {
+void MimeSniffingResourceHandler::ReplayReadCompleted() {
   DCHECK_EQ(STATE_REPLAYING_RESPONSE_RECEIVED, state_);
 
   state_ = STATE_STREAMING;
 
-  if (!read_buffer_.get())
-    return true;
+  if (!read_buffer_.get()) {
+    ResumeInternal();
+    return;
+  }
 
-  bool result = next_handler_->OnReadCompleted(bytes_read_, defer);
+  int bytes_read = bytes_read_;
 
   read_buffer_ = nullptr;
   read_buffer_size_ = 0;
   bytes_read_ = 0;
 
-  return result;
+  next_handler_->OnReadCompleted(bytes_read,
+                                 base::MakeUnique<Controller>(this));
 }
 
 bool MimeSniffingResourceHandler::ShouldSniffContent() {
   if (request_context_type_ == REQUEST_CONTEXT_TYPE_FETCH) {
     // MIME sniffing should be disabled for a request initiated by fetch().
     return false;
   }
 
   const std::string& mime_type = response_->head.mime_type;
 
@@ skipping 10 matching lines @@
     // We're going to look at the data before deciding what the content type
     // is.  That means we need to delay sending the ResponseStarted message
     // over the IPC channel.
     VLOG(1) << "To buffer: " << request()->url().spec();
     return true;
   }
 
   return false;
 }
 
-bool MimeSniffingResourceHandler::MaybeStartInterception(bool* defer) {
+bool MimeSniffingResourceHandler::MaybeStartInterception() {
   if (!CanBeIntercepted())
     return true;
 
   DCHECK(!response_->head.mime_type.empty());
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
   const std::string& mime_type = response_->head.mime_type;
 
   // Allow requests for object/embed tags to be intercepted as streams.
   if (info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
     DCHECK(!info->allow_download());
 
     bool handled_by_plugin;
-    if (!CheckForPluginHandler(defer, &handled_by_plugin))
+    if (!CheckForPluginHandler(&handled_by_plugin))
       return false;
-    if (handled_by_plugin || *defer)
+    if (handled_by_plugin)
       return true;
   }
 
   if (!info->allow_download())
     return true;
 
   // info->allow_download() == true implies
   // info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME or
   // info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME.
   DCHECK(info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME ||
          info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME);
 
   bool must_download = MustDownload();
   if (!must_download) {
     if (mime_util::IsSupportedMimeType(mime_type))
       return true;
 
     bool handled_by_plugin;
-    if (!CheckForPluginHandler(defer, &handled_by_plugin))
+    if (!CheckForPluginHandler(&handled_by_plugin))
       return false;
-    if (handled_by_plugin || *defer)
+    if (handled_by_plugin)
       return true;
   }
 
   // This request is a download.
 
   if (!CheckResponseIsNotProvisional())
     return false;
 
   info->set_is_download(true);
   std::unique_ptr<ResourceHandler> handler(
       host_->CreateResourceHandlerForDownload(request(),
                                               true,  // is_content_initiated
                                               must_download,
                                               false /* is_new_request */));
   intercepting_handler_->UseNewHandler(std::move(handler), std::string());
   return true;
 }
 
 bool MimeSniffingResourceHandler::CheckForPluginHandler(
-    bool* defer,
     bool* handled_by_plugin) {
   *handled_by_plugin = false;
 #if BUILDFLAG(ENABLE_PLUGINS)
   ResourceRequestInfoImpl* info = GetRequestInfo();
   bool allow_wildcard = false;
   bool stale;
   WebPluginInfo plugin;
   bool has_plugin = plugin_service_->GetPluginInfo(
       info->GetChildID(), info->GetRenderFrameID(), info->GetContext(),
       request()->url(), url::Origin(), response_->head.mime_type,
       allow_wildcard, &stale, &plugin, NULL);
 
   if (stale) {
     // Refresh the plugins asynchronously.
     plugin_service_->GetPlugins(
         base::Bind(&MimeSniffingResourceHandler::OnPluginsLoaded,
                    weak_ptr_factory_.GetWeakPtr()));
     request()->LogBlockedBy("MimeSniffingResourceHandler");
-    *defer = true;
-    return true;
+    // Will complete asynchronously.
+    return false;
   }
 
   if (has_plugin && plugin.type != WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) {
     *handled_by_plugin = true;
     return true;
   }
 
   // Attempt to intercept the request as a stream.
   base::FilePath plugin_path;
   if (has_plugin)
     plugin_path = plugin.path;
   std::string payload;
   std::unique_ptr<ResourceHandler> handler(host_->MaybeInterceptAsStream(
       plugin_path, request(), response_.get(), &payload));
   if (handler) {
-    if (!CheckResponseIsNotProvisional())
+    if (!CheckResponseIsNotProvisional()) {
+      Cancel();
       return false;
+    }
     *handled_by_plugin = true;
     intercepting_handler_->UseNewHandler(std::move(handler), payload);
   }
 #endif
   return true;
 }
 
 bool MimeSniffingResourceHandler::CanBeIntercepted() {
   if (response_->head.headers.get() &&
       response_->head.headers->response_code() == 304) {
@@ skipping 43 matching lines @@
 
 void MimeSniffingResourceHandler::OnPluginsLoaded(
     const std::vector<WebPluginInfo>& plugins) {
   // No longer blocking on the plugins being loaded.
   request()->LogUnblocked();
   if (state_ == STATE_BUFFERING)
     AdvanceState();
 }
 
 }  // namespace content