Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/download/download_resource_handler.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_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 88 matching lines @@
 
 void DeleteOnUIThread(
     std::unique_ptr<DownloadResourceHandler::DownloadTabInfo> tab_info) {}
 
 }  // namespace
 
 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() {
   if (tab_info_) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&DeleteOnUIThread, base::Passed(&tab_info_)));
   }
 }
 
@@ skipping 58 matching lines @@
   if (create_info->result == DOWNLOAD_INTERRUPT_REASON_USER_CANCELED &&
       create_info->download_id == DownloadItem::kInvalidId) {
     if (!callback.is_null())
       BrowserThread::PostTask(
           BrowserThread::UI, FROM_HERE,
           base::Bind(callback, nullptr, create_info->result));
     return;
   }
 
   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();
 
   create_info->request_handle.reset(new DownloadRequestHandle(
       AsWeakPtr(), request_info->GetWebContentsGetterForRequest()));
 
   int render_process_id = -1;
   int render_frame_id = -1;
   request_info->GetAssociatedRenderFrame(&render_process_id, &render_frame_id);
 
@@ skipping 32 matching lines @@
                             " }",
                             request() ?
                                 request()->url().spec().c_str() :
                                 "<NULL request>",
                             info->GetChildID(),
                             info->GetRequestID(),
                             info->GetRouteID());
 }
 
 }  // namespace content