Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/download/download_resource_handler.cc

Index: content/browser/download/download_resource_handler.cc
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 05d22a8ee73cfe46f093aa7503646ed3a2bb6f98..a224b325f8891bb567774fac15eb05601e71a3d6 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -106,19 +106,6 @@ DownloadResourceHandler::DownloadResourceHandler(net::URLRequest* request)
     : ResourceHandler(request),
       tab_info_(new DownloadTabInfo()),
       core_(request, this) {
-  // Do UI thread initialization for tab_info_ asap after
-  // DownloadResourceHandler creation since the tab could be navigated
-  // before StartOnUIThread gets called.  This is safe because deletion
-  // will occur via PostTask() as well, which will serialized behind this
-  // PostTask()
-  const ResourceRequestInfoImpl* request_info = GetRequestInfo();
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(
-          &InitializeDownloadTabInfoOnUIThread,
-          DownloadRequestHandle(AsWeakPtr(),
-                                request_info->GetWebContentsGetterForRequest()),
-          tab_info_.get()));
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
@@ -197,6 +184,20 @@ void DownloadResourceHandler::OnStart(
   }
 
   const ResourceRequestInfoImpl* request_info = GetRequestInfo();
+  // Post the task (InitializeDownloadTabInfoOnUIThread) to initialize the
+  // tab_info_ structure on the UI thread before posting the StartOnUIThread()
+  // task. The tab could be navigated away before either of these tasks run,
+  // which is safe because the deletion of the tab_info_ structure also occurs
+  // on the UI thread via PostTask() which will be serialized behind these
+  // tasks.

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

I'm sorry, I don't understand what drove moving this.  Was there a problem with
where it was?

The problem with moving it is that it gives more time for the user (or malicious
code) to navigate the tab away and thus make the tab_info_ invalid/unuseful.  So
it's better to get the information as quickly as possible when the download
starts (i.e. triggered from the constructor).

The "This is safe" in the original comment was about why it was ok to pass a raw
pointer to a member variable of an object that lives on the IO thread to a
different thread (i.e. why that raw pointer would stay valid no matter what
thread races occurred).   It wasn't about why navigating away was safe;
navigating away will just result in a null web_contents, which will result in
the tab information not being set.

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

The ResourceRequestInfoImpl instance which is associated with the original
request is not available here in all cases. For normal downloads which are
detected via the mime type, yes. However for downloads initiated via the context
menu for e.g. Save As, or Save image as, etc the request is initiated eventually
via the DownloadUrl function in download_manager_impl.cc. OnStart gets called in
all cases, success/failure and hence seemed like the right place to do this.

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

So the code is squirrelly, so I may be mistaken, but I just spent some time
tracing code, and DownloadUrl in download_manager_impl.cc will never go through
this code.  It does have anything to do with the DownloadResourceHandler, and
instead creates a UrlDownloader to handle the job of getting bits from the
URLRequest to the download system.  So by my reading Save As/Save Image as will
never go through this code, so there's no point moving it for them.  What am I
missing?

+  BrowserThread::PostTask(
+    BrowserThread::UI, FROM_HERE,
+    base::Bind(
+    &InitializeDownloadTabInfoOnUIThread,
+    DownloadRequestHandle(
+    AsWeakPtr(), request_info->GetWebContentsGetterForRequest()),
+    tab_info_.get()));
+
   create_info->has_user_gesture = request_info->HasUserGesture();
   create_info->transition_type = request_info->GetPageTransition();