content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_driver.cc

Index: content/browser/loader/async_revalidation_driver.cc
diff --git a/content/browser/loader/async_revalidation_driver.cc b/content/browser/loader/async_revalidation_driver.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dd994d9d40db094549411c2c83e868b22f4dc032
--- /dev/null
+++ b/content/browser/loader/async_revalidation_driver.cc
@@ -0,0 +1,262 @@
+// 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 <utility>
+
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/metrics/user_metrics_action.h"
+#include "base/single_thread_task_runner.h"

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

Unused?

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

Used. I've rewritten the code to make the use explicit (see the calls to
PostTask).

+#include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "content/public/browser/user_metrics.h"
+#include "net/base/net_errors.h"
+#include "net/cert/cert_status_flags.h"

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

Unused?

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

Removed.

+#include "net/ssl/ssl_info.h"

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

Unused?

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

Removed.

+#include "net/url_request/url_request_context.h"

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

Unused?

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

Removed.

+#include "net/url_request/url_request_status.h"
+
+namespace content {
+
+namespace {
+// This matches the maximum allocation size of AsyncResourceHandler.
+const int kReadBufSize = 32 * 1024;
+
+// This value should not be too large, as this request may be tying up a socket
+// that could be used for something better. However, if it is too small, the
+// cache entry will be truncated for no good reason.

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

I don't believe this comment is right. If we time out before the server has even
sent a response (or before the ResourceScheduler even schedules us, or before
the cache entry is even open), the cache entry will not be truncated.

(See also why none of these provisions for when we're "allowed to not
revalidate" are meaningless. The fallback must always be the limited cache reuse
flag.)

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

This value isn't used for the response timeout. In fact, I realised that I have
no timeout implemented at all for the period between when the connect succeeds
and the response headers are received. This is pretty bad even for an
experiment.
So I added one using as little extra code as possible.

+// TODO(ricea): Find a more scientific way to set this timeout.
+const int kReadTimeoutSeconds = 30;
+}
+
+// The use of base::Unretained() in the initialisation of read_timer_ is safe
+// because base::Timer guarantees not to call the callback after being
+// destroyed.
+AsyncRevalidationDriver::AsyncRevalidationDriver(
+    scoped_ptr<net::URLRequest> request,
+    scoped_ptr<ResourceThrottle> throttle,
+    const base::Closure& completion_callback)
+    : read_timer_(FROM_HERE,
+                  base::TimeDelta::FromSeconds(kReadTimeoutSeconds),
+                  base::Bind(&AsyncRevalidationDriver::OnReadTimeout,
+                             base::Unretained(this)),
+                  false),
+      request_(std::move(request)),
+      throttle_(std::move(throttle)),
+      completion_callback_(completion_callback),
+      weak_ptr_factory_(this) {
+  request_->set_delegate(this);
+  throttle_->set_controller(this);
+}
+
+AsyncRevalidationDriver::~AsyncRevalidationDriver() {

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

But for releasing the completion callback, this is already what the destruction
order will do.

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

Removed.

+  weak_ptr_factory_.InvalidateWeakPtrs();
+  // Run ResourceThrottle destructor before we tear-down the rest of our state
+  // as the ResourceThrottle may want to inspect the URLRequest and other state.
+  throttle_.reset();
+}
+
+void AsyncRevalidationDriver::StartRequest() {
+  RecordAction(base::UserMetricsAction("AsyncRevalidationCreated"));
+  // Give the handler a chance to delay the URLRequest from being started.
+  bool defer_start = false;
+  throttle_->WillStartRequest(&defer_start);
+
+  if (defer_start) {
+    RecordDefer();
+  } else {
+    StartRequestInternal();
+  }
+}
+
+void AsyncRevalidationDriver::CancelRequest() {
+  CancelRequestInternal(net::ERR_ABORTED);
+}
+
+void AsyncRevalidationDriver::OnReceivedRedirect(
+    net::URLRequest* unused,

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

Nit: Match the header file's variable names.

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

Done.

+    const net::RedirectInfo& redirect_info,
+    bool* defer) {
+  DCHECK_EQ(request_.get(), unused);
+
+  // The async revalidation should not follow redirects, because caching is
+  // a property of an individual HTTP resource.
+  DVLOG(1) << "OnReceivedRedirect: " << request_->url().spec();
+  RecordAction(base::UserMetricsAction("AsyncRevalidationRedirected"));
+  CancelRequest();
+}
+
+void AsyncRevalidationDriver::OnAuthRequired(
+    net::URLRequest* unused,
+    net::AuthChallengeInfo* auth_info) {
+  DCHECK_EQ(request_.get(), unused);
+  // This error code doesn't have exactly the right semantics, but it should
+  // be sufficient to narrow down the problem in net logs.
+  request_->CancelWithError(net::ERR_ACCESS_DENIED);
+}
+
+void AsyncRevalidationDriver::OnBeforeNetworkStart(net::URLRequest* unused,
+                                                   bool* defer) {
+  DCHECK_EQ(request_.get(), unused);
+
+  // Verify that the ResourceScheduler does not defer here.
+  throttle_->WillStartUsingNetwork(defer);
+  DCHECK(!*defer);
+}
+
+void AsyncRevalidationDriver::OnResponseStarted(net::URLRequest* unused) {
+  DCHECK_EQ(request_.get(), unused);
+
+  DVLOG(1) << "OnResponseStarted: " << request_->url().spec();
+
+  if (!request_->status().is_success()) {
+    ResponseCompleted();
+    return;
+  }
+
+  const net::HttpResponseInfo& response_info = request_->response_info();
+  if (!response_info.response_time.is_null() && response_info.was_cached) {

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

What is the response_time check for?

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

From the comment on the was_cached member in http_response_info.h: "The
following is only defined if the request_time member is set.". Maybe this
comment is intended as a simple statement of fact, but I interpreted it as an
interface contract.

+    // The cached entry was revalidated. No need to read it in.
+    ResponseCompleted();
+    return;
+  }
+
+  bool defer = false;
+  throttle_->WillProcessResponse(&defer);
+  DCHECK(!defer);
+
+  if (request_->status().is_success()) {

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

You've already checked this. The ResourceLoader code has to check again because
the request may have been cancelled, but you don't even implemented the Cancel
hooks.

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

Okay, that makes sense. Removed.

+    StartReading(false);  // Read the first chunk.
+  } else {
+    ResponseCompleted();
+  }
+}
+
+void AsyncRevalidationDriver::OnReadCompleted(net::URLRequest* unused,
+                                              int bytes_read) {
+  DCHECK_EQ(request_.get(), unused);
+  DCHECK(!is_deferred_);
+  DVLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
+           << " bytes_read = " << bytes_read;
+
+  // bytes_read == -1 is an error.
+  // bytes_read == 0 is EOF.
+  if (bytes_read == -1 || bytes_read == 0 || !request_->status().is_success()) {
+    ResponseCompleted();
+    return;
+  }
+
+  DCHECK_GT(bytes_read, 0);
+  StartReading(true);  // Read the next chunk.
+}
+
+void AsyncRevalidationDriver::Resume() {
+  DCHECK(is_deferred_);
+  is_deferred_ = false;
+  StartRequestInternal();
+}
+
+void AsyncRevalidationDriver::Cancel() {
+  NOTREACHED();
+}
+
+void AsyncRevalidationDriver::CancelAndIgnore() {
+  NOTREACHED();
+}
+
+void AsyncRevalidationDriver::CancelWithError(int error_code) {
+  NOTREACHED();
+}
+
+void AsyncRevalidationDriver::StartRequestInternal() {
+  DCHECK(!request_->is_pending());
+
+  // This can happen if Resume() is called after CancelRequest().
+  // Since CancelRequest() will have called ResponseCompleted() asynchronously,
+  // it's not necessary to call it again.
+  if (!request_->status().is_success())
+    return;
+
+  request_->Start();
+}
+
+void AsyncRevalidationDriver::CancelRequestInternal(int error) {
+  DVLOG(1) << "CancelRequestInternal: " << request_->url().spec();
+
+  bool was_pending = request_->is_pending();
+
+  request_->CancelWithError(error);
+
+  if (!was_pending) {
+    // If the request isn't in flight, then we won't get an asynchronous
+    // notification from the request, so we have to signal ourselves to finish
+    // this request.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&AsyncRevalidationDriver::ResponseCompleted,
+                              weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void AsyncRevalidationDriver::StartReading(bool is_continuation) {
+  int bytes_read = 0;
+  ReadMore(&bytes_read);
+
+  // If IO is pending, wait for the URLRequest to call OnReadCompleted.
+  if (request_->status().is_io_pending())
+    return;
+
+  if (!is_continuation || bytes_read <= 0) {
+    OnReadCompleted(request_.get(), bytes_read);
+  } else {
+    // Else, trigger OnReadCompleted asynchronously to avoid starving the IO
+    // thread in case the URLRequest can provide data synchronously.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&AsyncRevalidationDriver::OnReadCompleted,
+                   weak_ptr_factory_.GetWeakPtr(), request_.get(), bytes_read));
+  }
+}
+
+void AsyncRevalidationDriver::ReadMore(int* bytes_read) {
+  DCHECK(!is_deferred_);
+
+  if (!read_buffer_)
+    read_buffer_ = new net::IOBuffer(kReadBufSize);
+
+  read_timer_.Reset();
+  request_->Read(read_buffer_.get(), kReadBufSize, bytes_read);
+
+  // No need to check the return value here as we'll detect errors by
+  // inspecting the URLRequest's status.
+}
+
+void AsyncRevalidationDriver::ResponseCompleted() {
+  DVLOG(1) << "ResponseCompleted: " << request_->url().spec();
+  // When this class cancels a redirect, URLRequest calls both the
+  // OnResponseStarted() and OnReadCompleted() callbacks. This class should not
+  // run |completion_callback_| twice.
+  //
+  // TODO(ricea): Work out why URLRequest calls both methods on cancellation and
+  // make it stop.

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

Is this still happening? In the previous version, you missed that cancelling
would signal a failing OnResponseStarted, so denote the end of Start(). If you
were to call Read() after that, I could believe that's why OnReadCompleted got
signaled. Otherwise this doesn't make sense.

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

Yes, still happening. It might just be a feature of URLRequestTestJob. I haven't
checked whether the same thing happens with a real server.

[davidben, 2015/12/07 23:56:03]

And confirmed. Sigh. https://crbug.com/564820. (Mind sticking that in the
comment? I'll do a fix asynchronously.)

Edit: Apparently I'm slow enough at complex reviews that that's already been
fixed.

+  if (completion_callback_.is_null())
+    return;
+  base::Closure completion_callback(completion_callback_);
+  completion_callback_.Reset();
+  completion_callback.Run();

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

base::ResetAndReturn(&completion_callback_).Run();

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

Add:
 // |this| may be deleted after this point.

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

Thank you! I knew that existed but I couldn't remember what it was called.

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

Done.

+}
+
+void AsyncRevalidationDriver::OnReadTimeout() {
+  RecordAction(base::UserMetricsAction("AsyncRevalidationTimeout"));
+  CancelRequestInternal(net::ERR_TIMED_OUT);
+}
+
+void AsyncRevalidationDriver::RecordDefer() {
+  request_->LogBlockedBy(throttle_->GetNameForLogging());
+  DCHECK(!is_deferred_);
+  is_deferred_ = true;
+}
+
+}  // namespace content