content::ResourceDispatcherHostImpl changes for stale-while-revalidate

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index 6cc7f6196f95a3832224a5332cb20df0b113cf6c..eb63c60683f30108eeab45ba851d6238672c52d8 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -8,6 +8,7 @@
 
 #include <algorithm>
 #include <set>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -19,6 +20,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/profiler/scoped_tracker.h"
@@ -91,6 +93,8 @@
 #include "net/cookies/cookie_monster.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/http/http_util.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
@@ -435,6 +439,23 @@ void LogResourceRequestTimeOnUI(
   }
 }
 
+bool 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)) {

[mmenke, 2015/10/09 16:02:22]

LOAD_IGNORE_LIMITS is also kinda weird.  You either need to remove it for the
revalidation request, use HIGHEST priority for the revalidation (Which is
weird), or just not allow async revalidation when it's present.

I think it may make sense to have a LOAD_FLAGS whitelist, instead of a
blacklist.  Also don't think we should preserve the LOAD_MAIN_FRAME flag.

[Adam Rice, 2015/10/13 22:53:17]

LOAD_IGNORE_LIMITS is never set by Blink, but I added anyway in case Blink
starts setting it in future. Sync XHR is already prevented from using s-w-r.

The nice thing about the blacklist is that it's easy to see why each item is
there. Whereas a whitelist would be a random grab-bag of whatever flags are left
over.

I added code to remove the LOAD_MAIN_FRAME flag from async revalidations.

+    return false;
+  }
+  if (request.method != "GET")
+    return false;
+  // A GET request should not have a body, but don't leave it to chance.
+  if (request.request_body.get())
+    return false;
+  if (!request.url.SchemeIsHTTPOrHTTPS())
+    return false;
+  return true;
+}
+
 }  // namespace
 
 // static
@@ -448,14 +469,14 @@ ResourceDispatcherHostImpl::ResourceDispatcherHostImpl()
       is_shutdown_(false),
       num_in_flight_requests_(0),
       max_num_in_flight_requests_(base::SharedMemory::GetHandleLimit()),
-      max_num_in_flight_requests_per_process_(
-          static_cast<int>(
-              max_num_in_flight_requests_ * kMaxRequestsPerProcessRatio)),
+      max_num_in_flight_requests_per_process_(static_cast<int>(
+          max_num_in_flight_requests_ * kMaxRequestsPerProcessRatio)),
       max_outstanding_requests_cost_per_process_(
           kMaxOutstandingRequestsCostPerProcess),
       filter_(NULL),
       delegate_(NULL),
-      allow_cross_origin_auth_prompt_(false) {
+      allow_cross_origin_auth_prompt_(false),
+      async_revalidation_enabled_(false) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!g_resource_dispatcher_host);
   g_resource_dispatcher_host = this;
@@ -473,10 +494,19 @@ ResourceDispatcherHostImpl::ResourceDispatcherHostImpl()
 
   update_load_states_timer_.reset(
       new base::RepeatingTimer<ResourceDispatcherHostImpl>());
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  if (base::FieldTrialList::FindFullName("StaleWhileRevalidate") == "Enabled" ||
+      command_line->HasSwitch(switches::kEnableStaleWhileRevalidate)) {
+    async_revalidation_enabled_ = true;
+  }
 }
 
 ResourceDispatcherHostImpl::~ResourceDispatcherHostImpl() {
   DCHECK(outstanding_requests_stats_map_.empty());
+  // It's not safe to destroy this object while AsyncRevalidationDriver objects
+  // have callbacks referencing it.
+  CHECK(in_progress_async_revalidations_.empty());
   DCHECK(g_resource_dispatcher_host);
   g_resource_dispatcher_host = NULL;
 }
