Move MimeTypeResourceHandler before ThrottlingResourceHandler

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
new file mode 100644
index 0000000000000000000000000000000000000000..3a15d94cdc46762a00fcf60e89e767bee7854704
--- /dev/null
+++ b/content/browser/loader/intercepting_resource_handler.cc
@@ -0,0 +1,157 @@
+// 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 <utility>

[mmenke, 2016/08/22 20:39:03]

What's this needed for (Sorry if I already asked this - doesn't look like I did,
though)?

[clamy, 2016/09/01 23:19:40]

It was no longer needed. Removed it.

+
+#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) {}
+
+InterceptingResourceHandler::~InterceptingResourceHandler() {}
+
+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;
+    first_read_buffer_ = nullptr;
+    return next_handler_->OnResponseStarted(response, defer);
+  }
+
+  // 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);
+
+  // 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. This data should be
+  // passed to the alternate ResourceHandler and not to to the current
+  // ResourceHandler.

[mmenke, 2016/08/19 16:33:02]

I think it's worth noting the weird magic here.  "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"...Hrm....That seems too weird.  Could this copying be done by the
MimeSniffingResourceHandler, before it calls into UseNewHandler?  It even knows
the actual amount of the buffer that's been populated.

You'd need to make it call OnWillRead after calling UseNewHandler, but I don't
think that's a problem?  It also maintains the contract that OnReadCompleted
will be called when the last data returned by OnWillRead will have been
populated.  This current approach breaks the contract, for both this class, and
the ThrottlingResourceHandler, which seems really weird.

Sorry for all the big[ish] suggestions, just suspect we'll be stuck with any
ugliness this CL adds for quite a while.

[clamy, 2016/09/01 23:19:40]

I've updated the comment and added a TODO to check if this should be moved to
the MimeSniffingResourceHandler. Considering that this patch passes all tests
and has testing coverage for this behavior, I'd like to land it ASAP to have it
spend some time on trunk before branch point. Moving the copy to the
MimeSniffingResourceHandler is non trivial, especially when it comes to the unit
tests. I'd rather do it as a follow-up CL, so that I can unblock other CLs
waiting on that one.

+  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;
+
+  // This is now handled entirely within the new ResourceHandler, so just reset
+  // the original ResourceHandler.

[mmenke, 2016/08/19 16:33:02]

"This" seems unclear.  Maybe "The original ResourceHandler is now no longer
needed, so replace it with the new one, before sending the response to the new
one."?

[clamy, 2016/09/01 23:19:40]

Done.

+  next_handler_ = std::move(new_handler_);
+
+  state_ =
+      first_read_buffer_copy_ ? State::WAITING_FOR_BUFFER_COPY : State::DONE;
+
+  return next_handler_->OnResponseStarted(response, defer);
+}
+
+bool InterceptingResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
+                                             int* buf_size,
+                                             int min_size) {
+  if (state_ == State::DONE)
+    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) {
+  if (state_ == State::DONE)
+    return next_handler_->OnReadCompleted(bytes_read, defer);
+
+  DCHECK_EQ(State::WAITING_FOR_BUFFER_COPY, state_);
+  state_ = State::DONE;
+
+  // 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;
+
+  CHECK((buf_len >= bytes_read) && (bytes_read >= 0));

[mmenke, 2016/08/22 20:39:04]

Split this in two - makes it clearer which one failed.

[clamy, 2016/09/01 23:19:40]

Done.

+  CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read));

[mmenke, 2016/08/22 20:39:04]

CHECK -> DCHECK (x2)

[clamy, 2016/09/01 23:19:40]

Considering we're about to write memory in the memcpy below, it seems safer from
a security POV to keep a CHECK here in order to crash rather than risk a buffer
overflow. Wdyt?

+  memcpy(buf->data(), first_read_buffer_copy_->data(), bytes_read);
+
+  first_read_buffer_copy_ = nullptr;
+
+  return next_handler_->OnReadCompleted(bytes_read, defer);

[mmenke, 2016/08/22 20:39:03]

No unit tests make sure we pass through defer here...I think we're probably safe
ignoring that case?

[clamy, 2016/09/01 23:19:40]

Added a TODO.

+}
+
+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());
+  payload_for_old_handler_ = payload_for_old_handler;
+}
+
+void InterceptingResourceHandler::SendPayloadToOldHandler() {
+  bool defer_ignored = false;
+  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, std::string(), &defer_ignored);
+    DCHECK(!defer_ignored);
+    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);
+
+  CHECK(buf);
+  CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length()));

[mmenke, 2016/08/22 20:39:04]

CHECK->DCHECK (x2)

[clamy, 2016/09/01 23:19:40]

Same comment as before. I left these as CHECKs because it checks for the
validity of the memcpy.

+  memcpy(buf->data(), payload_for_old_handler_.c_str(),
+         payload_for_old_handler_.length());
+  next_handler_->OnReadCompleted(payload_for_old_handler_.length(),

[mmenke, 2016/08/22 20:39:03]

I assume payload_for_old_handler_ is short enough that keeping it around isn't a
problem?  (Worth noting that payload_for_old_handler_.clear() wouldn't actually
free the memory it uses)

[clamy, 2016/09/01 23:19:40]

I now reset it to an empty string.

+                                 &defer_ignored);
+  DCHECK(!defer_ignored);
+
+  // Finalize the request.
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0);
+  next_handler_->OnResponseCompleted(status, std::string(), &defer_ignored);
+  DCHECK(!defer_ignored);
+}
+
+}  // namespace content