content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_driver.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 "base/location.h"  // For FROM_HERE

[kinuko, 2015/11/19 07:00:28]

nit: I see you added these comments per review feedback, while I feel this is
not very common and could be probably omitted.

[Adam Rice, 2015/11/19 21:13:52]

Done.

[Bence, 2015/11/20 13:18:35]

I genuinely couldn't find what these headers were there for, so I meant to
simply ask.  I did not mean to imply that it should be clarified in a comment. 
I'm very sorry for my ambiguous feedback.

[Adam Rice, 2015/11/20 13:23:51]

Absolutely no problem. I follow the philosophy that all feedback is an
opportunity
to improve my code. Sometimes this works out better than others.

+#include "base/logging.h"
+#include "base/metrics/user_metrics_action.h"
+#include "base/single_thread_task_runner.h"  // Gets a SingleThreadTaskRunner
+#include "base/thread_task_runner_handle.h"  // from ThreadTaskRunnerHandle.

[kinuko, 2015/11/19 07:00:28]

These seem obvious from header names.

[Adam Rice, 2015/11/19 21:13:52]

What's not obvious is that the SingleThreadTaskRunner include is required to
call PostTask on the return value from ThreadTaskRunnerHandle::Get(). I don't
know how to fix that without adding a lot of verbosity.

For now, I have removed these comments since they don't seem to help.

[Bence, 2015/11/20 13:18:35]

Ditto.

+#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"
+#include "net/ssl/ssl_info.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_status.h"
+
+namespace content {
+
+namespace {
+// This matches the maximum allocation size of AsyncResourceHandler.
+const int kReadBufSize = 32 * 1024;
+const int kReadTimeoutSeconds = 5 * 60;

[kinuko, 2015/11/19 08:38:34]

How is this timeout value chosen?

[Adam Rice, 2015/11/19 21:13:52]

I don't remember. The most likely place is kDefaultDetachableCancelDelayMs, but
that would mean I was off by a factor of 10. Also,
kDefaultDetachableCancelDelayMs is the total time for the request after it is
detached, whereas this is the time between reads, so it is not directly
comparable.

I have changed it to 30 seconds. Even on a really slow connection, if you
receive nothing for 30 seconds it is probably time to give up.

+}
+
+// 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_(request.Pass()),
+      throttle_(throttle.Pass()),

[kinuko, 2015/11/19 07:00:28]

nit: you could now use std::move for these

[Adam Rice, 2015/11/19 21:13:52]

Done.

+      completion_callback_(completion_callback),
+      weak_ptr_factory_(this) {
+  request_->set_delegate(this);
+  throttle_->set_controller(this);
+}
+
+AsyncRevalidationDriver::~AsyncRevalidationDriver() {
+  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,
+    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);

[kinuko, 2015/11/19 07:00:28]

Help me understand: for this case we don't need/want to call CancelRequest() ?

[Adam Rice, 2015/11/19 21:13:52]

CancelRequest() would use ERR_ABORTED, which is very generic and makes life more
difficult for people trying to track down the cause of fluctuations in the
ERR_ABORTED rate.

Also, CancelRequestInternal() does some checking whether the request is pending
which is completely unneeded in this case (we know it's pending).

Can you think of anything I could do to make this clearer to someone reading the
code?

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

[kinuko, 2015/11/19 07:00:29]

!*defer ?

[Adam Rice, 2015/11/19 21:13:52]

Oh. The tests in https://codereview.chromium.org/1308203003 would have caught
this, but I haven't rebased them in 2 months. I probably should.

+}
+
+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) {
+    // 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()) {
+    StartReading(false);  // Read the first chunk.
+  } else {
+    ResponseCompleted();
+  }
+}
+
+void AsyncRevalidationDriver::OnReadCompleted(net::URLRequest* unused,
+                                              int bytes_read) {
+  DCHECK_EQ(request_.get(), unused);
+  DVLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
+           << " bytes_read = " << bytes_read;
+
+  // bytes_read == -1 always implies an error.
+  if (bytes_read == -1 || !request_->status().is_success()) {
+    ResponseCompleted();
+    return;
+  }
+
+  DCHECK_GE(bytes_read, 0);
+  DCHECK(request_->status().is_success());
+  DCHECK(!is_deferred_);
+
+  if (bytes_read > 0) {
+    StartReading(true);  // Read the next chunk.
+  } else {
+    // URLRequest reported an EOF.
+    DCHECK_EQ(0, bytes_read);

[kinuko, 2015/11/19 07:00:28]

nit: feels a bit verbose to me

[Adam Rice, 2015/11/19 21:13:52]

I have made it less verbose. PTAL.

+    ResponseCompleted();
+  }
+}
+
+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::CompleteResponseStarted() {

[kinuko, 2015/11/19 07:00:28]

Where is this used?

[Adam Rice, 2015/11/19 21:13:52]

It's not. Thank you for checking. I moved the implementation of this into
OnResponseStarted() but it seems I forgot to delete the method.

+  bool defer = false;
+  throttle_->WillProcessResponse(&defer);
+  DCHECK(!defer);
+}
+
+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.
+  if (completion_callback_.is_null())
+    return;
+  base::Closure completion_callback(completion_callback_);
+  completion_callback_.Reset();
+  completion_callback.Run();
+}
+
+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