@@ -578,6 +608,10 @@ void ResourceDispatcherHostImpl::CancelRequestsForContext(
 
   loaders_to_cancel.clear();
 
+  // Cancelling async revalidations should not result in the creation of new
+  // requests, but do it before the checks just to be on the safe side.
+  CancelAsyncRevalidationsForResourceContext(context);
+
   // Validate that no more requests for this context were added.
   for (LoaderMap::const_iterator i = pending_loaders_.begin();
        i != pending_loaders_.end(); ++i) {
@@ -776,6 +810,14 @@ scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::MaybeInterceptAsStream(
   return handler.Pass();
 }
 
+ResourceDispatcherHostImpl::ExtraRequestParams::ExtraRequestParams()
+    : blob_context(nullptr),
+      child_id(0),
+      is_sync_load(false),
+      use_embedded_identity(false),
+      support_async_revalidation(false),
+      extra_headers(nullptr) {}
+
 ResourceDispatcherHostLoginDelegate*
 ResourceDispatcherHostImpl::CreateLoginDelegate(
     ResourceLoader* loader,
@@ -824,6 +866,14 @@ void ResourceDispatcherHostImpl::DidReceiveRedirect(ResourceLoader* loader,
   if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_host))
     return;
 
+  // Remove the LOAD_SUPPORT_ASYNC_REVALIDATION flag if it is present.
+  net::URLRequest* request = loader->request();
+  if (request->load_flags() & net::LOAD_SUPPORT_ASYNC_REVALIDATION) {
+    int new_load_flags = request->load_flags() &
+        ~net::LOAD_SUPPORT_ASYNC_REVALIDATION;
+    request->SetLoadFlags(new_load_flags);

[davidben, 2015/10/08 21:57:51]

This isn't sufficient. The first redirect leg already was sent with
LOAD_SUPPORT_ASYNC_REVALIDATION, which means we've used a stale request while
systematically not doing an async revalidation for it. (See also proposal on
net-dev.)

One option is to do the BeginAsyncRevalidation check BOTH here and in in
DidReceiveResponse. Also in BeginAsyncRevalidation, you should DCHECK that the
redirect chain has length 1 because that's the only case you're capable of
handling right now.

[Adam Rice, 2015/10/13 22:53:17]

I think will just modify net::HttpCache::Transaction to avoid returning a stale
response with a 300 status code for the purposes of the experiment. I need to
fix it properly before shipping anyway (assuming the experiment succeeds).

+  }
+
   // Notify the observers on the UI thread.
   scoped_ptr<ResourceRedirectDetails> detail(new ResourceRedirectDetails(
       loader->request(),
@@ -846,6 +896,9 @@ void ResourceDispatcherHostImpl::DidReceiveResponse(ResourceLoader* loader) {
         info->GetChildID(), info->GetRouteID());
   }
 
+  if (request->response_info().async_revalidation_required)
+    BeginAsyncRevalidation(request);

[mmenke, 2015/10/09 16:02:22]

We shouldn't call this on failures, right?  Particularly cancellations - maybe
the page was navigated away from, or maybe safebrowsing or group policy blocked
the request or something.

[mmenke, 2015/10/09 16:29:57]

David pointed out to me that we don't reach this point on requests that failed
before headers were received, so this is a non-issue.

Still may want to not do revalidation on requests that were cancelled before
they completed, but that's a minor consideration.

+
   int render_process_id, render_frame_host;
   if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_host))
     return;
@@ -1210,55 +1263,13 @@ void ResourceDispatcherHostImpl::BeginRequest(
     return;
   }
 
-  // Construct the request.
-  scoped_ptr<net::URLRequest> new_request = request_context->CreateRequest(
-      request_data.url, request_data.priority, NULL);
-
-  new_request->set_method(request_data.method);
-  new_request->set_first_party_for_cookies(
-      request_data.first_party_for_cookies);
-
-  // If the request is a MAIN_FRAME request, the first-party URL gets updated on
-  // redirects.
-  if (request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME) {
-    new_request->set_first_party_url_policy(
-        net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
-  }
-
-  const Referrer referrer(request_data.referrer, request_data.referrer_policy);
-  SetReferrerForRequest(new_request.get(), referrer);
-
-  new_request->SetExtraRequestHeaders(headers);
-
-  storage::BlobStorageContext* blob_context =
-      GetBlobStorageContext(filter_->blob_storage_context());
-  // Resolve elements from request_body and prepare upload data.
-  if (request_data.request_body.get()) {
-    // |blob_context| could be null when the request is from the plugins because
-    // ResourceMessageFilters created in PluginProcessHost don't have the blob
-    // context.
-    if (blob_context) {
-      // Attaches the BlobDataHandles to request_body not to free the blobs and
-      // any attached shareable files until upload completion. These data will
-      // be used in UploadDataStream and ServiceWorkerURLRequestJob.
-      AttachRequestBodyBlobDataHandles(
-          request_data.request_body.get(),
-          blob_context);
-    }
-    new_request->set_upload(UploadDataStreamBuilder::Build(
-        request_data.request_body.get(),
-        blob_context,
-        filter_->file_system_context(),
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)
-            .get()));
-  }
-
-  bool allow_download = request_data.allow_download &&
-      IsResourceTypeFrame(request_data.resource_type);
+  ExtraRequestParams params;
+  params.blob_context = GetBlobStorageContext(filter_->blob_storage_context());
+  params.child_id = child_id;
+  params.is_sync_load = sync_result != nullptr;
+  params.use_embedded_identity = true;
+  params.extra_headers = &headers;
   bool do_not_prompt_for_login = request_data.do_not_prompt_for_login;
-  bool is_sync_load = sync_result != NULL;
-  int load_flags =
-      BuildLoadFlagsForRequest(request_data, child_id, is_sync_load);
   if (request_data.resource_type == RESOURCE_TYPE_PREFETCH ||
       request_data.resource_type == RESOURCE_TYPE_FAVICON) {
     do_not_prompt_for_login = true;
@@ -1276,18 +1287,19 @@ void ResourceDispatcherHostImpl::BeginRequest(
     // filter sources for <script>, <link>, <embed>, <object>, <iframe> tags
     // would be considered vulnerable in and of itself.
     do_not_prompt_for_login = true;
-    load_flags |= net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY;
+    params.use_embedded_identity = false;
   }
 
-  // Sync loads should have maximum priority and should be the only
-  // requets that have the ignore limits flag set.
-  if (is_sync_load) {
-    DCHECK_EQ(request_data.priority, net::MAXIMUM_PRIORITY);
-    DCHECK_NE(load_flags & net::LOAD_IGNORE_LIMITS, 0);
-  } else {
-    DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);
-  }
-  new_request->SetLoadFlags(load_flags);
+  // Check if request is eligible for async revalidation.
+  params.support_async_revalidation =
+      (!params.is_sync_load && async_revalidation_enabled_ &&
+       QualifiesForAsyncRevalidation(request_data));

[mmenke, 2015/10/08 21:58:26]

We shouldn't do async revalidations when "predictive network actions" is false. 
That's a per-profile setting managed that lives chrome-side (See
chrome/browser/net/prediction_options.h)

