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 f342602626ab81b27875ce55c0effbbb83afdc6c..314828debdc6a2d5f2dc20e84f9a1902a5336342 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -19,6 +19,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"
@@ -482,10 +483,19 @@ ResourceDispatcherHostImpl::ResourceDispatcherHostImpl()
                                      base::Unretained(this)));
 
   update_load_states_timer_.reset(new base::RepeatingTimer());
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  if (base::FieldTrialList::FindFullName("StaleWhileRevalidate") == "Enabled" ||
+      command_line->HasSwitch(switches::kEnableStaleWhileRevalidate)) {
+    async_revalidation_manager_.reset(new AsyncRevalidationManager);
+  }
 }
 
 ResourceDispatcherHostImpl::~ResourceDispatcherHostImpl() {
   DCHECK(outstanding_requests_stats_map_.empty());
+  // It's not safe to destroy this object while AsyncRevalidationDriver objects
+  // have callbacks referencing it.

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

What are "this object" and "it" here? RDH? ARM?

I also don't understand what the concern is here. Surely at this point,
CancelAsyncRevalidationsForRequestContext would have happened.

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

Sorry, the comment was out-of-date, and there's no need to explicitly destroy
async_revalidation_manager_ any more.

+  async_revalidation_manager_.reset();
   DCHECK(g_resource_dispatcher_host);
   g_resource_dispatcher_host = NULL;
 }
@@ -587,6 +597,13 @@ void ResourceDispatcherHostImpl::CancelRequestsForContext(
 
   loaders_to_cancel.clear();
 
+  if (async_revalidation_manager_) {
+    // 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.

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

checks -> CHECKs and "just to be on the safe side" -> "to ensure this does not
happen".

It took me several tries to understand what this comment was about. "just to be
on the safe side" could just as easily mean "so we don't crash if this happens".

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

Sorry about that. It should be clearer now.

+    async_revalidation_manager_->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) {
@@ -835,6 +852,28 @@ void ResourceDispatcherHostImpl::DidReceiveRedirect(ResourceLoader* loader,
   if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_host))
     return;
 
+  net::URLRequest* request = loader->request();
+  if (request->response_info().async_revalidation_required && !is_shutdown_) {

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

In what situation could is_shutdown_ be true here?

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

It can't. Removed.

+    // Async revalidation is only supported for the first redirect leg.
+    DCHECK_EQ(request->url_chain().size(), 1u);
+    DCHECK(async_revalidation_manager_);
+
+    async_revalidation_manager_->BeginAsyncRevalidation(request,
+                                                        scheduler_.get());
+  }
+
+  // Remove the LOAD_SUPPORT_ASYNC_REVALIDATION flag if it is present.
+  // It is difficult to create a URLRequest with the correct flags and headers
+  // for redirect legs other than the first one. Since stale-while-revalidate in
+  // combination with redirects isn't needed for experimental use, punt on it
+  // for now.
+  // TODO(ricea): Fix this before launching the feature.
+  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);
+  }
+
   // Don't notify WebContents observers for requests known to be
   // downloads; they aren't really associated with the Webcontents.
   // Note that not all downloads are known before content sniffing.
@@ -863,6 +902,12 @@ void ResourceDispatcherHostImpl::DidReceiveResponse(ResourceLoader* loader) {
         info->GetChildID(), info->GetRouteID());
   }
 
+  if (request->response_info().async_revalidation_required && !is_shutdown_) {

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

In what situation can is_shutdown_ be true here?

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

Removed.

+    DCHECK(async_revalidation_manager_);
+    async_revalidation_manager_->BeginAsyncRevalidation(request,
+                                                        scheduler_.get());
+  }
+
   int render_process_id, render_frame_host;
   if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_host))
     return;
@@ -1321,6 +1366,13 @@ void ResourceDispatcherHostImpl::BeginRequest(
     load_flags |= net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY;
   }
 
+  bool support_async_revalidation =
+      (!is_sync_load && async_revalidation_manager_ &&
+       AsyncRevalidationManager::QualifiesForAsyncRevalidation(request_data));
+
+  if (support_async_revalidation)
+    load_flags |= net::LOAD_SUPPORT_ASYNC_REVALIDATION;
+
   // Sync loads should have maximum priority and should be the only
   // requets that have the ignore limits flag set.
   if (is_sync_load) {
@@ -1357,7 +1409,8 @@ void ResourceDispatcherHostImpl::BeginRequest(
       report_raw_headers,
       !is_sync_load,
       IsUsingLoFi(request_data.lofi_state, delegate_,
-                  *new_request, resource_context));
+                  *new_request, resource_context),
+      support_async_revalidation ? request_data.headers : std::string());
   // Request takes ownership.
   extra_info->AssociateWithRequest(new_request.get());
 
@@ -1644,7 +1697,8 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
       base::WeakPtr<ResourceMessageFilter>(),  // filter
       false,                                   // report_raw_headers
       true,                                    // is_async
-      false);                                  // is_using_lofi
+      false,                                   // is_using_lofi
+      std::string());                          // original_headers
 }
 
 void ResourceDispatcherHostImpl::OnRenderViewHostCreated(int child_id,
@@ -2087,9 +2141,10 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
       blink::WebPageVisibilityStateVisible, resource_context,
       base::WeakPtr<ResourceMessageFilter>(),  // filter
       false,  // request_data.report_raw_headers
-      true,
       IsUsingLoFi(info.common_params.lofi_state, delegate_,
-                  *new_request, resource_context));
+                  *new_request, resource_context),
+      true,                                    // is_async

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

BUG: You switched two parameters here.

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

Fixed.

+      std::string());                          // original_headers

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

Please leave at least a TODO, preferably a bug, that this doesn't work with
PlzNavigate.

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

Done.

   // Request takes ownership.
   extra_info->AssociateWithRequest(new_request.get());
 
@@ -2127,6 +2182,11 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
   BeginRequestInternal(new_request.Pass(), handler.Pass());
 }
 
+void ResourceDispatcherHostImpl::EnableStaleWhileRevalidateForTesting() {
+  if (!async_revalidation_manager_)
+    async_revalidation_manager_.reset(new AsyncRevalidationManager);
+}
+
 // static
 int ResourceDispatcherHostImpl::CalculateApproximateMemoryCost(
     net::URLRequest* request) {