Refactor ResourceHandler API.

content/browser/loader/intercepting_resource_handler.cc

Index: content/browser/loader/intercepting_resource_handler.cc
diff --git a/content/browser/loader/intercepting_resource_handler.cc b/content/browser/loader/intercepting_resource_handler.cc
index 733ca71d22de50ef164acb6f184dac791ae4d42a..69af9f871772772a1bb440fdd42c587a24514328 100644
--- a/content/browser/loader/intercepting_resource_handler.cc
+++ b/content/browser/loader/intercepting_resource_handler.cc
@@ -4,59 +4,92 @@
 
 #include "content/browser/loader/intercepting_resource_handler.h"
 
+#include "base/auto_reset.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "content/browser/loader/null_resource_controller.h"
+#include "content/browser/loader/resource_controller.h"
 #include "content/public/common/resource_response.h"
 #include "net/base/io_buffer.h"
 #include "net/url_request/url_request.h"
 
 namespace content {
 
+class InterceptingResourceHandler::Controller : public ResourceController {
+ public:
+  explicit Controller(InterceptingResourceHandler* mime_handler)
+      : intercepting_handler_(mime_handler) {}
+
+  void Resume() override {
+    MarkAsUsed();
+    intercepting_handler_->ResumeInternal();
+  }
+
+  void Cancel() override {
+    MarkAsUsed();
+    intercepting_handler_->Cancel();
+  }
+
+  void CancelAndIgnore() override {
+    MarkAsUsed();
+    intercepting_handler_->CancelAndIgnore();
+  }
+
+  void CancelWithError(int error_code) override {
+    MarkAsUsed();
+    intercepting_handler_->CancelWithError(error_code);
+  }
+
+ private:
+  void MarkAsUsed() {
+#if DCHECK_IS_ON()
+    DCHECK(!used_);
+    used_ = true;
+#endif
+  }
+
+#if DCHECK_IS_ON()
+  bool used_ = false;
+#endif
+  InterceptingResourceHandler* intercepting_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(Controller);
+};
+
 InterceptingResourceHandler::InterceptingResourceHandler(
     std::unique_ptr<ResourceHandler> next_handler,
     net::URLRequest* request)
     : LayeredResourceHandler(request, std::move(next_handler)),
       weak_ptr_factory_(this) {
-  next_handler_->SetController(this);
 }
 
 InterceptingResourceHandler::~InterceptingResourceHandler() {}
 
-void InterceptingResourceHandler::SetController(
-    ResourceController* controller) {
-  if (state_ == State::PASS_THROUGH)
-    return LayeredResourceHandler::SetController(controller);
-  ResourceHandler::SetController(controller);
-}
-
-bool InterceptingResourceHandler::OnResponseStarted(ResourceResponse* response,
-                                                    bool* defer) {
+void InterceptingResourceHandler::OnResponseStarted(
+    ResourceResponse* response,
+    std::unique_ptr<ResourceController> controller) {
   // If there's no need to switch handlers, just start acting as a blind
   // pass-through ResourceHandler.
   if (!new_handler_) {
     state_ = State::PASS_THROUGH;
-    next_handler_->SetController(controller());
-    return next_handler_->OnResponseStarted(response, defer);
+    next_handler_->OnResponseStarted(response, std::move(controller));
+    return;
   }
 
   DCHECK_EQ(state_, State::STARTING);
-  // Otherwise, switch handlers. First, inform the original ResourceHandler
-  // that this will be handled entirely by the new ResourceHandler.
-  bool defer_ignored = false;
-  if (!next_handler_->OnResponseStarted(response, &defer_ignored))
-    return false;
-
-  // Although deferring OnResponseStarted is legal, the only downstream handler
-  // which does so is CrossSiteResourceHandler. Cross-site transitions should
-  // not trigger when switching handlers.
-  DCHECK(!defer_ignored);
 
   // TODO(yhirano): Retaining ownership from a raw pointer is bad.
   response_ = response;
-  state_ = State::SENDING_PAYLOAD_TO_OLD_HANDLER;
-  return DoLoop(defer);
+
+  // Otherwise, switch handlers. First, inform the original ResourceHandler
+  // that this will be handled entirely by the new ResourceHandler.
+  HoldController(std::move(controller));
+  state_ = State::SWAPPING_HANDLERS;
+
+  DoLoop();
 }
 
 bool InterceptingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
@@ -78,7 +111,11 @@ bool InterceptingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
   return true;
 }
 
-bool InterceptingResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
+void InterceptingResourceHandler::OnReadCompleted(
+    int bytes_read,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
+
   DCHECK_GE(bytes_read, 0);
   if (state_ == State::PASS_THROUGH) {
     if (first_read_buffer_double_) {
@@ -89,88 +126,65 @@ bool InterceptingResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
       first_read_buffer_ = nullptr;
       first_read_buffer_double_ = nullptr;
     }
-    return next_handler_->OnReadCompleted(bytes_read, defer);
+    next_handler_->OnReadCompleted(bytes_read, std::move(controller));
+    return;
   }
 
   DCHECK_EQ(State::WAITING_FOR_ON_READ_COMPLETED, state_);
   first_read_buffer_bytes_read_ = bytes_read;
   state_ = State::SENDING_BUFFER_TO_NEW_HANDLER;
-  return DoLoop(defer);
+  HoldController(std::move(controller));
+  DoLoop();
 }
 
 void InterceptingResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
-    bool* defer) {
+    std::unique_ptr<ResourceController> controller) {
   if (state_ == State::PASS_THROUGH) {
-    LayeredResourceHandler::OnResponseCompleted(status, defer);
+    LayeredResourceHandler::OnResponseCompleted(status, std::move(controller));
     return;
   }
   if (!new_handler_) {
     // Therer is only one ResourceHandler in this InterceptingResourceHandler.
     state_ = State::PASS_THROUGH;
     first_read_buffer_double_ = nullptr;
-    next_handler_->SetController(controller());
-    next_handler_->OnResponseCompleted(status, defer);
+    next_handler_->OnResponseCompleted(status, std::move(controller));
     return;
   }
 
   // There are two ResourceHandlers in this InterceptingResourceHandler.
   // |next_handler_| is the old handler and |new_handler_| is the new handler.
   // As written in the class comment, this class assumes that the old handler
-  // will not set |*defer| in OnResponseCompleted.
-  next_handler_->SetController(controller());
-  next_handler_->OnResponseCompleted(status, defer);
-  DCHECK(!*defer);
+  // will immediately call Resume() in OnResponseCompleted.
+  bool was_resumed = false;
+  next_handler_->OnResponseCompleted(

[asanka, 2017/01/30 22:20:49]

You probably want to swap out the delegate for the RH chain starting at
next_handler_. Otherwise any OOB cancellations would choke this call stack.

[mmenke, 2017/01/30 22:36:02]

Note that |next_handler_| is destroyed 4 lines down, and in-band out-of-band
cancellation isn't allowed.  I'm reluctant to swap the RH::Delegate because
doing that is currently allowed, and I don't want to introduce it just for this
(If I swap it with anything, I'd actually swap it with nullptr, so we crash,
since cancel here is currently not handled - and cancel on OnResponseCompleted
is more generally not allowed, per the ResourceHandler API).

I'd actually like to get rid of NullResourceController entirely, just working
around it in the places that use it was too complicated for this already huge
CL.  I already have a TODO in one place to get rid of it, I've added them to all
callsites now.

I'd like to add a DCHECK that we don't OOB cancel synchronously, but
sufficiently nervous about this CL that I'm not going to add it now.

+      status, base::MakeUnique<NullResourceController>(&was_resumed));
+  DCHECK(was_resumed);
 
   state_ = State::PASS_THROUGH;
   first_read_buffer_double_ = nullptr;
-  new_handler_->SetController(controller());
   next_handler_ = std::move(new_handler_);
-  next_handler_->OnResponseCompleted(status, defer);
-}
-
-void InterceptingResourceHandler::Cancel() {
-  DCHECK_NE(State::PASS_THROUGH, state_);
-  controller()->Cancel();
-}
-
-void InterceptingResourceHandler::CancelAndIgnore() {
-  DCHECK_NE(State::PASS_THROUGH, state_);
-  controller()->CancelAndIgnore();
-}
-
-void InterceptingResourceHandler::CancelWithError(int error_code) {
-  DCHECK_NE(State::PASS_THROUGH, state_);
-  controller()->CancelWithError(error_code);
-}
-
-void InterceptingResourceHandler::Resume() {
-  DCHECK_NE(State::PASS_THROUGH, state_);
-  if (state_ == State::STARTING ||
-      state_ == State::WAITING_FOR_ON_READ_COMPLETED) {
-    // Uninteresting Resume: just delegate to the original resource controller.
-    controller()->Resume();
-    return;
-  }
-
-  // Can't call DoLoop synchronously, as it may call into |next_handler_|
-  // synchronously, which is what called Resume().
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&InterceptingResourceHandler::AdvanceState,
-                            weak_ptr_factory_.GetWeakPtr()));
+  next_handler_->OnResponseCompleted(status, std::move(controller));
 }
 
 void InterceptingResourceHandler::UseNewHandler(
     std::unique_ptr<ResourceHandler> new_handler,
     const std::string& payload_for_old_handler) {
   new_handler_ = std::move(new_handler);
-  new_handler_->SetController(this);
+  new_handler_->SetDelegate(delegate());
   payload_for_old_handler_ = payload_for_old_handler;
 }
 
