content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_driver_unittest.cc

Index: content/browser/loader/async_revalidation_driver_unittest.cc
diff --git a/content/browser/loader/async_revalidation_driver_unittest.cc b/content/browser/loader/async_revalidation_driver_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fd68bc2180ebf7b6340c5d13f7d297bf6fcf313d
--- /dev/null
+++ b/content/browser/loader/async_revalidation_driver_unittest.cc
@@ -0,0 +1,399 @@
+// Copyright 2015 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/async_revalidation_driver.h"
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "ipc/ipc_message.h"
+#include "net/base/net_errors.h"
+#include "net/base/request_priority.h"
+#include "net/ssl/ssl_cert_request_info.h"
+#include "net/url_request/url_request_job_factory.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+namespace {
+
+// A mock URLRequestJob which simulates an SSL client auth request.
+class MockClientCertURLRequestJob : public net::URLRequestTestJob {
+ public:
+  MockClientCertURLRequestJob(net::URLRequest* request,
+                              net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  static std::vector<std::string> test_authorities() {

[davidben, 2015/11/23 23:40:41]

You needn't bother with this field.

[Adam Rice, 2015/11/25 19:39:39]

Removed, thanks.

+    return std::vector<std::string>(1, "dummy");
+  }
+
+  // net::URLRequestTestJob implementation:
+  void Start() override {
+    scoped_refptr<net::SSLCertRequestInfo> cert_request_info(
+        new net::SSLCertRequestInfo);
+    cert_request_info->cert_authorities = test_authorities();
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&MockClientCertURLRequestJob::NotifyCertificateRequested,
+                   this, cert_request_info));

[davidben, 2015/11/23 23:40:42]

This is really obnoxious (we need to unrefcount this thing), but this needs to
be WeakPtr'd and you need to override Kill() to InvalidateWeakPtrs(). See
https://codereview.chromium.org/1395643003

[Adam Rice, 2015/11/25 19:39:39]

I now have a hierarchy with three layers of WeakPtrFactorys. I feel weak.

+  }
+
+  void ContinueWithCertificate(net::X509Certificate* cert) override {
+    ADD_FAILURE() << "Certificate supplied.";
+  }
+
+ private:
+  ~MockClientCertURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob);
+};
+
+class MockClientCertJobProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  // URLRequestJobFactory::ProtocolHandler implementation:
+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new MockClientCertURLRequestJob(request, network_delegate);
+  }
+};
+
+// A mock URLRequestJob which simulates an SSL certificate error.
+class MockSSLErrorURLRequestJob : public net::URLRequestTestJob {
+ public:
+  MockSSLErrorURLRequestJob(net::URLRequest* request,
+                            net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  // net::URLRequestTestJob implementation:
+  void Start() override {
+    // This SSLInfo isn't really valid, but it is good enough for testing.
+    net::SSLInfo ssl_info;
+    ssl_info.SetCertError(net::ERR_CERT_DATE_INVALID);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&MockSSLErrorURLRequestJob::NotifySSLCertificateError, this,
+                   ssl_info, false));
+  }
+
+  void ContinueDespiteLastError() override {
+    ADD_FAILURE() << "ContinueDespiteLastError called.";
+  }
+
+ private:
+  ~MockSSLErrorURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(MockSSLErrorURLRequestJob);
+};
+
+class MockSSLErrorJobProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  // URLRequestJobFactory::ProtocolHandler implementation:
+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new MockSSLErrorURLRequestJob(request, network_delegate);
+  }
+};
+
+// Dummy implementation of ResourceThrottle, an instance of which is needed to
+// initialize AsyncRevalidationDriver.
+class ResourceThrottleStub : public ResourceThrottle {
+ public:
+  ResourceThrottleStub() {}
+
+  // If true, defers the request in WillStartRequest.
+  void set_defer_request_on_will_start_request(
+      bool defer_request_on_will_start_request) {
+    defer_request_on_will_start_request_ = defer_request_on_will_start_request;
+  }
+
+  // If true, defers the request in WillStartUsingNetwork.
+  void set_defer_request_on_will_start_using_network(

[davidben, 2015/11/23 23:40:41]

Unused

[Adam Rice, 2015/11/25 19:39:39]

Removed, thanks.

+      bool defer_request_on_will_start_using_network) {
+    defer_request_on_will_start_using_network_ =
+        defer_request_on_will_start_using_network;
+  }
+
+  // If true, defers the request in WillProcessResponse.
+  void set_defer_request_on_will_process_response(

[davidben, 2015/11/23 23:40:41]

Unused

[Adam Rice, 2015/11/25 19:39:39]

Removed.

+      bool defer_request_on_will_process_response) {
+    defer_request_on_will_process_response_ =
+        defer_request_on_will_process_response;
+  }
+
+  bool will_redirect_request_called() const {

[davidben, 2015/11/23 23:40:41]

Unused

[Adam Rice, 2015/11/25 19:39:39]

Removed.

+    return will_redirect_request_called_;
+  }
+
+  // ResourceThrottler implementation:
+  void WillStartRequest(bool* defer) override {
+    *defer = defer_request_on_will_start_request_;
+  }
+
+  void WillStartUsingNetwork(bool* defer) override {
+    *defer = defer_request_on_will_start_using_network_;
+  }
+
+  void WillRedirectRequest(const net::RedirectInfo& redirect_info,
+                           bool* defer) override {
+    will_redirect_request_called_ = true;
+  }
+
+  void WillProcessResponse(bool* defer) override {
+    *defer = defer_request_on_will_process_response_;
+  }
+
+  const char* GetNameForLogging() const override {
+    return "ResourceThrottleStub";
+  }
+
+ private:
+  bool defer_request_on_will_start_request_ = false;
+  bool defer_request_on_will_start_using_network_ = false;
+  bool will_redirect_request_called_ = false;
+  bool defer_request_on_will_process_response_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(ResourceThrottleStub);
+};
+
+}  // namespace
+
+class AsyncRevalidationDriverTest : public testing::Test {
+ protected:
+  AsyncRevalidationDriverTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        raw_ptr_resource_throttle_(nullptr),
+        raw_ptr_request_(nullptr) {
+    test_url_request_context_.set_job_factory(&job_factory_);
+  }
+
+  GURL test_url() const { return net::URLRequestTestJob::test_url_1(); }
+
+  std::string test_data() const {
+    return net::URLRequestTestJob::test_data_1();
+  }
+
+  bool async_revalidation_complete_called() const {
+    return async_revalidation_complete_called_;
+  }
+
+  virtual scoped_ptr<net::URLRequestJobFactory::ProtocolHandler>
+  CreateProtocolHandler() {
+    return net::URLRequestTestJob::CreateProtocolHandler();
+  }
+
+  void SetUpAsyncRevalidationDriverWithRequest(
+      scoped_ptr<net::URLRequest> request) {
+    raw_ptr_request_ = request.get();
+    raw_ptr_resource_throttle_ = new ResourceThrottleStub();
+    // This use of base::Unretained() is safe because |driver_|, and the closure
+    // passed to it, will be destroyed before this object is.
+    driver_.reset(new AsyncRevalidationDriver(
+        std::move(request), make_scoped_ptr(raw_ptr_resource_throttle_),
+        base::Bind(&AsyncRevalidationDriverTest::OnAsyncRevalidationComplete,
+                   base::Unretained(this))));
+  }
+
+  void SetUp() override {
+    job_factory_.SetProtocolHandler("test", CreateProtocolHandler());
+
+    scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+        test_url(), net::DEFAULT_PRIORITY, nullptr /* delegate */));
+    SetUpAsyncRevalidationDriverWithRequest(std::move(request));
+  }
+
+  void OnAsyncRevalidationComplete() {
+    EXPECT_FALSE(async_revalidation_complete_called_);
+    async_revalidation_complete_called_ = true;
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  net::URLRequestJobFactoryImpl job_factory_;
+  net::TestURLRequestContext test_url_request_context_;
+
+  // The AsyncRevalidationDriver owns the URLRequest and the ResourceThrottle.
+  ResourceThrottleStub* raw_ptr_resource_throttle_;
+  net::URLRequest* raw_ptr_request_;
+  scoped_ptr<AsyncRevalidationDriver> driver_;
+  bool async_revalidation_complete_called_ = false;
+};
+
+TEST_F(AsyncRevalidationDriverTest, NormalRequestCompletes) {
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+class AsyncRevalidationDriverClientCertTest
+    : public AsyncRevalidationDriverTest {
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> CreateProtocolHandler()
+      override {
+    return make_scoped_ptr(new MockClientCertJobProtocolHandler());
+  }
+};
+
+// Verifies that async revalidation requests do not attempt to provide client
+// certificates.
+TEST_F(AsyncRevalidationDriverClientCertTest, RequestRejected) {
+  scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+      test_url(), net::LOW, nullptr /* delegate */));
+
+  SetUpAsyncRevalidationDriverWithRequest(std::move(request));
+
+  // Start the request and wait for it to pause.
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that SelectClientCertificate wasn't called and the request aborted.

