Relax ad-hoc assumptions on InterceptingResourceHandler

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/logging.h"
 #include "base/strings/string_util.h"
 #include "content/public/common/resource_response.h"
 #include "net/base/io_buffer.h"
 
 namespace content {
 
 InterceptingResourceHandler::InterceptingResourceHandler(
     std::unique_ptr<ResourceHandler> next_handler,
     net::URLRequest* request)
-    : LayeredResourceHandler(request, std::move(next_handler)),
-      state_(State::STARTING),
-      first_read_buffer_size_(0) {}
+    : LayeredResourceHandler(request, std::move(next_handler)) {
+  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) {
   // If there's no need to switch handlers, just start acting as a blind
   // pass-through ResourceHandler.
   if (!new_handler_) {
-    state_ = State::DONE;
+    state_ = State::PASS_THROUGH;
     first_read_buffer_ = nullptr;
+    next_handler_->SetController(controller());
     return next_handler_->OnResponseStarted(response, defer);
   }
 
+  DCHECK_EQ(state_, State::STARTING);
   // Otherwise, switch handlers. First, inform the original ResourceHandler
   // that this will be handled entirely by the new ResourceHandler.
-  // TODO(clamy): We will probably need to check the return values of these for
-  // PlzNavigate.
   bool defer_ignored = false;
-  next_handler_->OnResponseStarted(response, &defer_ignored);
+  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);
 
-  // Make a copy of the data in the first read buffer. Despite not having been
-  // informed of any data being stored in first_read_buffer_, the
-  // MimeSniffingResourceHandler has read the data, it's just holding it back.
-  // This data should be passed to the alternate ResourceHandler and not to to
-  // the current ResourceHandler.
-  // TODO(clamy): see if doing the copy should be moved to the
-  // MimeSniffingResourceHandler.
-  if (first_read_buffer_) {
-    first_read_buffer_copy_ = new net::IOBuffer(first_read_buffer_size_);
-    memcpy(first_read_buffer_copy_->data(), first_read_buffer_->data(),
-           first_read_buffer_size_);
-  }
+  if (!new_handler_->OnResponseStarted(response, defer))
+    return false;
 
-  // Send the payload to the old handler.
-  SendPayloadToOldHandler();
-  first_read_buffer_ = nullptr;
-
-  // The original ResourceHandler is now no longer needed, so replace it with
-  // the new one, before sending the response to the new one.
-  next_handler_ = std::move(new_handler_);
-
-  state_ =
-      first_read_buffer_copy_ ? State::WAITING_FOR_BUFFER_COPY : State::DONE;
-
-  return next_handler_->OnResponseStarted(response, defer);
+  if (*defer)
+    state_ = State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
+  return true;
 }
 
 bool InterceptingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                                              int* buf_size,
                                              int min_size) {
-  if (state_ == State::DONE)
+  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;
   return true;
 }
 
 bool InterceptingResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
-  DCHECK(bytes_read >= 0);
-  if (state_ == State::DONE)
+  DCHECK_GE(bytes_read, 0);
+  if (state_ == State::PASS_THROUGH)
     return next_handler_->OnReadCompleted(bytes_read, defer);
 
-  DCHECK_EQ(State::WAITING_FOR_BUFFER_COPY, state_);
-  state_ = State::DONE;
+  DCHECK_EQ(State::STARTING, state_);

[Charlie Harrison, 2016/09/29 22:32:58]

It seems strange that we are STARTING at this point. Does it make sense to have
another book-keeping state after OnResponseStarted finishes?

[yhirano, 2016/10/03 11:50:22]

Makes sense, but I couldn't come up with a good name. What do you think about
WAITING_FOR_ON_READ_COMPLETED? Do you have a good idea?

+  state_ = State::SENDING_BUFFER_TO_NEW_HANDLER;
+  first_read_buffer_bytes_read_ = bytes_read;
 
-  // Copy the data from the first read to the new ResourceHandler.
-  scoped_refptr<net::IOBuffer> buf;
-  int buf_len = 0;
-  if (!next_handler_->OnWillRead(&buf, &buf_len, bytes_read))
-    return false;
+  return SendFirstReadBufferToNewHandler(defer);
+}
 
-  CHECK(buf_len >= bytes_read);
-  CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read));
-  memcpy(buf->data(), first_read_buffer_copy_->data(), bytes_read);
+void InterceptingResourceHandler::OnResponseCompleted(
+    const net::URLRequestStatus& status,
+    bool* defer) {
+  switch (state_) {
+    case State::PASS_THROUGH:
+      LayeredResourceHandler::OnResponseCompleted(status, defer);
+      return;
+    case State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER:
+    case State::DONE:
+      NOTREACHED();
+      return;
+    default:
+      break;
+  }
 
-  first_read_buffer_copy_ = nullptr;
+  next_handler_->OnResponseCompleted(status, defer);
+  DCHECK(!*defer);
 
-  // TODO(clamy): Add a unit test to check that the deferral value is properly
-  // passed to the caller.
-  return next_handler_->OnReadCompleted(bytes_read, defer);
+  completion_status_ = status;
+  state_ = State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER;
+  NotifyOnResponseCompletedToNewHandler(defer);
 }
 
 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(controller());
+  new_handler_->SetController(this);
   payload_for_old_handler_ = payload_for_old_handler;
 }
 
