content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_driver_unittest.cc

+// 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 <vector>
+
+#include "base/bind.h"
+#include "base/location.h"

[Bence, 2015/11/17 13:12:22]

What do you need this include for?

[Adam Rice, 2015/11/17 17:45:52]

For FROM_HERE. Comment added.

+#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/io_buffer.h"

[Bence, 2015/11/17 13:12:22]

Please remove this include, because you have already included it in the header
file.

+#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.h"

[Bence, 2015/11/17 13:12:22]

Please remove this include, because you have already included it in the header
file.

[Adam Rice, 2015/11/17 17:45:51]

Thanks, done.

+#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() {
+    return std::vector<std::string>(1, "dummy");
+  }
+
+  // net::URLRequestTestJob:

[Bence, 2015/11/17 13:12:22]

Add " implementation" right before colon.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  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));
+  }
+
+  void ContinueWithCertificate(net::X509Certificate* cert) override {
+    ADD_FAILURE() << "Certificate supplied";

[Bence, 2015/11/17 13:12:22]

Optional: consider adding a period to end this sentence.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  }
+
+ private:
+  ~MockClientCertURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob);

[Bence, 2015/11/17 13:12:22]

This class does not have any members so it should be safe to copy or to assign. 
What are you trying to express here?

[Adam Rice, 2015/11/17 17:45:52]

It's not actually copyable because of the base class. I am just being explicit
about the fact that it's not copyable.

[Bence, 2015/11/17 21:34:27]

Oh okay, that makes sense.  Thank you for clarifying.

+};
+
+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:

[Bence, 2015/11/17 13:12:22]

Add " implementation" right before colon.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  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";

[Bence, 2015/11/17 13:12:22]

Optional: consider adding a full stop to end this sentence.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  }
+
+ private:
+  ~MockSSLErrorURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(MockSSLErrorURLRequestJob);

[Bence, 2015/11/17 13:12:22]

Why?

[Adam Rice, 2015/11/17 17:45:52]

No particular reason, just making it explicit.

[Bence, 2015/11/17 21:34:27]

Sounds good.