[Adam Rice, 2015/10/13 22:53:17]

I don't agree that stale-while-revalidate is a predictive feature. We are not
guessing that a resource will be used. A resource has been used, and we are
revalidating it.

The Prefetch settings toggle exists to preserve privacy and conserve bandwidth.
However, forcing revalidations to be synchronous neither improves privacy nor
reduces bandwidth usage.

[mmenke, 2015/10/13 23:01:53]

You are guessing that it will be used in the future, and using a user's
bandwidth based on that assumption.  If it won't be used, we're just wasting
bandwidth on a resource that can, in fact, still be used.

[mmenke, 2015/10/14 00:16:49]

Worth noting it also potentially drains battery by keeping the cell modem up
longer than usual.

[Adam Rice, 2015/10/14 15:03:45]

The feature is not speculative. We are not making guesses about future
behaviour, we are fulfilling a contract with the server that says we can
use the resource stale while we revalidate it.

We're not wasting bandwidth, because in absence of s-w-r we would have
had to perform a synchronous revalidation. We are performing the same
operations, but in a different order.

There are cases in which we cannot perform the async revalidation, but that
does not mean that the revalidation is optional.

Although I do not consider it necessary for experimental purposes, before
shipping I will have to implement the feature of preventing further stale use
of the resource after an async revalidation fails.

The only place where any heuristics factor into it at all is in scheduling the
asynchronous request. Since the same scheduling algorithm is applied
to almost all other requests, this cannot by itself be considered to place
the request in the category of "predictive network actions".

I think it's entirely possible that the ResourceScheduler will in some cases use
more battery for a low-priority request that it would for the same resource
requested at higher priority.  However, that has no bearing on the semantics
of stale-while-revalidate, it is simply a deficiency of our scheduling
implementation.

