Add some tests for ThrottlingResourceHandler.

content/browser/loader/mock_resource_loader.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/mock_resource_loader.h"
+
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "content/browser/loader/resource_controller.h"
+#include "content/browser/loader/resource_handler.h"
+#include "content/public/common/resource_response.h"
+#include "net/base/io_buffer.h"
+#include "net/url_request/url_request_status.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+MockResourceLoader::MockResourceLoader(ResourceHandler* resource_handler)
+    : resource_handler_(resource_handler) {
+  resource_handler_->SetController(this);
+}
+
+MockResourceLoader::~MockResourceLoader() {}
+
+MockResourceLoader::Status MockResourceLoader::OnWillStart(const GURL& url) {
+  EXPECT_EQ(Status::IDLE, status_);
+  status_ = Status::CALLING_HANDLER;
+
+  bool defer = false;
+  bool result = resource_handler_->OnWillStart(url, &defer);
+  // The second case isn't really allowed, but a number of classes do it
+  // anyways.
+  EXPECT_TRUE(status_ == Status::CALLING_HANDLER ||
+              (result == false && status_ == Status::CANCELED));
+  if (!result) {
+    status_ = Status::CANCELED;
+  } else if (defer) {
+    status_ = Status::CALLBACK_PENDING;
+  } else {
+    status_ = Status::IDLE;
+  }
+  return status_;
+}
+
+MockResourceLoader::Status MockResourceLoader::OnRequestRedirected(
+    const net::RedirectInfo& redirect_info,
+    scoped_refptr<ResourceResponse> response) {
+  EXPECT_EQ(Status::IDLE, status_);
+  status_ = Status::CALLING_HANDLER;
+
+  bool defer = false;
+  bool result = resource_handler_->OnRequestRedirected(redirect_info,
+                                                       response.get(), &defer);

[Randy Smith (Not in Mondays), 2017/01/11 19:57:15]

Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn the
leak into a use-after-free?  The response will be deleted when the last
scoped_refptr<> pointing to it goes out of scope, which looks to me to be at the
end of this routine.  If OnRequestRedirected results in a CALLBACK_PENDING
state, doesn't that mean the resource_handler_ may have held onto a pointer to
the response?

(Same for next routine.)

[mmenke, 2017/01/11 20:39:14]

Great question!  This is indeed rather unclear.

I did this to mimic the real ResourceLoader's behavior - see
https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c....
 If ResourceHandlers process the event asynchronously, they should hold onto a
pointer themselves if they want to keep it alive.  I've added comments to both
these methods.

Think I'll modify the other two test CLs I've sent you to use std::move to pass
in the ResourceResponse (Both text fixtures currently hand on to their own
reference to the response when they call this method, currently), with more
comments about it.

[Randy Smith (Not in Mondays), 2017/01/11 21:11:02]

Shouldn't there be a comment to this effect in resource_handler.h before the
routines that take a ResourceResponse?  It's only slightly surprising that the
RR can be deallocated on return from the call (One hand: I think of that as
default for raw pointers, other hand: It's not clear how to think about incoming
arguments when the request is deferred), but it's also slightly surprising that
it's good practice in a resource handler to take a reference can be taken to
keep it around (One hand: it's generally how scoped_refptr<>s work, other hand:
It's generally *not* how raw pointers work, so it isn't clear from the interface
that that's expected.)  A comment to clarify all the surprises would be welcome
(by me, at least).

[mmenke, 2017/01/11 21:16:37]

SGTM.  I'll update the RH interface update CL with a comment with that
information (Since that CL rewrites the interface, seems simpler to do it there,
rather than do it here, merge up to the final CL, merge the comment, and rewrite
the comment).

May make sense to switch the argument to a scoped_refptr<ResourceResponse>, to
make lifetimes clearer, but that CL does enough already.  :)

[Randy Smith (Not in Mondays), 2017/01/11 21:35:53]

FWIW, I'd support the interface change (at some time, in some CL :-}); I think
it makes it very likely that assumptions about lifetime and argument usage will
be correct without needing to read the comment documentation.  

+  // The second case isn't really allowed, but a number of classes do it
+  // anyways.
+  EXPECT_TRUE(status_ == Status::CALLING_HANDLER ||
+              (result == false && status_ == Status::CANCELED));
+  if (!result) {
+    status_ = Status::CANCELED;
+  } else if (defer) {
+    status_ = Status::CALLBACK_PENDING;
+  } else {
+    status_ = Status::IDLE;
+  }
+  return status_;
+}
+
+MockResourceLoader::Status MockResourceLoader::OnResponseStarted(
+    scoped_refptr<ResourceResponse> response) {
+  EXPECT_EQ(Status::IDLE, status_);
+  status_ = Status::CALLING_HANDLER;
+
+  bool defer = false;
+  bool result = resource_handler_->OnResponseStarted(response.get(), &defer);
+  // The second case isn't really allowed, but a number of classes do it
+  // anyways.
+  EXPECT_TRUE(status_ == Status::CALLING_HANDLER ||
+              (result == false && status_ == Status::CANCELED));
+  if (!result) {
+    status_ = Status::CANCELED;
+  } else if (defer) {
+    status_ = Status::CALLBACK_PENDING;
+  } else {
+    status_ = Status::IDLE;
+  }
+  return status_;
+}
+
+MockResourceLoader::Status MockResourceLoader::OnWillRead(int min_size) {
+  EXPECT_EQ(Status::IDLE, status_);
+  status_ = Status::CALLING_HANDLER;
+
+  scoped_refptr<net::IOBuffer> buf;
+  int buf_size;
+  bool result = resource_handler_->OnWillRead(&buf, &buf_size, min_size);
+  // The second case isn't really allowed, but a number of classes do it
+  // anyways.
+  EXPECT_TRUE(status_ == Status::CALLING_HANDLER ||
+              (result == false && status_ == Status::CANCELED));
+  if (!result) {
+    status_ = Status::CANCELED;
+  } else {
+    EXPECT_LE(min_size, buf_size);
+    status_ = Status::IDLE;
+  }
+  return status_;
+};
+
+MockResourceLoader::Status MockResourceLoader::OnReadCompleted(int bytes_read) {
+  EXPECT_EQ(Status::IDLE, status_);
+  status_ = Status::CALLING_HANDLER;
+
+  bool defer = false;
+  bool result = resource_handler_->OnReadCompleted(bytes_read, &defer);
+  // The second case isn't really allowed, but a number of classes do it
+  // anyways.
+  EXPECT_TRUE(status_ == Status::CALLING_HANDLER ||
+              (result == false && status_ == Status::CANCELED));
+  if (!result) {
+    status_ = Status::CANCELED;
+  } else if (defer) {
+    status_ = Status::CALLBACK_PENDING;
+  } else {
+    status_ = Status::IDLE;
+  }
+  return status_;
+}
+
+MockResourceLoader::Status MockResourceLoader::OnResponseCompleted(
+    const net::URLRequestStatus& status) {
+  // This should only happen while the ResourceLoader is idle or the request was
+  // canceled.
+  EXPECT_TRUE(status_ == Status::IDLE ||
+              (!status.is_success() && status_ == Status::CANCELED &&
+               error_code_ == status.error()));
+
+  status_ = Status::CALLING_HANDLER;
+
+  bool defer = false;
+  resource_handler_->OnResponseCompleted(status, &defer);
+  EXPECT_EQ(Status::CALLING_HANDLER, status_);
+  if (defer) {
+    status_ = Status::CALLBACK_PENDING;
+  } else {
+    status_ = Status::IDLE;
+  }
+  return status_;
+}
+
+void MockResourceLoader::Cancel() {
+  CancelWithError(net::ERR_ABORTED);
+}
+
+void MockResourceLoader::CancelAndIgnore() {
+  NOTREACHED();
+}
+
+void MockResourceLoader::CancelWithError(int error_code) {
+  EXPECT_LT(error_code, 0);
+  // Ignore double cancels.  Classes shouldn't be doing this, as
+  // ResourceLoader doesn't really support it, but they do anywways.  :(
+  // TODO(mmenke): Remove this check.
+  if (error_code_ != net::OK)
+    return;
+
+  // ResourceLoader really expects canceled not to be called synchronously, but
+  // a lot of code does it, so allow it.
+  // TODO(mmenke):  Remove CALLING_HANDLER.
+  EXPECT_TRUE(status_ == Status::CALLBACK_PENDING ||
+              status_ == Status::CALLING_HANDLER || status_ == Status::IDLE);
+  status_ = Status::CANCELED;
+  error_code_ = error_code;
+}
+
+void MockResourceLoader::Resume() {
+  EXPECT_EQ(Status::CALLBACK_PENDING, status_);
+  status_ = Status::IDLE;
+}
+
+}  // namespace content