+};
+
+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(
+      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(
+      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 {
+    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_;
+  }
+
+  // Returns the name of the throttle, as a UTF-8 C-string, for logging
+  // purposes.  nullptr is not allowed.  Caller does *not* take ownership of the

[Bence, 2015/11/17 13:12:22]

Optional: consider writing |nullptr| to mitigate the ugh-feeling one might get
when reading a non-capitalised first word of a sentence.  (Just to be clear, I
am definitely not asking you to capitalise nullptr.)

[Adam Rice, 2015/11/17 17:45:52]

It appears for some reason I copied this comment verbatim from
resource_throttle.h.
Maybe I should just remove it completely?

[Bence, 2015/11/17 21:34:28]

That would also be fine with me

+  // returned string.
+  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);

[Bence, 2015/11/17 13:12:22]

Copying and assigning this class seems to be safe to me, why bother disallowing?

[Adam Rice, 2015/11/17 17:45:51]

I don't want to provide a guarantee that copying is safe, because a change in
the
base class could make it unsafe. There would also be a risk of slicing if in
future
someone derived from this class.

[Bence, 2015/11/17 21:34:27]

Fair enough.

+};
+
+// This class is a variation on URLRequestTestJob that will call
+// URLRequest::WillStartUsingNetwork before starting.
+class URLRequestTestDelayedNetworkJob : public net::URLRequestTestJob {

[Bence, 2015/11/17 13:12:22]

This class does not seem to be used anywhere.  Please remove it, or better yet,
add the test that you had in mind when crafting this class.

[Adam Rice, 2015/11/17 17:45:52]

Ah, sorry, I failed to remove this. The history is that originally
AsyncRevalidationDriver supported various throttling scenarios that are not
actually used by ResourceScheduler. I realised that was just pointless
complexity so I removed the unused features, including deferral on
WillStartUsingNetwork. But I failed to remove this test support class.

+ public:
+  URLRequestTestDelayedNetworkJob(net::URLRequest* request,
+                                  net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  // Only start if not deferred for network start.
+  void Start() override {
+    bool defer = false;
+    NotifyBeforeNetworkStart(&defer);
+    if (defer)
+      return;
+    net::URLRequestTestJob::Start();

[Bence, 2015/11/17 13:12:22]

Early returns seems a bit contorted to me.  Consider inverting the if condition
and save one line.

[Adam Rice, 2015/11/17 17:45:52]

Acknowledged.

+  }
+
+  void ResumeNetworkStart() override { net::URLRequestTestJob::StartAsync(); }
+
+ private:
+  ~URLRequestTestDelayedNetworkJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(URLRequestTestDelayedNetworkJob);

[Bence, 2015/11/17 13:12:22]

Why?

[Adam Rice, 2015/11/17 17:45:52]

I like to be explicit.

+};
+
+}  // namespace
+
+class AsyncRevalidationDriverTest : public testing::Test {
+ protected:
+  AsyncRevalidationDriverTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        raw_ptr_resource_throttle_(nullptr),
+        raw_ptr_to_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 net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() {
+    return net::URLRequestTestJob::CreateProtocolHandler();
+  }
+
+  void SetUpAsyncRevalidationDriverWithRequest(
+      scoped_ptr<net::URLRequest> request) {
+    raw_ptr_to_request_ = request.get();
+
+    scoped_ptr<ResourceThrottleStub> resource_throttle(

[Bence, 2015/11/17 13:12:22]

Optional: throw away |resource_throttle| local variable and use
make_scoped_ptr(raw_ptr_resource_throttle) on line 237.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+        new ResourceThrottleStub());
+    raw_ptr_resource_throttle_ = resource_throttle.get();
+    // 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(
+        request.Pass(), resource_throttle.Pass(),
+        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(request.Pass());
+  }
+
+  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_to_request_;

[Bence, 2015/11/17 13:12:22]

Please harmonise prefix: raw_ptr_ or raw_ptr_to_ for both.

[Adam Rice, 2015/11/17 17:45:52]

Done.

+  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 {
+  net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() override {
+    return 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(request.Pass());
+
+  // Start the request and wait for it to pause.
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that SelectClientCertificate wasn't called and the request aborted.
+  const net::URLRequestStatus& status = raw_ptr_to_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 {
+  net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() override {
+    return 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(request.Pass());
+
+  // 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_to_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();
+  implicit_cast<ResourceController*>(driver_.get())->Resume();

[Bence, 2015/11/17 13:12:22]

Please do not do implicit_cast, because it is used very-very rarely in the
codebase [1], so developers might not be familiar with it.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=implicit_cast%20-file...

[Adam Rice, 2015/11/17 17:45:52]

Also, since I wrote this I accidentally got implicit_cast removed from //base.

+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(async_revalidation_complete_called());
+  EXPECT_FALSE(raw_ptr_to_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) {
+  raw_ptr_resource_throttle_->set_defer_request_on_will_start_request(true);
+
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(raw_ptr_to_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(request.Pass());
+
+  driver_->StartRequest();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage())
+    base::RunLoop().RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+  const net::URLRequestStatus& status = raw_ptr_to_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);

[Bence, 2015/11/17 13:12:22]

Why bother?

[Adam Rice, 2015/11/17 17:45:52]

It seems like the right thing to do.

[Bence, 2015/11/17 21:34:27]

Okay then.

+};
+
+class FromCacheProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {
+ 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 {
+  net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() override {
+    return new FromCacheProtocolHandler();
+  }
+};
+
+TEST_F(AsyncRevalidationDriverFromCacheTest,
+       CacheNotReadOnSuccessfulRevalidation) {
+  scoped_ptr<net::URLRequest> request(test_url_request_context_.CreateRequest(
+      test_url(), net::LOW, nullptr /* delegate */));
+
+  SetUpAsyncRevalidationDriverWithRequest(request.Pass());
+
+  driver_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(async_revalidation_complete_called());
+}
+
+}  // namespace content