[davidben, 2015/11/23 23:40:41]

Huh? You aren't checking that SelectClientCertificate wasn't called...

[Adam Rice, 2015/11/25 19:39:39]

Good point. Fixed.

+  const net::URLRequestStatus& status = raw_ptr_request_->status();
+  EXPECT_FALSE(status.is_success());
+  EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, status.error());
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+class AsyncRevalidationDriverSSLErrorTest : public AsyncRevalidationDriverTest {
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> CreateProtocolHandler()
+      override {
+    return make_scoped_ptr(new MockSSLErrorJobProtocolHandler());
+  }
+};
+
+// Verifies that async revalidation requests do not attempt to recover from SSL
+// certificate errors.
+TEST_F(AsyncRevalidationDriverSSLErrorTest, RequestWithSSLErrorRejected) {
+  scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+      test_url(), net::LOW, nullptr /* delegate */));
+
+  SetUpAsyncRevalidationDriverWithRequest(std::move(request));
+
+  // Start the request and wait for it to pause.
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the request has been aborted.
+  const net::URLRequestStatus& status = raw_ptr_request_->status();
+  EXPECT_FALSE(status.is_success());
+  EXPECT_EQ(net::ERR_ABORTED, status.error());
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+// Verifies that resuming a cancelled request does not start it again.
+TEST_F(AsyncRevalidationDriverTest, ResumeCancelledRequest) {
+  raw_ptr_resource_throttle_->set_defer_request_on_will_start_request(true);
+
+  driver_->StartRequest();
+  driver_->CancelRequest();
+  ResourceController* driver_as_resource_controller = driver_.get();
+  driver_as_resource_controller->Resume();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(async_revalidation_complete_called());
+  EXPECT_FALSE(raw_ptr_request_->status().is_success());
+}
+
+// Verify that a cancelled request calls |completion_callback|.
+TEST_F(AsyncRevalidationDriverTest, CancelledRequestCallsCompleteCallback) {
+  driver_->StartRequest();
+  driver_->CancelRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+// Verifies that request that should be deferred at start is deferred.
+TEST_F(AsyncRevalidationDriverTest, DeferOnStart) {

[davidben, 2015/11/23 23:40:41]

I don't think you ever test that resuming a revalidation works.

[Adam Rice, 2015/11/25 19:39:39]

You're right. I have added one. And it found a bug. I wasn't called
LogUnblocked() on the request.

+  raw_ptr_resource_throttle_->set_defer_request_on_will_start_request(true);
+
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(raw_ptr_request_->is_pending());
+  EXPECT_FALSE(async_revalidation_complete_called());
+}
+
+// Verifies that redirects are not followed.
+TEST_F(AsyncRevalidationDriverTest, RedirectsAreNotFollowed) {
+  scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+      net::URLRequestTestJob::test_url_redirect_to_url_2(),
+      net::DEFAULT_PRIORITY, nullptr /* delegate */));
+  SetUpAsyncRevalidationDriverWithRequest(std::move(request));
+
+  driver_->StartRequest();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage())
+    base::RunLoop().RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+  const net::URLRequestStatus& status = raw_ptr_request_->status();
+  EXPECT_FALSE(status.is_success());
+  EXPECT_EQ(net::ERR_ABORTED, status.error());
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+// A URLRequestTestJob that sets |request_time| and |was_cached| on their
+// response_info, and causes the test to fail if Read() is called.
+class FromCacheURLRequestJob : public net::URLRequestTestJob {
+ public:
+  FromCacheURLRequestJob(net::URLRequest* request,
+                         net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  void GetResponseInfo(net::HttpResponseInfo* info) override {
+    URLRequestTestJob::GetResponseInfo(info);
+    info->request_time = base::Time::Now();
+    info->was_cached = true;
+  }
+
+  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override {
+    ADD_FAILURE() << "ReadRawData() was called.";
+    return URLRequestTestJob::ReadRawData(buf, buf_size, bytes_read);
+  }
+
+ private:
+  ~FromCacheURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(FromCacheURLRequestJob);
+};
+
+class FromCacheProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {

[davidben, 2015/11/23 23:40:41]

This isn't in the anonymous namespace. Though you could also just wrap the
entire file in the anonymous namespace which is probably simpler.

I'd previously thought the convention was:

  namespace foo {
  namespace {

  // fixtures and helpers

  }  // namespace

  // tests

  }  // namespace foo

But apparently other folks on //net prefer to write:

  namespace foo {
  namespace {

  // fixtures and helpers, tests, whatever

  }  // namespace
  }  // namespace foo

So I probably am just misremembering what people told me the convention was. :-)
Anyway, do what you like. FromCacheProtocolHandler probably shouldn't leak out
of this compilation unit. Beyond that, it's probably less obnoxious if you're
allowed to interleave them, so feel free to pick the latter one.

[Adam Rice, 2015/11/25 19:39:39]

Both conventions are used, and both sides claim they are correct. I try to
follow
the style in the surrounding code to avoid the discussion.

However, I do like to follow the "definition close to use" rule when it comes to
tests, so I have put everything in the anonymous namespace like you said.

+ public:
+  // URLRequestJobFactory::ProtocolHandler implementation:
+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new FromCacheURLRequestJob(request, network_delegate);
+  }
+};
+
+class AsyncRevalidationDriverFromCacheTest
+    : public AsyncRevalidationDriverTest {
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> CreateProtocolHandler()
+      override {
+    return make_scoped_ptr(new FromCacheProtocolHandler());
+  }
+};
+
+TEST_F(AsyncRevalidationDriverFromCacheTest,
+       CacheNotReadOnSuccessfulRevalidation) {
+  scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+      test_url(), net::LOW, nullptr /* delegate */));
+
+  SetUpAsyncRevalidationDriverWithRequest(std::move(request));
+
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+}  // namespace content