+
+  scoped_ptr<net::URLRequest> new_request =
+      ConstructRequest(request_context, request_data, params);
+
+  bool allow_download = request_data.allow_download &&
+      IsResourceTypeFrame(request_data.resource_type);
 
   // Make extra info and read footer (contains request ID).
   ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl(
@@ -1312,7 +1324,8 @@ void ResourceDispatcherHostImpl::BeginRequest(
       request_data.referrer_policy,
       request_data.visiblity_state,
       resource_context, filter_->GetWeakPtr(),
-      !is_sync_load);
+      !params.is_sync_load,
+      params.support_async_revalidation ? &request_data : NULL);
   // Request takes ownership.
   extra_info->AssociateWithRequest(new_request.get());
 
@@ -1328,9 +1341,9 @@ void ResourceDispatcherHostImpl::BeginRequest(
   // Initialize the service worker handler for the request. We don't use
   // ServiceWorker for synchronous loads to avoid renderer deadlocks.
   ServiceWorkerRequestHandler::InitializeHandler(
-      new_request.get(), filter_->service_worker_context(), blob_context,
+      new_request.get(), filter_->service_worker_context(), params.blob_context,
       child_id, request_data.service_worker_provider_id,
-      request_data.skip_service_worker || is_sync_load,
+      request_data.skip_service_worker || params.is_sync_load,
       request_data.fetch_request_mode, request_data.fetch_credentials_mode,
       request_data.fetch_redirect_mode, request_data.resource_type,
       request_data.fetch_request_context_type, request_data.fetch_frame_type,
@@ -1342,16 +1355,79 @@ void ResourceDispatcherHostImpl::BeginRequest(
       request_data.appcache_host_id, request_data.resource_type,
       request_data.should_reset_appcache);
 
-  scoped_ptr<ResourceHandler> handler(
-       CreateResourceHandler(
-           new_request.get(),
-           request_data, sync_result, route_id, process_type, child_id,
-           resource_context));
+  scoped_ptr<ResourceHandler> handler(CreateResourceHandler(
+      new_request.get(), request_data, sync_result, route_id, process_type,
+      child_id, resource_context));
 
   if (handler)
     BeginRequestInternal(new_request.Pass(), handler.Pass());
 }
 
+scoped_ptr<net::URLRequest> ResourceDispatcherHostImpl::ConstructRequest(
+    net::URLRequestContext* request_context,
+    const ResourceHostMsg_Request& request_data,
+    const ExtraRequestParams& extra_params) {
+  // Construct the request.
+  scoped_ptr<net::URLRequest> new_request = request_context->CreateRequest(
+      request_data.url, request_data.priority, NULL);
+
+  new_request->set_method(request_data.method);
+  new_request->set_first_party_for_cookies(
+      request_data.first_party_for_cookies);
+
+  // If the request is a MAIN_FRAME request, the first-party URL gets updated on
+  // redirects.
+  if (request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME) {
+    new_request->set_first_party_url_policy(
+        net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
+  }
+
+  const Referrer referrer(request_data.referrer, request_data.referrer_policy);
+  SetReferrerForRequest(new_request.get(), referrer);
+
+  new_request->SetExtraRequestHeaders(*extra_params.extra_headers);
+
+  // Resolve elements from request_body and prepare upload data.
+  if (request_data.request_body.get()) {
+    // |blob_context| could be null when the request is from the plugins because
+    // ResourceMessageFilters created in PluginProcessHost don't have the blob
+    // context.
+    if (extra_params.blob_context) {
+      // Attaches the BlobDataHandles to request_body not to free the blobs and
+      // any attached shareable files until upload completion. These data will
+      // be used in UploadDataStream and ServiceWorkerURLRequestJob.
+      AttachRequestBodyBlobDataHandles(request_data.request_body.get(),
+                                       extra_params.blob_context);
+    }
+    new_request->set_upload(UploadDataStreamBuilder::Build(
+        request_data.request_body.get(), extra_params.blob_context,
+        filter_->file_system_context(),
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)
+            .get()));
+  }
+
+  int load_flags = BuildLoadFlagsForRequest(request_data, extra_params.child_id,
+                                            extra_params.is_sync_load);
+
+  if (!extra_params.use_embedded_identity)
+    load_flags |= net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY;
+
+  if (extra_params.support_async_revalidation)
+    load_flags |= net::LOAD_SUPPORT_ASYNC_REVALIDATION;
+
+  // Sync loads should have maximum priority and should be the only
+  // requests that have the ignore limits flag set.
+  if (extra_params.is_sync_load) {
+    DCHECK_EQ(request_data.priority, net::MAXIMUM_PRIORITY);
+    DCHECK_NE(load_flags & net::LOAD_IGNORE_LIMITS, 0);
+  } else {
+    DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);
+  }
+  new_request->SetLoadFlags(load_flags);
+
+  return new_request.Pass();
+}
+
 scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::CreateResourceHandler(
     net::URLRequest* request,
     const ResourceHostMsg_Request& request_data,
@@ -1583,7 +1659,8 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
       blink::WebPageVisibilityStateVisible,
       context,
       base::WeakPtr<ResourceMessageFilter>(),  // filter
-      true);                                   // is_async
+      true,                                    // is_async
+      NULL);                                   // original_request

