Fix various issues in RedirectToFileResourceHandler.

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 bb2305b0d6a290611184c65687eef96f70157501..2829218adc9b612dff2df2f8425cfb36c881e1d0 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -40,6 +40,7 @@
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/loader/stream_resource_handler.h"
 #include "content/browser/loader/sync_resource_handler.h"
+#include "content/browser/loader/temporary_file_manager.h"
 #include "content/browser/loader/throttling_resource_handler.h"
 #include "content/browser/loader/upload_data_stream_builder.h"
 #include "content/browser/plugin_service_impl.h"
@@ -237,12 +238,6 @@ bool ShouldServiceRequest(int process_type,
   return true;
 }
 
-void RemoveDownloadFileFromChildSecurityPolicy(int child_id,
-                                               const base::FilePath& path) {
-  ChildProcessSecurityPolicyImpl::GetInstance()->RevokeAllPermissionsForFile(
-      child_id, path);
-}
-
 #if defined(OS_WIN)
 #pragma warning(disable: 4748)
 #pragma optimize("", off)
@@ -797,6 +792,7 @@ bool ResourceDispatcherHostImpl::RenderViewForRequest(
 
 void ResourceDispatcherHostImpl::OnInit() {
   scheduler_.reset(new ResourceScheduler);
+  temporary_file_manager_.reset(new TemporaryFileManager);
   appcache::AppCacheInterceptor::EnsureRegistered();
 }
 
@@ -828,6 +824,7 @@ void ResourceDispatcherHostImpl::OnShutdown() {
     CancelBlockedRequestsForRoute(iter->child_id, iter->route_id);
   }
 
+  temporary_file_manager_.reset();
   scheduler_.reset();
 }
 
@@ -1154,18 +1151,21 @@ scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::CreateResourceHandler(
     handler.reset(new SyncResourceHandler(request, sync_result, this));
   } else {
     handler.reset(new AsyncResourceHandler(request, this));
-    if (IsDetachableResourceType(request_data.resource_type)) {
-      handler.reset(new DetachableResourceHandler(
-          request,
-          base::TimeDelta::FromMilliseconds(kDefaultDetachableCancelDelayMs),
-          handler.Pass()));
-    }
   }
 
   // The RedirectToFileResourceHandler depends on being next in the chain.
   if (request_data.download_to_file) {
     handler.reset(
-        new RedirectToFileResourceHandler(handler.Pass(), request, this));
+        new RedirectToFileResourceHandler(handler.Pass(), request,
+                                          temporary_file_manager_.get()));
+  }
+
+  // Prefetches and <a ping> requests outlive their child process.

[darin (slow to review), 2013/12/06 17:56:07]

Is this part of a different CL?

[davidben, 2013/12/19 22:21:01]

I moved it out from the code above where AsyncResourceHandler is created.
Noticed that it results in RedirectToFileResourceHandler not being next in the
chain, so I swapped the order here. (Shouldn't ever happen, but if the renderer
decides to create a download_to_file prefetch for some incomprehensible
reason...) This way DetachableResourceHandler never gets its
OnWillRead/OnReadCompleted preempted in favor of OnDataDownloaded and it
preserves the behavior that RedirectToFileResourceHandler gets destroyed when
the request is cancelled via CancelRequestsForRoute.

+  if (!sync_result && IsDetachableResourceType(request_data.resource_type)) {
+    handler.reset(new DetachableResourceHandler(
+        request,
+        base::TimeDelta::FromMilliseconds(kDefaultDetachableCancelDelayMs),
+        handler.Pass()));
   }
 
   // Install a CrossSiteResourceHandler for all main frame requests.  This will
@@ -1206,45 +1206,19 @@ scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::CreateResourceHandler(
 }
 
 void ResourceDispatcherHostImpl::OnReleaseDownloadedFile(int request_id) {
-  UnregisterDownloadedTempFile(filter_->child_id(), request_id);
+  if (temporary_file_manager_) {
+    temporary_file_manager_->UnregisterDownloadedTempFile(filter_->child_id(),
+                                                          request_id);
+  } else {
+    // Can the RDH still receive messages after being shut down?
+    NOTREACHED();
+  }
 }
 
 void ResourceDispatcherHostImpl::OnDataDownloadedACK(int request_id) {
   // TODO(michaeln): maybe throttle DataDownloaded messages
 }
 
-void ResourceDispatcherHostImpl::RegisterDownloadedTempFile(
-    int child_id, int request_id, ShareableFileReference* reference) {
-  registered_temp_files_[child_id][request_id] = reference;
-  ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
-      child_id, reference->path());
-
-  // When the temp file is deleted, revoke permissions that the renderer has
-  // to that file. This covers an edge case where the file is deleted and then
-  // the same name is re-used for some other purpose, we don't want the old
-  // renderer to still have access to it.
-  //
-  // We do this when the file is deleted because the renderer can take a blob
-  // reference to the temp file that outlives the url loaded that it was
-  // loaded with to keep the file (and permissions) alive.
-  reference->AddFinalReleaseCallback(
-      base::Bind(&RemoveDownloadFileFromChildSecurityPolicy,
-                 child_id));
-}
-
-void ResourceDispatcherHostImpl::UnregisterDownloadedTempFile(
-    int child_id, int request_id) {
-  DeletableFilesMap& map = registered_temp_files_[child_id];
-  DeletableFilesMap::iterator found = map.find(request_id);
-  if (found == map.end())
-    return;
-
-  map.erase(found);
-
-  // Note that we don't remove the security bits here. This will be done
-  // when all file refs are deleted (see RegisterDownloadedTempFile).
-}
-
 bool ResourceDispatcherHostImpl::Send(IPC::Message* message) {
   delete message;
   return false;
@@ -1379,7 +1353,9 @@ void ResourceDispatcherHostImpl::ResumeDeferredNavigation(
 // even if initiated via a renderer.
 void ResourceDispatcherHostImpl::CancelRequestsForProcess(int child_id) {
   CancelRequestsForRoute(child_id, -1 /* cancel all */);
-  registered_temp_files_.erase(child_id);
+  // The RDH may already have been shutdown by the time this is called.
+  if (temporary_file_manager_)
+    temporary_file_manager_->UnregisterFilesForChild(child_id);
 }
 
 void ResourceDispatcherHostImpl::CancelRequestsForRoute(int child_id,