Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

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 847f0fd5f9d82e6dd5d54a4e4db3d75360c35fde..e03170d7bd01e202b5017d72af5a6afbaf0cb96f 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -39,8 +39,6 @@
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/cert_store_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
-#include "content/browser/download/download_resource_handler.h"
-#include "content/browser/download/save_file_resource_handler.h"

[scottmg, 2016/08/18 17:38:08]

And this one.

[ananta, 2016/08/18 22:27:09]

Done.

 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/frame_host/navigator.h"
@@ -610,72 +608,6 @@ void ResourceDispatcherHostImpl::CancelRequestsForContext(
   }
 }
 
-DownloadInterruptReason ResourceDispatcherHostImpl::BeginDownload(
-    std::unique_ptr<net::URLRequest> request,
-    const Referrer& referrer,
-    bool is_content_initiated,
-    ResourceContext* context,
-    int render_process_id,
-    int render_view_route_id,
-    int render_frame_route_id,
-    bool do_not_prompt_for_login) {
-  if (is_shutdown_)
-    return DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
-
-  const GURL& url = request->original_url();
-  SetReferrerForRequest(request.get(), referrer);
-
-  // We treat a download as a main frame load, and thus update the policy URL on
-  // redirects.
-  //
-  // TODO(davidben): Is this correct? If this came from a
-  // ViewHostMsg_DownloadUrl in a frame, should it have first-party URL set
-  // appropriately?
-  request->set_first_party_url_policy(
-      net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
-
-  // Check if the renderer is permitted to request the requested URL.
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->
-          CanRequestURL(render_process_id, url)) {
-    DVLOG(1) << "Denied unauthorized download request for "
-             << url.possibly_invalid_spec();
-    return DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST;
-  }
-
-  request_id_--;
-
-  const net::URLRequestContext* request_context = request->context();
-  if (!request_context->job_factory()->IsHandledURL(url)) {
-    DVLOG(1) << "Download request for unsupported protocol: "
-             << url.possibly_invalid_spec();
-    return DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST;

[Randy Smith (Not in Mondays), 2016/08/18 17:38:58]

This check seems to have been removed in the refactoring.  Why?

[ananta, 2016/08/18 22:27:09]

Thanks for catching this. Fixed

-  }
-
-  ResourceRequestInfoImpl* extra_info =
-      CreateRequestInfo(render_process_id, render_view_route_id,
-                        render_frame_route_id, true, context);
-  extra_info->set_do_not_prompt_for_login(do_not_prompt_for_login);
-  extra_info->AssociateWithRequest(request.get());  // Request takes ownership.
-
-  if (request->url().SchemeIs(url::kBlobScheme) &&
-      !storage::BlobProtocolHandler::GetRequestBlobDataHandle(request.get())) {
-    ChromeBlobStorageContext* blob_context =
-        GetChromeBlobStorageContextForResourceContext(context);
-    storage::BlobProtocolHandler::SetRequestedBlobDataHandle(
-        request.get(),
-        blob_context->context()->GetBlobDataFromPublicURL(request->url()));
-  }
-
-  // From this point forward, the |DownloadResourceHandler| is responsible for
-  // |started_callback|.
-  std::unique_ptr<ResourceHandler> handler(CreateResourceHandlerForDownload(
-      request.get(), is_content_initiated, true));
-
-  BeginRequestInternal(std::move(request), std::move(handler));
-
-  return DOWNLOAD_INTERRUPT_REASON_NONE;
-}
-
 void ResourceDispatcherHostImpl::ClearLoginDelegateForRequest(
     net::URLRequest* request) {
   ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request);