[davidben, 2015/10/08 21:57:52]

nullptr

[Adam Rice, 2015/10/13 22:53:17]

Done.

 }
 
 void ResourceDispatcherHostImpl::OnRenderViewHostCreated(int child_id,
@@ -2026,7 +2103,8 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
       blink::WebPageVisibilityStateVisible,
       resource_context,
       base::WeakPtr<ResourceMessageFilter>(),  // filter
-      true);
+      true,                                    // is_async
+      NULL);                                   // original_message

[davidben, 2015/10/08 21:57:51]

original_request

[davidben, 2015/10/08 21:57:52]

nullptr

[Adam Rice, 2015/10/13 22:53:17]

Done.

[Adam Rice, 2015/10/13 22:53:17]

Done.

   // Request takes ownership.
   extra_info->AssociateWithRequest(new_request.get());
 
@@ -2053,8 +2131,8 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
   handler = AddStandardHandlers(new_request.get(), resource_type,
                                 resource_context,
                                 nullptr,  // appcache_service
-                                -1,  // child_id
-                                -1,  // route_id
+                                -1,       // child_id
+                                -1,       // route_id
                                 handler.Pass());
 
   BeginRequestInternal(new_request.Pass(), handler.Pass());
@@ -2392,4 +2470,104 @@ int ResourceDispatcherHostImpl::BuildLoadFlagsForRequest(
   return load_flags;
 }
 
