content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/async_revalidation_manager.cc

Index: content/browser/loader/async_revalidation_manager.cc
diff --git a/content/browser/loader/async_revalidation_manager.cc b/content/browser/loader/async_revalidation_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..49a846153bf27a7300628892b2f45f6a9ed399e6
--- /dev/null
+++ b/content/browser/loader/async_revalidation_manager.cc
@@ -0,0 +1,177 @@
+// 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_manager.h"
+
+#include <tuple>
+#include <utility>
+
+#include "base/logging.h"
+#include "content/browser/loader/async_revalidation_driver.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/loader/resource_scheduler.h"
+#include "content/common/resource_messages.h"
+#include "content/public/browser/resource_throttle.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "url/gurl.h"
+
+namespace content {
+
+AsyncRevalidationManager::AsyncRevalidationKey::AsyncRevalidationKey(
+    const ResourceContext* resource_context,
+    const net::HttpCache* http_cache,
+    const GURL& url)
+    : resource_context(resource_context),
+      http_cache(http_cache),
+      url_key(net::HttpUtil::SpecForRequest(url)) {}
+
+AsyncRevalidationManager::AsyncRevalidationKey::AsyncRevalidationKey(
+    const ResourceContext* resource_context)
+    : resource_context(resource_context), http_cache(nullptr), url_key() {}
+
+AsyncRevalidationManager::AsyncRevalidationKey::~AsyncRevalidationKey() {}
+
+bool AsyncRevalidationManager::AsyncRevalidationKey::LessThan::operator()(
+    const AsyncRevalidationKey& lhs,
+    const AsyncRevalidationKey& rhs) const {
+  return std::tie(lhs.resource_context, lhs.http_cache, lhs.url_key) <
+         std::tie(rhs.resource_context, rhs.http_cache, rhs.url_key);
+}
+
+AsyncRevalidationManager::AsyncRevalidationManager() {}
+
+AsyncRevalidationManager::~AsyncRevalidationManager() {
+  DCHECK(in_progress_.empty());
+}
+
+void AsyncRevalidationManager::BeginAsyncRevalidation(
+    net::URLRequest* for_request,
+    ResourceScheduler* scheduler) {
+  DCHECK_EQ(for_request->url_chain().size(), 1u);
+  ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(for_request);
+  DCHECK(info);
+  if (!info->filter()) {
+    // The child has gone away. Since it never received the body of the stale
+    // resource, it is okay not to perform an async revalidation.

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

This comment is inaccurate. If the request turned into a download, we still
consumed the body. This also will fail to do stale-while-revalidate for
PlzNavigate, so you need at minimum a TODO here. Probably a bug as well.

(This also underscores why the HttpCache logic is a hard blocker for the
non-experimental implementation. So long as you are interacting with the RDH,
you cannot be making this kinds of impossible-to-ensure assumptions to justify
why we don't do an async revalidation. The implementation MUST be resilient to
that.)

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

I added a TODO with a bug.

+    return;
+  }
+
+  int child_id = info->GetChildID();
+  int route_id = info->GetRouteID();

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

This also will not work with PlzNavigate.

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

I filed crbug.com/561610 to track the incompatibility, and modified the RDH
constructor to never enable s-w-r when the PlzNavigate flag is set.

+
+  // ResourceMsgFilter::GetContexts requires a ResourceHostMsg_Request object be
+  // passed but only cares about the resource_type field.

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

Change the type of the method to only take the resource_type. Otherwise this
will break in the future without you noticing.

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

Done in http://crrev.com/1481583002

+  ResourceHostMsg_Request dummy_resource_host_msg_request;
+  dummy_resource_host_msg_request.resource_type = info->GetResourceType();
+  ResourceContext* resource_context = nullptr;
+  net::URLRequestContext* request_context = nullptr;
+
+  // This |request_context| needs to be valid until
+  // RemoveResourceContext(resource_context) is called. This is currently
+  // correct, see the comment about member variable ordering in
+  // //chrome/browser/profiles/profile_io_data.h.

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

//content referencing //chrome is a clear layering violation, and this comment
will easily become inaccurate without anyone noticing.

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

Comment changed. PTAL.

+  info->filter()->GetContexts(dummy_resource_host_msg_request,
+                              &resource_context, &request_context);
+
+  AsyncRevalidationKey async_revalidation_key(
+      resource_context, request_context->http_transaction_factory()->GetCache(),
+      for_request->url());
+  std::pair<AsyncRevalidationMap::iterator, bool> insert_result =
+      in_progress_.insert(AsyncRevalidationMap::value_type(
+          async_revalidation_key, scoped_ptr<AsyncRevalidationDriver>()));
+  if (!insert_result.second) {
+    // A matching async revalidation is already in progress for this cache; we
+    // don't need another one.

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

This comment is inaccurate. If there is a Vary header, things won't behave
correctly. This is fine because the real implementation will require the extra
HttpCache provision, but the comment should be written accordingly.

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

I added an explanation of the Vary issue. PTAL.

+    return;
+  }
+
+  net::HttpRequestHeaders headers;
+  headers.AddHeadersFromString(info->original_headers());

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

What are the situations where this differs from
URLRequest::extra_request_headers()?

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

URLRequest-internal changes appear to only happen on redirect (after calling the
delegate), so we don't have to worry about those.

External changes that I have found:

ChromeNetworkDelegate adds DNT: 1
PolicyHeaderIOHelper adds Chrome-Policy-Posture: <blob>
signin_header_helper.cc adds an X-Chrome-Connected header sometimes (maybe
redirects only?)
ChromeResourceDispatcherHostDelegate adds X-Chrome-UMA-Enabled and X-Client-Data

Those are the ones I've found with codesearch. They look manageable, but I'm not
confident I've found everything and more are likely to appear without warning.

+
+  // Construct the request.
+  scoped_ptr<net::URLRequest> new_request = request_context->CreateRequest(
+      for_request->url(), net::MINIMUM_PRIORITY, nullptr);
+
+  new_request->set_method(for_request->method());
+  new_request->set_first_party_for_cookies(
+      for_request->first_party_for_cookies());
+  new_request->set_first_party_url_policy(
+      for_request->first_party_url_policy());
+
+  new_request->SetReferrer(for_request->referrer());
+  new_request->set_referrer_policy(for_request->referrer_policy());
+
+  new_request->SetExtraRequestHeaders(headers);
+
+  int load_flags = for_request->load_flags() &
+                   ~net::LOAD_SUPPORT_ASYNC_REVALIDATION &
+                   ~net::LOAD_MAIN_FRAME;
+  DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);

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

Don't blacklist load flags. This will easily break when new ones are added. Set
the ones you need and whitelist what you carry over.

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

Done.

+  new_request->SetLoadFlags(load_flags);
+
+  scoped_ptr<ResourceThrottle> throttle =
+      scheduler->ScheduleRequest(child_id, route_id, false, new_request.get());
+
+  auto& async_revalidation = insert_result.first->second;

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

No non-const references. Also please spell out the type here.

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

Removed.

+  DCHECK(!async_revalidation);
+  // This use of base::Unretained() is safe because the AsyncRevalidatonDriver
+  // object will be destroyed before this object is. The use of the raw iterator
+  // is safe because objects are only erased from the map when they are deleted,
+  // and a deleted AsyncRevalidationDriver will not call the completion
+  // callback.

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

This is pretty verbose. How about:

// AsyncRevalidationDriver does not outlive its entry in |in_progress_|,
// so the iterator and |this| may be passed to base::Bind directly.

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

Done.

+  async_revalidation.reset(new AsyncRevalidationDriver(
+      std::move(new_request), std::move(throttle),
+      base::Bind(&AsyncRevalidationManager::OnAsyncRevalidationComplete,
+                 base::Unretained(this), insert_result.first)));
+  async_revalidation->StartRequest();
+}
+
+void AsyncRevalidationManager::CancelAsyncRevalidationsForResourceContext(
+    ResourceContext* resource_context) {
+  // For this algorithm to work, elements using |resource_context| must be
+  // contiguous in the map (ie. it must form the first part of the key).
+  AsyncRevalidationKey partial_key(resource_context);
+  for (auto it = in_progress_.lower_bound(partial_key);
+       it != in_progress_.end() &&
+       it->first.resource_context == resource_context;) {
+    // It is necessary to delete behind the iterator because an iterator can't
+    // be incremented once it has been removed from the map.
+    auto prev_it = it;
+    ++it;
+    prev_it->second->CancelRequest();
+    in_progress_.erase(prev_it);

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

All this can be replaced with:

it->second->CancelRequest();
it = in_progress_.erase(it);

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

That's much better, thank you.

+  }
+}
+
+bool AsyncRevalidationManager::QualifiesForAsyncRevalidation(
+    const ResourceHostMsg_Request& request) {
+  if (request.load_flags &
+      (net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+       net::LOAD_VALIDATE_CACHE | net::LOAD_PREFERRING_CACHE |
+       net::LOAD_ONLY_FROM_CACHE | net::LOAD_IGNORE_LIMITS)) {
+    return false;
+  }
+  if (request.method != "GET")
+    return false;
+  // A GET request should not have a body, but don't leave it to chance.

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

"but don't leave it to chance" is weird to put in the comment. I'd just take it
out.

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

Done.

+  if (request.request_body.get())
+    return false;
+  if (!request.url.SchemeIsHTTPOrHTTPS())
+    return false;
+
+  return true;
+}
+
+void AsyncRevalidationManager::OnAsyncRevalidationComplete(
+    AsyncRevalidationMap::iterator it) {
+  in_progress_.erase(it);
+}
+
+}  // namespace content