Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/download/download_manager_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.
 
 #include "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/debug/alias.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/supports_user_data.h"
 #include "base/synchronization/lock.h"
 #include "build/build_config.h"
 #include "content/browser/byte_stream.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_item_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/loader/throttling_resource_handler.h"

[Randy Smith (Not in Mondays), 2016/08/25 20:58:49]

Are these still needed?

[ananta, 2016/08/25 22:20:49]

Removed

 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/resource_context_impl.h"

[Randy Smith (Not in Mondays), 2016/08/25 20:58:49]

I think this could be replaced by a forward decl?

[ananta, 2016/08/25 22:20:49]

Removed

 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/browser/resource_throttle.h"

[Randy Smith (Not in Mondays), 2016/08/25 20:58:49]

Still needed?

[ananta, 2016/08/25 22:20:50]

Removed

 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/referrer.h"
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/load_flags.h"
 #include "net/base/request_priority.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/url_request/url_request_context.h"
 #include "storage/browser/blob/blob_url_request_job_factory.h"
 #include "url/origin.h"
 
@@ skipping 14 matching lines @@
   if (blob_data_handle) {
     storage::BlobProtocolHandler::SetRequestedBlobDataHandle(
         url_request.get(), std::move(blob_data_handle));
   }
 
   // If there's a valid renderer process associated with the request, then the
   // request should be driven by the ResourceLoader. Pass it over to the
   // ResourceDispatcherHostImpl which will in turn pass it along to the
   // ResourceLoader.
   if (params->render_process_host_id() >= 0) {
-    DownloadInterruptReason reason =
-        ResourceDispatcherHostImpl::Get()->BeginDownload(
-            std::move(url_request), params->referrer(),
-            params->content_initiated(), resource_context,
-            params->render_process_host_id(),
-            params->render_view_host_routing_id(),
-            params->render_frame_host_routing_id(),
-            params->do_not_prompt_for_login());
+    DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest(
+        std::move(url_request), params->referrer(), resource_context,
+        params->content_initiated(), params->render_process_host_id(),
+        params->render_view_host_routing_id(),
+        params->render_frame_host_routing_id(),
+        params->do_not_prompt_for_login());
 
     // If the download was accepted, the DownloadResourceHandler is now
     // responsible for driving the request to completion.
     if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
       return nullptr;
 
     // Otherwise, create an interrupted download.
     std::unique_ptr<DownloadCreateInfo> failed_created_info(
         new DownloadCreateInfo(base::Time::Now(), net::BoundNetLog(),
                                base::WrapUnique(new DownloadSaveInfo)));
@@ skipping 420 matching lines @@
 void DownloadManagerImpl::RemoveUrlDownloader(UrlDownloader* downloader) {
   for (auto ptr = url_downloaders_.begin(); ptr != url_downloaders_.end();
        ++ptr) {
     if (ptr->get() == downloader) {
       url_downloaders_.erase(ptr);
       return;
     }
   }
 }
 
+// static
+DownloadInterruptReason DownloadManagerImpl::BeginDownloadRequest(
+    std::unique_ptr<net::URLRequest> url_request,
+    const Referrer& referrer,
+    ResourceContext* resource_context,
+    bool is_content_initiated,
+    int render_process_id,
+    int render_view_route_id,
+    int render_frame_route_id,
+    bool do_not_prompt_for_login) {
+  if (ResourceDispatcherHostImpl::Get()->is_shutdown())
+    return DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
+
+  // The URLRequest needs to be initialized with the referrer and other
+  // information prior to issuing it.
+  ResourceDispatcherHostImpl::Get()->InitializeURLRequest(
+      url_request.get(), referrer,
+      true,  // download.
+      render_process_id, render_view_route_id, render_frame_route_id,
+      resource_context);
+
+  // 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?
+  url_request->set_first_party_url_policy(
+      net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
+
+  const GURL& url = url_request->original_url();
+
+  // 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;
+  }
+
+  const net::URLRequestContext* request_context = url_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;
+  }

[mmenke, 2016/08/25 21:03:09]

Seems like this check should probably also go in RDH, too.

[Randy Smith (Not in Mondays), 2016/08/25 21:17:45]

Yes, except that then BeginUrlRequest() would have to return the information
about why the request was rejected, which would require some sort of enum,
possibly created just for that function, enumerating the various reasons it
could fail.  That's why the is_shutdown() check is in this function as well;
that struck me as cleaner than creating a new enum just to return back the
failure information.  

WRT shutdown, I thought about repurposing net::Error, but that doesn't really
have a "this failed due to shutdown" return.   Though net::ERR_INVALID_URL might
serve here.

[ananta, 2016/08/25 22:20:50]

Will look into this in a followup patch

+
+  // From this point forward, the |DownloadResourceHandler| is responsible for
+  // |started_callback|.
+  // TODO(ananta)
+  // Find a better way to create the DownloadResourceHandler instance.
+  std::unique_ptr<ResourceHandler> handler(
+      DownloadResourceHandler::Create(url_request.get()));
+
+  ResourceDispatcherHostImpl::Get()->BeginURLRequest(
+      std::move(url_request), std::move(handler), true,  // download
+      is_content_initiated, do_not_prompt_for_login, resource_context);
+  return DOWNLOAD_INTERRUPT_REASON_NONE;
+}
+
 namespace {
 
 bool EmptyFilter(const GURL& url) {
   return true;
 }
 
 bool RemoveDownloadByURLAndTime(
     const base::Callback<bool(const GURL&)>& url_filter,
           base::Time remove_begin,
           base::Time remove_end,
@@ skipping 161 matching lines @@
   if (delegate_)
     delegate_->OpenDownload(download);
 }
 
 void DownloadManagerImpl::ShowDownloadInShell(DownloadItemImpl* download) {
   if (delegate_)
     delegate_->ShowDownloadInShell(download);
 }
 
 }  // namespace content