+void ResourceDispatcherHostImpl::BeginAsyncRevalidation(
+    net::URLRequest* for_request) {
+  ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(for_request);
+  DCHECK(info);
+  ResourceHostMsg_Request* original_request_data = info->original_request();
+  DCHECK(original_request_data);
+  DCHECK(!original_request_data->request_body);
+
+  ResourceHostMsg_Request new_request_data = *original_request_data;
+  new_request_data.do_not_prompt_for_login = true;
+  new_request_data.allow_download = false;
+  new_request_data.priority = net::MINIMUM_PRIORITY;
+
+  int child_id = info->GetChildID();
+  int route_id = info->GetRouteID();
+
+  ResourceContext* resource_context = NULL;
+  net::URLRequestContext* request_context = NULL;
+  // This |request_context| needs to be valid until
+  // RemoveResourceContext(resource_context) is called. This is currently
+  // correct, see
+  // https://groups.google.com/a/chromium.org/d/msg/net-dev/lHh764ZFdr0/phJIWXokt7cJ

[davidben, 2015/10/08 21:57:51]

I must have missed the context when you made the original thread posting because
this is not accurate. While you can assume that info's ResourceContext is valid,
info->filter() MAY still be NULL. There is no guarantee that, at this point, the
child is still around. (Again, I want to fix the cases where the request may
outlive its child, which is part of why the original version of this CL was not
okay, but right now there are cases where the request will outlive the child.)

[Adam Rice, 2015/10/13 22:53:17]

If the child is already gone at this point, I think it's reasonable to just
pretend the request never happened.

What happens if the request is transferred to a different renderer?

+  info->filter()->GetContexts(new_request_data, &resource_context,
+                              &request_context);
+  CHECK(ContainsKey(active_resource_contexts_, resource_context));
+  if (is_shutdown_)
+    return;

[mmenke, 2015/10/09 16:02:22]

Can this happen while shutting down?  Also, why isn't this first?

[Adam Rice, 2015/10/13 22:53:17]

1. Based on other uses in this file: probably.
2. Based on other uses in this file, it seems quite popular to do a bunch of
other processing before checking is_shutdown_, but I can't see a good reason for
it.

+
+  AsyncRevalidationKey async_revalidation_key(
+      resource_context, request_context->http_transaction_factory()->GetCache(),
+      original_request_data->url);
+  std::pair<AsyncRevalidationMap::iterator, bool> insert_result =
+      in_progress_async_revalidations_.insert(
+          AsyncRevalidationMap::value_type(async_revalidation_key, nullptr));
+  if (!insert_result.second) {
+    // A matching async revalidation is already in progress for this cache; we
+    // don't need another one.
+    return;
+  }
+
+  net::HttpRequestHeaders headers;
+  headers.AddHeadersFromString(new_request_data.headers);

[davidben, 2015/10/08 21:57:51]

If the request did not change, it really needs to be conditionalized. I.e. if
we're replaying a response like:

  Cache-Control: max-age=5000, stale-while-revalidate=10000
  ETag: "123456"

The revalidation should only ever request:

  If-None-Match: "123456".

That means either we do it within HttpCache which already makes such requests
or, if making a new URLRequest (I've come around to this interpretation, pending
the change from net-dev), we externally conditionalize it. (The HTTP cache
should be able to handle externally conditionalized requests, but you should
test this.)

This is important as, in the common case, we do not expect the request to have
changed. This implementation requires we read the cached body twice.

[Adam Rice, 2015/10/13 22:53:17]

The cache will conditionalize the request if If-Modified-Since or ETag headers
are present in the cached response. See
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...

It's better to let the cache do it, because it is fussy about externally
conditionalized requests and won't store the response if the request doesn't
exactly match its expectations.

+
+  ExtraRequestParams params;
+  params.child_id = child_id;

[davidben, 2015/10/08 21:57:52]

This request cannot be associated with this process. It may outlive it. (In some
cases, the child may not even be around anymore. Those also should get fixed,
but don't add new cases.)

[Adam Rice, 2015/10/13 22:53:17]

This is only used by BuildLoadFlagsForRequest to decide whether to set
LOAD_REPORT_RAW_HEADERS. Once devtools learn how to display async revalidations
this will be good to have, so I am tempted to leave it in.

+  params.is_sync_load = false;
+  params.use_embedded_identity = true;
+  params.support_async_revalidation = false;
+  params.extra_headers = &headers;
+  if (new_request_data.resource_type == RESOURCE_TYPE_IMAGE &&
+      HTTP_AUTH_RELATION_BLOCKED_CROSS ==
+          HttpAuthRelationTypeOf(new_request_data.url,
+                                 new_request_data.first_party_for_cookies)) {

[davidben, 2015/10/08 21:57:52]

We should avoid duplicating this code. Ideally by not making it an issue to
begin with. Matt and I had been talking about, for redirects, having APIs within
//net to return a new URLRequest with the right set of parameters for starting
at a particular redirect leg. (We were thinking parameters for replaying the
redirect at this point.) It was intended as a "for when we want to bother with
redirects" thing, but given this logic, it's probably worth doing this now.

[Adam Rice, 2015/10/13 22:53:17]

I would rather do the experiment first before worrying too much about the
redirect case. If the experiment fails to show benefit then any extra work we do
now is wasted. If we do demonstrate benefit, then we need to fix redirects
before shipping. Either way, this code duplication is extremely temporary.

+    // Prevent <img> tags from using cached credentials to third-party sites
+    // to perform actions on those sites.
+    params.use_embedded_identity = false;
+  }
+
+  scoped_ptr<net::URLRequest> new_request =
+      ConstructRequest(request_context, new_request_data, params);
+
+  scoped_ptr<ResourceThrottle> throttle = scheduler_->ScheduleRequest(
+      child_id, route_id, false, new_request.get());

[davidben, 2015/10/08 21:57:51]

Associating the request with child_id / route_id doesn't make sense. It outlives
the document, but we may navigate in-process or cross-process, which means
quirks of process allocation control scheduling.

This also doesn't make much sense when combined with AsyncRevalidationKey (maybe
the async revalidation would have happened sooner if we used the other route.)

It also means that we call Start() on the request as soon as you close the tab
which seems especially bizarre.

[Adam Rice, 2015/10/13 22:53:17]

The async request has to be scheduled after the other resources for the same
page. In my experiments, this is critical to the performance of the feature.
Since the ResourceScheduler identifies a page (strictly speaking, a "Client") by
the (child_id, route_id) pair, the async request has to share the (child_id,
route_id) pair with the other resources on the page.

I have a CL which modifies the ResourceScheduler to schedule async revalidations
after everything else: https://codereview.chromium.org/1209013003/
I abandoned it because it offered no significant benefit over the normal FIFO
behaviour in my experiments, but I didn't test what happens on navigations, so
it may be worth revisiting.
AsyncRevalidationKey is used to de-dupe async revalidations, and also to cancel
them when the ResourceContext goes away. It has nothing to do with scheduling.
Yes. That is the behaviour of the ResourceScheduler, and I agree that it is
bizarre and should be changed, but I think it is outside the scope of this CL.

+  // This use of base::Unretained() is safe because the AsyncRevalidatonDriver
+  // object will be destroyed before this object is.
+  insert_result.first->second = new AsyncRevalidationDriver(
+      new_request.Pass(), throttle.Pass(),
+      base::Bind(&ResourceDispatcherHostImpl::OnAsyncRevalidationComplete,
+                 base::Unretained(this), async_revalidation_key));

[mmenke, 2015/10/09 16:02:22]

Not creating a ResourceRequestInfo for the request will have some side effects,
like ChromeNetworkDelegate::OnCanSetCookie behaving differently (At least not
posting a task), different behavior when it comes to flywheel/freighter magic,
skipping the blacklist logic, and who knows what else.  Should dig through
ChromeNetworkDelegate and figure what it breaks.  I'm also concerned about
possible future breakages.

[Adam Rice, 2015/10/13 22:53:17]

Sorry, I thought we had agreed that the request should not have a
ResourceRequestInfo to avoid NetworkDelegate hooks that perform UI operations.

I will verify that flywheel works manually. Please don't do any heavy lifting on
this until we have experimental validation that stale-while-revalidate delivers
the benefits we expect.

[mmenke, 2015/10/13 23:01:53]

I'm not saying we should have the object, but that the request will have
different behavior in subtly different ways, even ignoring the UI stuff.

+  insert_result.first->second->StartRequest();
+}
+
+void ResourceDispatcherHostImpl::OnAsyncRevalidationComplete(
+    const AsyncRevalidationKey& key) {
+  // TODO(ricea): Record histograms?
+  auto it = in_progress_async_revalidations_.find(key);
+  // This is CHECK() and not DCHECK() because deferencing an off-the-end
+  // iterator would cause a security hole.
+  CHECK(it != in_progress_async_revalidations_.end());

[davidben, 2015/10/08 21:57:52]

I don't think we do CHECK vs DCHECK this way. Otherwise we'd never DCHECK it !=
foo.end() checks ever.

[Adam Rice, 2015/10/13 22:53:17]

My understanding is that the security people would rather we never did DCHECK(it
!= foo.end()) ever. They hate off-the-end iterators. The cost of getting it
wrong is very high.

I'll change it if you insist.

+  delete it->second, it->second = nullptr;

[davidben, 2015/10/08 21:57:51]

Style: Don't use the comma operator.

[Adam Rice, 2015/10/13 22:53:17]

Done.

+  in_progress_async_revalidations_.erase(it);
+}
+
+void ResourceDispatcherHostImpl::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);
+  auto next_it = in_progress_async_revalidations_.lower_bound(partial_key);
+  while (next_it != in_progress_async_revalidations_.end() &&
+         next_it->first.resource_context == resource_context) {
+    // Erasing the element invalidates the iterator. Increment it first.
+    auto current_it = next_it++;
+    current_it->second->CancelRequest();
+    delete current_it->second, current_it->second = nullptr;

[davidben, 2015/10/08 21:57:52]

Style: Don't use the comma operator.

[Adam Rice, 2015/10/13 22:53:17]

Done.

+    in_progress_async_revalidations_.erase(current_it);
+  }
+}
+
 }  // namespace content