Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 21 matching lines @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/time/time.h"
 #include "content/browser/appcache/appcache_interceptor.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/bad_message.h"
 #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"
 #include "content/browser/loader/async_resource_handler.h"
 #include "content/browser/loader/async_revalidation_manager.h"
 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/loader_delegate.h"
 #include "content/browser/loader/mime_type_resource_handler.h"
 #include "content/browser/loader/mojo_async_resource_handler.h"
@@ skipping 549 matching lines @@
   loaders_to_cancel.clear();
 
   if (async_revalidation_manager_) {
     // Cancelling async revalidations should not result in the creation of new
     // requests. Do it before the CHECKs to ensure this does not happen.
     async_revalidation_manager_->CancelAsyncRevalidationsForResourceContext(
         context);
   }
 }
 
-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);
   if (info) {
     ResourceLoader* loader = GetLoader(info->GetGlobalRequestID());
     if (loader)
       loader->ClearLoginDelegate();
   }
 }
 
@@ skipping 20 matching lines @@
                           FROM_HERE,
                           base::Bind(&ResourceDispatcherHostImpl::OnShutdown,
                                      base::Unretained(this)));
 }
 
 std::unique_ptr<ResourceHandler>
 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>
 ResourceDispatcherHostImpl::MaybeInterceptAsStream(
     const base::FilePath& plugin_path,
     net::URLRequest* request,
     ResourceResponse* response,
     std::string* payload) {
   payload->clear();
   ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request);
@@ skipping 693 matching lines @@
                              new URLRequestServiceWorkerData());
   }
 
   // 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) {
@@ skipping 122 matching lines @@
   AppCacheInterceptor::SetExtraRequestInfo(
       new_request.get(), filter_->appcache_service(), child_id,
       request_data.appcache_host_id, request_data.resource_type,
       request_data.should_reset_appcache);
 
   std::unique_ptr<ResourceHandler> handler(CreateResourceHandler(
       new_request.get(), request_data, sync_result, route_id, process_type,
       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>
 ResourceDispatcherHostImpl::CreateResourceHandler(
     net::URLRequest* request,
     const ResourceRequest& request_data,
     IPC::Message* sync_result,
     int route_id,
     int process_type,
     int child_id,
@@ skipping 216 matching lines @@
 
   loader->CancelRequest(true);
 }
 
 ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo(
     int child_id,
     int render_view_route_id,
     int render_frame_route_id,
     bool download,
     ResourceContext* context) {
+  request_id_--;
   return new ResourceRequestInfoImpl(
       PROCESS_TYPE_RENDERER,
       child_id,
       render_view_route_id,
       -1,  // frame_tree_node_id
       0,
       request_id_,
       render_frame_route_id,
       false,             // is_main_frame
       false,             // parent_is_main_frame
@@ skipping 12 matching lines @@
       context,
       base::WeakPtr<ResourceMessageFilter>(),  // filter
       false,                                   // report_raw_headers
       true,                                    // is_async
       false,                                   // is_using_lofi
       std::string(),                           // original_headers
       nullptr,                                 // body
       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);
 }
 
 void ResourceDispatcherHostImpl::OnRenderViewHostDeleted(int child_id,
                                                          int route_id) {
   scheduler_->OnClientDeleted(child_id, route_id);
 }
 
 void ResourceDispatcherHostImpl::OnRenderViewHostSetIsLoading(int child_id,
                                                               int route_id,
                                                               bool is_loading) {
   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) {
   GetLoader(id)->MarkAsTransferring(response);
 }
 
 void ResourceDispatcherHostImpl::CancelTransferringNavigation(
     const GlobalRequestID& id) {
   // Request should still exist and be in the middle of a transfer.
   DCHECK(IsTransferredNavigation(id));
@@ skipping 256 matching lines @@
 
   new_request->set_method(info.common_params.method);
   new_request->set_first_party_for_cookies(
       info.first_party_for_cookies);
   new_request->set_initiator(info.request_initiator);
   if (info.is_main_frame) {
     new_request->set_first_party_url_policy(
         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);
 
   new_request->SetLoadFlags(load_flags);
 
   storage::BlobStorageContext* blob_context = GetBlobStorageContext(
       GetChromeBlobStorageContextForResourceContext(resource_context));
 
   // Resolve elements from request_body and prepare upload data.
@@ skipping 76 matching lines @@
   // dependency on child_id/route_id. Those are used by the ResourceScheduler;
   // currently it's a no-op.
   handler =
       AddStandardHandlers(new_request.get(), resource_type, resource_context,
                           info.begin_params.request_context_type,
                           nullptr,  // appcache_service
                           -1,       // child_id
                           -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() {
   if (!async_revalidation_manager_)
     async_revalidation_manager_.reset(new AsyncRevalidationManager);
 }
 
 void ResourceDispatcherHostImpl::SetLoaderDelegate(
     LoaderDelegate* loader_delegate) {
   loader_delegate_ = loader_delegate;
@@ skipping 27 matching lines @@
   int strings_cost = request->extra_request_headers().ToString().size() +
                      request->original_url().spec().size() +
                      request->referrer().size() +
                      request->method().size();
 
   // Note that this expression will typically be dominated by:
   // |kAvgBytesPerOutstandingRequest|.
   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(
         request->load_flags() | net::LOAD_MAYBE_USER_GESTURE);
   }
 
   // Add the memory estimate that starting this request will consume.
   info->set_memory_cost(CalculateApproximateMemoryCost(request.get()));
 
   // If enqueing/starting this request will exceed our per-process memory
@@ skipping 341 matching lines @@
                        << iter->filesystem_url().spec();
           return false;
         }
       }
     }
   }
   return true;
 }
 
 }  // namespace content