-void InterceptingResourceHandler::SendPayloadToOldHandler() {
+void InterceptingResourceHandler::Cancel() {
+  controller()->Cancel();
+}
+
+void InterceptingResourceHandler::CancelAndIgnore() {
+  controller()->CancelAndIgnore();
+}
+
+void InterceptingResourceHandler::CancelWithError(int error_code) {
+  controller()->CancelWithError(error_code);
+}
+
+bool InterceptingResourceHandler::SendFirstReadBufferToNewHandler(bool* defer) {
+  DCHECK_EQ(state_, State::SENDING_BUFFER_TO_NEW_HANDLER);
+

[Charlie Harrison, 2016/09/29 22:32:58]

DCHECK_EQ(first_read_buffer_bytes_written_, 0)?

[yhirano, 2016/10/03 11:50:22]

No, it can be greater than zero when called from Resume.

+  while (first_read_buffer_bytes_written_ < first_read_buffer_bytes_read_) {
+    scoped_refptr<net::IOBuffer> buf;
+    int size = 0;
+    if (!new_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_->data() + first_read_buffer_bytes_written_, size);
+    if (!new_handler_->OnReadCompleted(size, defer))
+      return false;
+    first_read_buffer_bytes_written_ += size;
+    if (*defer)
+      return true;
+  }
+  state_ = State::SENDING_PAYLOAD_TO_OLD_HANDLER;
+  return SendPayloadToOldHandler(defer);
+}
+
+bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) {

[Charlie Harrison, 2016/09/29 22:32:58]

Optional: Should this be a MaybeSendPayloadToOldHandler, because
payload_for_old_handler_ could be empty?

[yhirano, 2016/10/03 11:50:22]

Hmm, I don't think we should append Maybe (weak opinion), but if we do so, we
should do the same thing for SendFirstReadBufferToNewHandler (strong opinion).

+  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_) {

[Charlie Harrison, 2016/09/29 22:32:58]

Can this first condition be pulled out of the loop? It seems like it will always
be true on first iteration? If that ends up being too ugly a
DCHECK(first_read_buffer_) outside of the loop would be good.

[yhirano, 2016/10/03 11:50:22]

|first_read_buffer_| is null at the function top when called from Resume.

+      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;
+  }
   bool defer_ignored = false;
+  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.
-    net::URLRequestStatus status(net::URLRequestStatus::CANCELED,
-                                 net::ERR_ABORTED);
-    next_handler_->OnResponseCompleted(status, &defer_ignored);
-    DCHECK(!defer_ignored);
+    status = net::URLRequestStatus(net::URLRequestStatus::CANCELED,
+                                   net::ERR_ABORTED);
+  }
+  next_handler_->OnResponseCompleted(status, &defer_ignored);
+  DCHECK(!defer_ignored);
+
+  state_ = State::PASS_THROUGH;
+  next_handler_ = std::move(new_handler_);
+  next_handler_->SetController(controller());
+  return true;
+}
+
+void InterceptingResourceHandler::Resume() {
+  bool defer = false;
+  switch (state_) {
+    case State::STARTING:
+      // Uninteresting Resume: just delegate to the original resource
+      // controller.
+      controller()->Resume();
+      return;
+    case State::PASS_THROUGH:
+    case State::DONE:
+      NOTREACHED();
+      break;
+    case State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER:
+      state_ = State::STARTING;
+      break;
+    case State::SENDING_BUFFER_TO_NEW_HANDLER:
+      if (!SendFirstReadBufferToNewHandler(&defer)) {
+        controller()->Cancel();
+        return;
+      }
+      break;
+    case State::SENDING_PAYLOAD_TO_OLD_HANDLER:
+      if (!SendPayloadToOldHandler(&defer)) {
+        controller()->Cancel();
+        return;
+      }
+      break;
+    case State::NOTIFYING_ON_RESPONSE_COMPLETED_TO_NEW_HANDLER:
+      state_ = State::DONE;
+      new_handler_ = nullptr;
+      break;
+  }
+  if (state_ == State::PASS_THROUGH || state_ == State::STARTING ||
+      state_ == State::DONE) {
+    DCHECK(!defer);
+    controller()->Resume();
+  } else {
+    DCHECK(defer);
+  }
+}
+
+void InterceptingResourceHandler::NotifyOnResponseCompletedToNewHandler(
+    bool* defer) {
+  if (!new_handler_) {
+    state_ = State::DONE;
     return;
   }
+  new_handler_->OnResponseCompleted(completion_status_, defer);
+  if (*defer)
+    return;
 
-  // Ensure the old ResourceHandler has a buffer that can store the payload.
-  scoped_refptr<net::IOBuffer> buf;
-  int size = 0;
-  if (first_read_buffer_) {
-    // The first read buffer can be reused. The data inside it has been copied
-    // before calling this function, so it can safely be overriden.
-    buf = first_read_buffer_;
-    size = first_read_buffer_size_;
-  }
-
-  // If there is no first read buffer, ask the old ResourceHandler to create a
-  // buffer that can contain payload.
-  if (!buf)
-    next_handler_->OnWillRead(&buf, &size, -1);
-
-  DCHECK(buf);
-  CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length()));
-  memcpy(buf->data(), payload_for_old_handler_.c_str(),
-         payload_for_old_handler_.length());
-  next_handler_->OnReadCompleted(payload_for_old_handler_.length(),
-                                 &defer_ignored);
-  payload_for_old_handler_ = std::string();
-  DCHECK(!defer_ignored);
-
-  // Finalize the request.
-  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0);
-  next_handler_->OnResponseCompleted(status, &defer_ignored);
-  DCHECK(!defer_ignored);
+  new_handler_ = nullptr;
+  state_ = State::DONE;
 }
 
 }  // namespace content