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"

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

What do you need this include for?

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

For FROM_HERE. Added a comment.

+#include "base/logging.h"

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

Please include base/macros.h for DCHECK.

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

Is is going to move there? It is currently in base/logging.h.

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

You are right.  Sorry.

+#include "base/metrics/user_metrics_action.h"
+#include "base/single_thread_task_runner.h"

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

What do you need this include for?

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

base::ThreadTaskRunnerHandle::Get() returns a SingleThreadTaskRunner. I added a
comment.

+#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"
+#include "net/http/http_transaction_factory.h"

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

What do you need this include for?

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

It looks like a mistake. I have removed it.

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

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

Please remove this include, because you already incuded it from the header file.

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

Thanks, done.

+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_status.h"
+
+using base::TimeDelta;
+using base::TimeTicks;

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

Please remove both these forward declarations, because you have base/time/time.h
included.

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

Done.

+
+namespace content {
+
+namespace {
+// This matches the maximum allocation size of AsyncResourceHandler.
+const int kReadBufSize = 32 * 1024;
+const int kReadTimeoutSeconds = 5 * 60;
+}
+
+// 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,
+                  TimeDelta::FromSeconds(kReadTimeoutSeconds),
+                  base::Bind(&AsyncRevalidationDriver::OnReadTimeout,
+                             base::Unretained(this)),
+                  false),
+      request_(request.Pass()),
+      throttle_(throttle.Pass()),
+      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);
+

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

Please add a comment along the lines of "The revalidated cache entry should not
follow redirects, because caching is a property of an individual HTTP resource."
(paraphrasing from davidben@'s e-mail on net-dev@).

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

Done.

+  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()) {
+    DCHECK(!has_already_failed_);
+    has_already_failed_ = true;

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

Consider moving these two lines (DCHECK and set |has_already_failed_|) to
ResponseCompleted() from here and from every other occurrence before a call to
ResponseCompleted().  In that case, |has_already_called_callback_| or something
similar would be a better name.

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

Done. I changed ResponseCompleted() to reset the callback, so we can avoid
calling
it twice without needing a separate flag.

+    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 {
+    DCHECK(!has_already_failed_);
+    has_already_failed_ = true;
+    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()) {
+    if (!has_already_failed_) {
+      has_already_failed_ = true;
+      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. Call ResponseCompleted.

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

Consider removing "Call ResponseCompleted." as it does not add carry additional
information to what is trivial from code.

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

Done.

+    DCHECK_EQ(0, bytes_read);
+    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() has already been called, it isn't necessary
+  // to call ResponseCompleted().

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

Consider adding DCHECK(has_already_failed_) to document this.

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

In this case CancelRequestInternal calls ResponseCompleted() asynchronously via
PostTask(), so it may not actually have run yet.

+  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() {
+  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();
+  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