-bool InterceptingResourceHandler::DoLoop(bool* defer) {
-  bool result = true;
-  do {
+void InterceptingResourceHandler::DoLoop() {
+  DCHECK(!in_do_loop_);
+  DCHECK(!advance_to_next_state_);
+
+  base::AutoReset<bool> auto_in_do_loop(&in_do_loop_, true);
+  advance_to_next_state_ = true;
+
+  while (advance_to_next_state_) {
+    advance_to_next_state_ = false;
+
     switch (state_) {
       case State::STARTING:
       case State::WAITING_FOR_ON_READ_COMPLETED:
@@ -178,7 +192,7 @@ bool InterceptingResourceHandler::DoLoop(bool* defer) {
         NOTREACHED();
         break;
       case State::SENDING_ON_WILL_START_TO_NEW_HANDLER:
-        result = SendOnResponseStartedToNewHandler(defer);
+        SendOnResponseStartedToNewHandler();
         break;
       case State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER:
         if (first_read_buffer_double_) {
@@ -190,105 +204,127 @@ bool InterceptingResourceHandler::DoLoop(bool* defer) {
           // OnWillRead has not been called, so no special handling will be
           // needed from now on.
           state_ = State::PASS_THROUGH;
-          next_handler_->SetController(controller());
         }
+        ResumeInternal();
+        break;
+      case State::SWAPPING_HANDLERS:
+        SendOnResponseStartedToOldHandler();
         break;
       case State::SENDING_PAYLOAD_TO_OLD_HANDLER:
-        result = SendPayloadToOldHandler(defer);
+        SendPayloadToOldHandler();
         break;
       case State::SENDING_BUFFER_TO_NEW_HANDLER:
-        result = SendFirstReadBufferToNewHandler(defer);
+        SendFirstReadBufferToNewHandler();
         break;
     }
-  } while (result && !*defer &&
-           state_ != State::WAITING_FOR_ON_READ_COMPLETED &&
-           state_ != State::PASS_THROUGH);
-  return result;
+  }
 }
 
-bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) {
-  DCHECK_EQ(State::SENDING_PAYLOAD_TO_OLD_HANDLER, state_);
-  while (payload_bytes_written_ < payload_for_old_handler_.size()) {
-    scoped_refptr<net::IOBuffer> buffer;
-    int size = 0;
-    if (first_read_buffer_) {
-      // |first_read_buffer_| is a buffer gotten from |next_handler_| via
-      // OnWillRead. Use the buffer.
-      buffer = first_read_buffer_;
-      size = first_read_buffer_size_;
+void InterceptingResourceHandler::ResumeInternal() {
+  DCHECK(has_controller());
+  if (state_ == State::STARTING ||
+      state_ == State::WAITING_FOR_ON_READ_COMPLETED ||
+      state_ == State::PASS_THROUGH) {
+    // Uninteresting Resume: just delegate to the original resource controller.
+    Resume();
+    return;
+  }
 
-      first_read_buffer_ = nullptr;
-      first_read_buffer_size_ = 0;
-    } else {
-      if (!next_handler_->OnWillRead(&buffer, &size, -1))
-        return false;
-    }
+  // If called recusively from a DoLoop, advance state when returning to the
+  // loop.
+  if (in_do_loop_) {
+    DCHECK(!advance_to_next_state_);
+    advance_to_next_state_ = true;
+    return;
+  }
 
-    size = std::min(size, static_cast<int>(payload_for_old_handler_.size() -
-                                           payload_bytes_written_));
-    memcpy(buffer->data(),
-           payload_for_old_handler_.data() + payload_bytes_written_, size);
-    if (!next_handler_->OnReadCompleted(size, defer))
-      return false;
-    payload_bytes_written_ += size;
-    if (*defer)
-      return true;
+  // Can't call DoLoop synchronously, as it may call into |next_handler_|
+  // synchronously, which is what called Resume().
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&InterceptingResourceHandler::DoLoop,
+                            weak_ptr_factory_.GetWeakPtr()));
+}
+
+void InterceptingResourceHandler::SendOnResponseStartedToOldHandler() {
+  state_ = State::SENDING_PAYLOAD_TO_OLD_HANDLER;
+  next_handler_->OnResponseStarted(response_.get(),
+                                   base::MakeUnique<Controller>(this));
+}
+
+void InterceptingResourceHandler::SendPayloadToOldHandler() {
+  DCHECK_EQ(State::SENDING_PAYLOAD_TO_OLD_HANDLER, state_);
+  if (payload_bytes_written_ == payload_for_old_handler_.size()) {
+    net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0);
+    if (payload_for_old_handler_.empty()) {
+      // If there is no payload, just finalize the request on the old handler.
+      status = net::URLRequestStatus::FromError(net::ERR_ABORTED);
+    }
+    bool was_resumed = false;
+    next_handler_->OnResponseCompleted(
+        status, base::MakeUnique<NullResourceController>(&was_resumed));
+    DCHECK(was_resumed);
+
+    next_handler_ = std::move(new_handler_);
+    state_ = State::SENDING_ON_WILL_START_TO_NEW_HANDLER;
+    next_handler_->OnWillStart(request()->url(),
+                               base::MakeUnique<Controller>(this));
+    return;
   }
 
-  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0);
-  if (payload_for_old_handler_.empty()) {
-    // If there is no payload, just finalize the request on the old handler.
-    status = net::URLRequestStatus::FromError(net::ERR_ABORTED);
+  scoped_refptr<net::IOBuffer> buffer;
+  int size = 0;
+  if (first_read_buffer_) {
+    // |first_read_buffer_| is a buffer gotten from |next_handler_| via
+    // OnWillRead. Use the buffer.
+    buffer = first_read_buffer_;
+    size = first_read_buffer_size_;
+
+    first_read_buffer_ = nullptr;
+    first_read_buffer_size_ = 0;
+  } else {
+    if (!next_handler_->OnWillRead(&buffer, &size, -1)) {
+      Cancel();
+      return;
+    }
   }
-  next_handler_->OnResponseCompleted(status, defer);
-  DCHECK(!*defer);
 
-  next_handler_ = std::move(new_handler_);
-  state_ = State::SENDING_ON_WILL_START_TO_NEW_HANDLER;
-  return next_handler_->OnWillStart(request()->url(), defer);
+  size = std::min(size, static_cast<int>(payload_for_old_handler_.size() -
+                                         payload_bytes_written_));
+  memcpy(buffer->data(),
+         payload_for_old_handler_.data() + payload_bytes_written_, size);
+  payload_bytes_written_ += size;
+  next_handler_->OnReadCompleted(size, base::MakeUnique<Controller>(this));
 }
 
-bool InterceptingResourceHandler::SendOnResponseStartedToNewHandler(
-    bool* defer) {
+void InterceptingResourceHandler::SendOnResponseStartedToNewHandler() {
   state_ = State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
-  return next_handler_->OnResponseStarted(response_.get(), defer);
+  next_handler_->OnResponseStarted(response_.get(),
+                                   base::MakeUnique<Controller>(this));
 }
 
-bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) {
+void InterceptingResourceHandler::SendFirstReadBufferToNewHandler() {
   DCHECK_EQ(state_, State::SENDING_BUFFER_TO_NEW_HANDLER);
 
-  while (first_read_buffer_bytes_written_ < first_read_buffer_bytes_read_) {
-    scoped_refptr<net::IOBuffer> buf;
-    int size = 0;
-    if (!next_handler_->OnWillRead(&buf, &size, -1))
-      return false;
-    size = std::min(size, static_cast<int>(first_read_buffer_bytes_read_ -
-                                           first_read_buffer_bytes_written_));
-    memcpy(buf->data(),
-           first_read_buffer_double_->data() + first_read_buffer_bytes_written_,
-           size);
-    if (!next_handler_->OnReadCompleted(size, defer))
-      return false;
-    first_read_buffer_bytes_written_ += size;
-    if (*defer)
-      return true;
+  if (first_read_buffer_bytes_written_ == first_read_buffer_bytes_read_) {
+    state_ = State::PASS_THROUGH;
+    first_read_buffer_double_ = nullptr;
+    ResumeInternal();
+    return;
   }
 
-  state_ = State::PASS_THROUGH;
-  first_read_buffer_double_ = nullptr;
-  next_handler_->SetController(controller());
-  return true;
-}
-
-void InterceptingResourceHandler::AdvanceState() {
-  bool defer = false;
-  if (!DoLoop(&defer)) {
-    controller()->Cancel();
+  scoped_refptr<net::IOBuffer> buf;
+  int size = 0;
+  if (!next_handler_->OnWillRead(&buf, &size, -1)) {
+    Cancel();
     return;
   }
-
-  if (!defer)
-    controller()->Resume();
+  size = std::min(size, static_cast<int>(first_read_buffer_bytes_read_ -
+                                         first_read_buffer_bytes_written_));
+  memcpy(buf->data(),
+         first_read_buffer_double_->data() + first_read_buffer_bytes_written_,
+         size);
+  first_read_buffer_bytes_written_ += size;
+  next_handler_->OnReadCompleted(size, base::MakeUnique<Controller>(this));
 }
 
 }  // namespace content