@@ -716,23 +648,11 @@ ResourceDispatcherHostImpl::CreateResourceHandlerForDownload(
     net::URLRequest* request,
     bool is_content_initiated,
     bool must_download) {
-  std::unique_ptr<ResourceHandler> handler(
-      new DownloadResourceHandler(request));
-  if (delegate_) {
-    const ResourceRequestInfoImpl* request_info(
-        ResourceRequestInfoImpl::ForRequest(request));
-
-    ScopedVector<ResourceThrottle> throttles;
-    delegate_->DownloadStarting(
-        request, request_info->GetContext(), request_info->GetChildID(),
-        request_info->GetRouteID(), is_content_initiated, must_download,
-        &throttles);
-    if (!throttles.empty()) {
-      handler.reset(new ThrottlingResourceHandler(std::move(handler), request,
-                                                  std::move(throttles)));
-    }
+  if (!create_download_handler_intercept_.is_null()) {
+    return create_download_handler_intercept_.Run(request, is_content_initiated,
+                                                  must_download);
   }
-  return handler;
+  return std::unique_ptr<ResourceHandler>();

[mmenke, 2016/08/18 17:35:43]

Returning a nullptr looks like it will result in a crash.  Maybe we should just
not let create_download_handler_intercept_ be null at this point?  If we're
going to crash anyways, we don't get anything by the extra logic here.

[Randy Smith (Not in Mondays), 2016/08/18 17:38:57]

I'm uncomfortable with this change.  Either we're in a situation where this
"shouldn't happen" (in which case let's have a DCHECK() and document in the
header that the interceptor must be set before any requests can be handled) or
if might happen, in which case I think we need to handle it better.  Unless
you're convinced that all the functions that call
CreateResourceHandlerForDownload() can handle a null resource handler?  If they
can't, then what we're doing is moving the failure from a place where it's
relatively clear what's happening (here) to one where it'll be less clear.

[ananta, 2016/08/18 22:27:09]

Sure thing. Added a DCHECK here

[ananta, 2016/08/18 22:27:09]

Added a DCHECK here

 }
 
 std::unique_ptr<ResourceHandler>
@@ -1446,9 +1366,6 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
         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 =
@@ -1591,8 +1508,12 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
       child_id, resource_context, std::move(mojo_request),
       std::move(url_loader_client)));
 
-  if (handler)
-    BeginRequestInternal(std::move(new_request), std::move(handler));
+  if (handler) {
+    BeginURLRequest(std::move(new_request),
+                    Referrer(request_data.referrer,
+                             request_data.referrer_policy),

[mmenke, 2016/08/18 17:35:43]

Moving where we set the referrer seems concerning - we've already passed the
URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler,
ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, and
all the standard handlers at this point.  Some certainly rely on
first_party_for_cookies.  Not sure if any depend on referrer or referrer policy,
but it seems like a bad idea to set anything on the request after we've already
let the world have a looksee at it.

[ananta, 2016/08/18 22:27:09]

Thanks. Fixed this by bringing back the old BeginRequestInternal function which
is called elsewhere in this file and from the new BeginURLRequest function.

[Randy Smith (Not in Mondays), 2016/08/21 23:32:57]

This looks to me to still have been changed on the BeginDownloadRequest path. 
In the old code, BeginDownload() set the referrer at the beginning of the
function and then called into the BlobStorageContext and protocol handler with
the request.  Now, DownloadManager::BeginDownloadRequest doesn't set the
referrer but still has the calls into the BlobStorageContext & etc.  Am I
missing something?

(Thanks for catching this, Matt.)

[ananta, 2016/08/22 19:14:57]

Yes this has changed. The current code  for context is here:
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
GetRequestBlobDataHandle does this
https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo...

SetRequestedBlobDataHandle does this
https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo...

The code is not inspecting the referrer. I think it should be safe?

[Randy Smith (Not in Mondays), 2016/08/23 19:54:45]

The issn't isn't about whether the code is currently inspecting the referrer,
it's whether we are making changes to the request after the point at which we
give external sections of the cod visibility into the request.  Another way to
think about it is that this change makes the blob code more fragile, because now
people making changes in the future need to know that they can't trust the
referrer that is visible in {Get,Set}RequestBlobHandle.  Maybe that's ok--it's
not obvious what blob handlers would care about the referrer.  But it's better
to keep dependency relationships like this to a minimum; the more we have the
more likely are bugs down the road.

Can you just promote SetReferrerForRequest to a (static) method on the RDHI and
call it from the initiating code sections where it used to be called?

+                    std::move(handler));
+  }
 }
 
 std::unique_ptr<ResourceHandler>
@@ -1829,6 +1750,7 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
     int render_frame_route_id,
     bool download,
     ResourceContext* context) {
+  request_id_--;
   return new ResourceRequestInfoImpl(
       PROCESS_TYPE_RENDERER,
       child_id,
@@ -1861,6 +1783,11 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
       false);                                  // initiated_in_secure_context
 }
 
