Relax ad-hoc assumptions on InterceptingResourceHandler

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 08907205397cc3fd52b3e9797f5094ab6ae2d0e2..2147fc61841144fbd70e3f3c3c685c0b4b31eaed 100644
--- a/content/browser/loader/intercepting_resource_handler.cc
+++ b/content/browser/loader/intercepting_resource_handler.cc
@@ -14,65 +14,54 @@ 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_);
-  }
-
-  // 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;
+  if (!new_handler_->OnResponseStarted(response, defer))
+    return false;
 
-  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_);
@@ -87,77 +76,178 @@ bool InterceptingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
 }
 
 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_);
+  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) {

[mmenke, 2016/10/04 15:08:01]

Why are you reversing the order of this and sending the payload to the new
handler?  To save the memcpy?

Per https://crbug.com/650717, we'll run into issues a little down the line (When
adding a call to OnWillStartRequest on the new handler) if we do this before we
pass the payload to the old handler, so I think we're stuck with the copy.

[yhirano, 2016/10/06 11:41:44]

Yes, I reversed order in order to remove a copy.

Sorry I still don't understand why this is problematic. Can you explain again?

[mmenke, 2016/10/06 16:10:18]

OnWillStart isn't being called on NewHandler, but should be.  I added it before
the OnReadCompleted call to both handlers, using the old code (Which, as far as
DownloadResourceHandler, is much the same as reversing the order of the read
calls).

DownloadTest.DownloadResourceThrottleCancels expects the opposite order, so
started failing, not sure why it's passing on some platforms when you ran the
tests.  I think the change would also mean we spin the spinner while showing the
loading dialog, unsure about other fallout here - could the old request get a
cancellation message while we're calling into the new handler?  I was thinking I
ran into some other fallout as well, but can't remember what it was.

[yhirano, 2016/10/07 15:03:53]

Thank you. I changed the state transition to keep the existing behavior as much
as possible.

[mmenke, 2016/10/07 15:48:42]

Sorry, "loading dialog" == "save as dialog".

+  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 (!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) {

[mmenke, 2016/10/04 15:08:01]

Would more of a DoLoop approach be simpler?

i.e., make a method:

bool DoLoop(bool* defer) {
  bool result = true;
  while (result && !defer && state_ != State::WHATEVER) {
    // Switch statement much like Resume() has.
  }
  return result;
}

Then call SendFirstReadBufferToNewHandler (Which no longer has a while loop, or
a call to SendPayloadToOldHandler - it just sets the state_ to handle whether it
should be called again, or the next method should be called).

Resume then calls that method as well...  Anyhow, just a suggestion.

[yhirano, 2016/10/06 11:41:44]

Done.

+  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_) {
+      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);
-    return;
+    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;
+}
 
-  // 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_;
+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:

[mmenke, 2016/10/04 15:08:01]

I don't think we ever set this state in the first place?

[yhirano, 2016/10/06 11:41:44]

I set |state_| to the state in InterceptingResourceHandler::OnResponseCompleted.

+      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);
+  }
+}
 
-  // 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);
+void InterceptingResourceHandler::NotifyOnResponseCompletedToNewHandler(
+    bool* defer) {
+  if (!new_handler_) {
+    state_ = State::DONE;
+    return;
+  }
+  new_handler_->OnResponseCompleted(completion_status_, defer);
+  if (*defer)
+    return;
 
-  // 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