Allows prefetch requests to live beyond the renderer by delaying

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 839678e30ce5cc01c6e57f60d725d6ef6e196d1b..1169bbacdd3b48bc42217c4029e5acbc759944a6 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -32,6 +32,7 @@
 #include "content/browser/fileapi/chrome_blob_storage_context.h"
 #include "content/browser/loader/async_resource_handler.h"
 #include "content/browser/loader/buffered_resource_handler.h"
+#include "content/browser/loader/detached_resource_handler.h"

[mmenke, 2013/10/17 18:33:48]

Remove.

[jkarlin2, 2013/10/24 15:33:11]

Done.

 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/power_save_block_resource_throttle.h"
 #include "content/browser/loader/redirect_to_file_resource_handler.h"
@@ -127,6 +128,15 @@ const int kUserGestureWindowMs = 3500;
 // use. Arbitrarily chosen.
 const double kMaxRequestsPerProcessRatio = 0.45;
 
+bool IsDetachableResourceType(const ResourceType::Type type) {
+  switch (type) {
+    case ResourceType::PREFETCH:
+      return true;
+    default:
+      return false;
+  }
+}
+
 // Aborts a request before an URLRequest has actually been created.
 void AbortRequestBeforeItStarts(ResourceMessageFilter* filter,
                                 IPC::Message* sync_result,
@@ -406,13 +416,16 @@ void ResourceDispatcherHostImpl::CancelRequestsForContext(
   for (LoaderList::iterator i = loaders_to_cancel.begin();
        i != loaders_to_cancel.end(); ++i) {
     // There is no strict requirement that this be the case, but currently
-    // downloads, streams  and transferred requests are the only requests that
-    // aren't cancelled when the associated processes go away. It may be OK for
-    // this invariant to change in the future, but if this assertion fires
-    // without the invariant changing, then it's indicative of a leak.
-    DCHECK((*i)->GetRequestInfo()->is_download() ||
-           (*i)->GetRequestInfo()->is_stream() ||
-           (*i)->is_transferring());
+    // downloads, streams, detachable requests, and transferred requests are the
+    // only requests that aren't cancelled when the associated processes go
+    // away. It may be OK for this invariant to change in the future, but if
+    // this assertion fires without the invariant changing, then it's indicative
+    // of a leak.
+    DCHECK(
+        (*i)->GetRequestInfo()->is_download() ||
+        (*i)->GetRequestInfo()->is_stream() ||
+        (*i)->GetRequestInfo()->is_detachable() ||

[mmenke, 2013/10/17 18:33:48]

Wait...what's stopping us from putting detachable requests in this list?

[jkarlin2, 2013/10/24 15:33:11]

I don't understand, they are in the list?

On 2013/10/17 18:33:48, mmenke wrote:

[mmenke, 2013/10/24 20:27:20]

Sorry...What's stopping requests from being in the loaders_to_cancel list?

[jkarlin2, 2013/10/25 14:10:21]

At this point the renderers (and their attached requests) for the context should
have already disappeared, so all that we're left with to clean up are detached
requests, like downloads and detachable resources.

On 2013/10/24 20:27:20, mmenke wrote:

[mmenke, 2013/10/25 14:57:51]

Ah, right...I was misreading the code (I thought we were DCHECKing if this
wasn't true, not if it was).

+        (*i)->is_transferring());
   }
 #endif
 
@@ -694,14 +707,15 @@ void ResourceDispatcherHostImpl::DidReceiveRedirect(ResourceLoader* loader,
 
 void ResourceDispatcherHostImpl::DidReceiveResponse(ResourceLoader* loader) {
   ResourceRequestInfoImpl* info = loader->GetRequestInfo();
+
   // There should be an entry in the map created when we dispatched the
-  // request.
+  // request unless it's been detached and the renderer has died.
   OfflineMap::iterator policy_it(
       offline_policy_map_.find(info->GetGlobalRoutingID()));
   if (offline_policy_map_.end() != policy_it) {
     policy_it->second->UpdateStateForSuccessfullyStartedRequest(
         loader->request()->response_info());
-  } else {
+  } else if (!info->is_detachable()) {
     // We should always have an entry in offline_policy_map_ from when
     // this request traversed Begin{Download,SaveFile,Request}.
     // TODO(rdsmith): This isn't currently true; see http://crbug.com/241176.
@@ -1073,6 +1087,7 @@ void ResourceDispatcherHostImpl::BeginRequest(
           request_data.transition_type,
           false,  // is download
           false,  // is stream
+          false,  // is detachable
           allow_download,
           request_data.has_user_gesture,
           request_data.referrer_policy,
@@ -1101,8 +1116,11 @@ void ResourceDispatcherHostImpl::BeginRequest(
     handler.reset(new SyncResourceHandler(request, sync_result, this));
   } else {
     handler.reset(new AsyncResourceHandler(request, this));
+    if (IsDetachableResourceType(request_data.resource_type))
+      extra_info->set_is_detachable(true);
   }
 
+

[mmenke, 2013/10/17 18:33:48]

nit:  Remove extra line break.

[jkarlin2, 2013/10/24 15:33:11]

Done.

   // The RedirectToFileResourceHandler depends on being next in the chain.
   if (request_data.download_to_file) {
     handler.reset(
@@ -1220,6 +1238,7 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
       PAGE_TRANSITION_LINK,
       download,  // is_download
       false,  // is_stream
+      false,  // is_detachable
       download,  // allow_download
       false,     // has_user_gesture
       WebKit::WebReferrerPolicyDefault,
@@ -1315,8 +1334,8 @@ void ResourceDispatcherHostImpl::ResumeDeferredNavigation(
 }
 
 // The object died, so cancel and detach all requests associated with it except
-// for downloads, which belong to the browser process even if initiated via a
-// renderer.
+// for downloads and detachable resources, which belong to the browser process
+// even if initiated via a renderer.
 void ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
   CancelRequestsForRoute(child_id, -1 /* cancel all */);
   registered_temp_files_.erase(child_id);
@@ -1341,12 +1360,10 @@ void ResourceDispatcherHostImpl::CancelRequestsForRoute(int child_id,
 
     GlobalRequestID id(child_id, i->first.request_id);
     DCHECK(id == i->first);
-
-    // Don't cancel navigations that are transferring to another process,
-    // since they belong to another process now.
+    // Don't cancel navigations that are expected to live beyond this process.
     if (IsTransferredNavigation(id))
       any_requests_transferring = true;
-    if (!info->is_download() && !info->is_stream() &&
+    if (!info->is_download() && !info->is_stream() && !info->is_detachable() &&

[mmenke, 2013/10/17 18:33:48]

We aren't calling into DelayCancelForDetachable here, are we?  So we don't start
the timer, and don't listen to the command line flag?  Think we need more
testing of this.

[jkarlin2, 2013/10/24 15:33:11]

Done. Great catch. Fixed and tests added.

         !IsTransferredNavigation(id) &&
         (route_id == -1 || route_id == info->GetRouteID())) {
       matching_requests.push_back(id);