+void ResourceDispatcherHostImpl::RegisterCreateDownloadHandlerInterceptor(
+    CreateDownloadHandlerIntercept intercept) {
+  create_download_handler_intercept_ = intercept;
+}
+
 void ResourceDispatcherHostImpl::OnRenderViewHostCreated(int child_id,
                                                          int route_id) {
   scheduler_->OnClientCreated(child_id, route_id);
@@ -1877,65 +1804,6 @@ void ResourceDispatcherHostImpl::OnRenderViewHostSetIsLoading(int child_id,
   scheduler_->OnLoadingStateChanged(child_id, route_id, !is_loading);
 }
 
-// This function is only used for saving feature.
-void ResourceDispatcherHostImpl::BeginSaveFile(const GURL& url,
-                                               const Referrer& referrer,
-                                               SaveItemId save_item_id,
-                                               SavePackageId save_package_id,
-                                               int child_id,
-                                               int render_view_route_id,
-                                               int render_frame_route_id,
-                                               ResourceContext* context) {
-  if (is_shutdown_)
-    return;
-
-  request_id_--;
-
-  const net::URLRequestContext* request_context = context->GetRequestContext();
-  bool known_proto =
-      request_context->job_factory()->IsHandledURL(url);
-  if (!known_proto) {
-    // Since any URLs which have non-standard scheme have been filtered
-    // by save manager(see GURL::SchemeIsStandard). This situation
-    // should not happen.
-    NOTREACHED();
-    return;
-  }
-
-  std::unique_ptr<net::URLRequest> request(
-      request_context->CreateRequest(url, net::DEFAULT_PRIORITY, NULL));
-  request->set_method("GET");
-  SetReferrerForRequest(request.get(), referrer);
-
-  // So far, for saving page, we need fetch content from cache, in the
-  // future, maybe we can use a configuration to configure this behavior.
-  request->SetLoadFlags(net::LOAD_PREFERRING_CACHE);
-
-  // Since we're just saving some resources we need, disallow downloading.
-  ResourceRequestInfoImpl* extra_info =
-      CreateRequestInfo(child_id, render_view_route_id,
-                        render_frame_route_id, false, context);
-  extra_info->AssociateWithRequest(request.get());  // Request takes ownership.
-
-  // Check if the renderer is permitted to request the requested URL.
-  using AuthorizationState = SaveFileResourceHandler::AuthorizationState;
-  AuthorizationState authorization_state = AuthorizationState::AUTHORIZED;
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(child_id,
-                                                                    url)) {
-    DVLOG(1) << "Denying unauthorized save of " << url.possibly_invalid_spec();
-    authorization_state = AuthorizationState::NOT_AUTHORIZED;
-    // No need to return here (i.e. okay to begin processing the request below),
-    // because NOT_AUTHORIZED will cause the request to be cancelled.  See also
-    // doc comments for AuthorizationState enum.
-  }
-
-  std::unique_ptr<SaveFileResourceHandler> handler(new SaveFileResourceHandler(
-      request.get(), save_item_id, save_package_id, child_id,
-      render_frame_route_id, url, authorization_state));
-
-  BeginRequestInternal(std::move(request), std::move(handler));
-}
-
 void ResourceDispatcherHostImpl::MarkAsTransferredNavigation(
     const GlobalRequestID& id,
     const scoped_refptr<ResourceResponse>& response) {
@@ -2212,8 +2080,6 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
         net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
   }
 
-  SetReferrerForRequest(new_request.get(), info.common_params.referrer);
-
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(info.begin_params.headers);
   new_request->SetExtraRequestHeaders(headers);
@@ -2310,7 +2176,9 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
                           -1,       // route_id
                           std::move(handler));
 
-  BeginRequestInternal(std::move(new_request), std::move(handler));
+  BeginURLRequest(std::move(new_request),
+                  info.common_params.referrer,
+                  std::move(handler));
 }
 
 void ResourceDispatcherHostImpl::EnableStaleWhileRevalidateForTesting() {
@@ -2358,13 +2226,17 @@ int ResourceDispatcherHostImpl::CalculateApproximateMemoryCost(
   return kAvgBytesPerOutstandingRequest + strings_cost;
 }
 
-void ResourceDispatcherHostImpl::BeginRequestInternal(
+void ResourceDispatcherHostImpl::BeginURLRequest(
     std::unique_ptr<net::URLRequest> request,
+    const Referrer& referrer,
     std::unique_ptr<ResourceHandler> handler) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!request->is_pending());
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request.get());
 
+  SetReferrerForRequest(request.get(), referrer);
+
   if ((TimeTicks::Now() - last_user_gesture_time_) <
       TimeDelta::FromMilliseconds(kUserGestureWindowMs)) {
     request->SetLoadFlags(