Refactor ResourceHandler API.

content/browser/loader/intercepting_resource_handler.cc

 // Copyright 2016 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/intercepting_resource_handler.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 {
+    DCHECK(!used_);
+    used_ = true;
+    intercepting_handler_->ResumeInternal();
+  }
+
+  void Cancel() override {
+    DCHECK(!used_);
+    used_ = true;
+    intercepting_handler_->Cancel();
+  }
+
+  void CancelAndIgnore() override {
+    DCHECK(!used_);
+    used_ = true;
+    intercepting_handler_->CancelAndIgnore();
+  }
+
+  void CancelWithError(int error_code) override {
+    DCHECK(!used_);
+    used_ = true;
+    intercepting_handler_->CancelWithError(error_code);
+  }
+
+ private:
+  InterceptingResourceHandler* intercepting_handler_;
+  bool used_ = false;

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

I hate to recommend it but since this member is only used for DCHECKs I'd wrap
it in #if DCHECK_IS_ON(). Same for your other controllers. You could write a
method:

void CheckUse() {
#if DCHECK_IS_ON()
  DCHECK(!used_);
  used_ = true;
#endif
}

The empty method will probably be elided.

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

Done, but it really seems like overkill to me.

+
+  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,
                                              int* buf_size,
                                              int min_size) {
   if (state_ == State::PASS_THROUGH)
     return next_handler_->OnWillRead(buf, buf_size, min_size);
 
   DCHECK_EQ(State::STARTING, state_);
   DCHECK_EQ(-1, min_size);
 
   if (!next_handler_->OnWillRead(buf, buf_size, min_size))
     return false;
 
   first_read_buffer_ = *buf;
   first_read_buffer_size_ = *buf_size;
   first_read_buffer_double_ = new net::IOBuffer(static_cast<size_t>(*buf_size));
   *buf = first_read_buffer_double_;
   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_) {
       // |first_read_buffer_double_| was allocated and the user wrote data to
       // the buffer, but switching has not been done after all.
       memcpy(first_read_buffer_->data(), first_read_buffer_double_->data(),
              bytes_read);
       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(
+      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);
+  next_handler_->OnResponseCompleted(status, std::move(controller));
 }
 
-void InterceptingResourceHandler::Cancel() {
-  DCHECK_NE(State::PASS_THROUGH, state_);
-  controller()->Cancel();
-}
+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;
+  }
 
-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();
+  // If called recusively from a DoLoop, advance state when return to the loop.

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

nit: s/return/returning

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

Done.

+  if (in_do_loop_) {
+    DCHECK(!advance_to_next_state_);
+    advance_to_next_state_ = true;
     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,
+      FROM_HERE, base::Bind(&InterceptingResourceHandler::DoLoop,
                             weak_ptr_factory_.GetWeakPtr()));
 }
 
 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_);
+
+  in_do_loop_ = true;

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

optional: use base::AutoReset?

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

Done.

+  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:
       case State::PASS_THROUGH:
         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_) {
           // OnWillRead has been called, so copying the data from
           // |first_read_buffer_double_| to |first_read_buffer_| will be needed
           // when OnReadCompleted is called.
           state_ = State::WAITING_FOR_ON_READ_COMPLETED;
+          ResumeInternal();
         } else {
           // 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;
+  }
+
+  in_do_loop_ = false;
 }
 
-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_;
-
-      first_read_buffer_ = nullptr;
-      first_read_buffer_size_ = 0;
-    } else {
-      if (!next_handler_->OnWillRead(&buffer, &size, -1))
-        return false;
-    }
-
-    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;
-  }
-
-  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);
-  }
-  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);
+void InterceptingResourceHandler::SendOnResponseStartedToOldHandler() {
+  state_ = State::SENDING_PAYLOAD_TO_OLD_HANDLER;
+  next_handler_->OnResponseStarted(response_.get(),
+                                   base::MakeUnique<Controller>(this));
 }
 
-bool InterceptingResourceHandler::SendOnResponseStartedToNewHandler(
-    bool* defer) {
-  state_ = State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
-  return next_handler_->OnResponseStarted(response_.get(), defer);
-}
+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);
 
-bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) {
-  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;
-  }
-
-  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();
+    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;
   }
 
-  if (!defer)
-    controller()->Resume();
+  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;
+    }
+  }
+
+  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));
+}
+
+void InterceptingResourceHandler::SendOnResponseStartedToNewHandler() {
+  state_ = State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
+  next_handler_->OnResponseStarted(response_.get(),
+                                   base::MakeUnique<Controller>(this));
+}
+
+void InterceptingResourceHandler::SendFirstReadBufferToNewHandler() {
+  DCHECK_EQ(state_, State::SENDING_BUFFER_TO_NEW_HANDLER);
+
+  if (first_read_buffer_bytes_written_ == first_read_buffer_bytes_read_) {
+    state_ = State::PASS_THROUGH;
+    first_read_buffer_double_ = nullptr;
+    ResumeInternal();
+    return;
+  }
+
+  scoped_refptr<net::IOBuffer> buf;
+  int size = 0;
+  if (!next_handler_->OnWillRead(&buf, &size, -1)) {
+    Cancel();
+    return;
+  }
+  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