Fix an issue in download with network service.

Download code uses frame_tree_node_id to find WebContents when browser
side navigation is enabled. The WebContents is used in UI layer code
to close the tab when the request is a download.


Bug: 715630, 781565
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ib9490a64692027e3b16c70b77b54aceebcfcb9b7
Reviewed-on: https://chromium-review.googlesource.com/761269
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515816}

content/browser/download/download_manager_impl.cc

@@ -194,6 +194,7 @@ void InterceptNavigationResponse(
     const scoped_refptr<ResourceResponse>& response,
     mojo::ScopedDataPipeConsumerHandle consumer_handle,
     const SSLStatus& ssl_status,
+    int frame_tree_node_id,
     std::unique_ptr<ResourceRequest> resource_request,
     std::unique_ptr<ThrottlingURLLoader> url_loader,
     std::vector<GURL> url_chain,
@@ -202,8 +203,9 @@ void InterceptNavigationResponse(
   DownloadManagerImpl::UniqueUrlDownloadHandlerPtr resource_downloader(
       ResourceDownloader::InterceptNavigationResponse(
           download_manager, std::move(resource_request), response,
-          std::move(consumer_handle), ssl_status, std::move(url_loader),
-          std::move(url_chain), std::move(completion_status))
+          std::move(consumer_handle), ssl_status, frame_tree_node_id,
+          std::move(url_loader), std::move(url_chain),
+          std::move(completion_status))
           .release());
 
   BrowserThread::PostTask(
@@ -673,13 +675,14 @@ NavigationURLLoader::NavigationInterceptionCB
 DownloadManagerImpl::GetNavigationInterceptionCB(
     const scoped_refptr<ResourceResponse>& response,
     mojo::ScopedDataPipeConsumerHandle consumer_handle,
-    const SSLStatus& ssl_status) {
+    const SSLStatus& ssl_status,
+    int frame_tree_node_id) {
   return base::BindOnce(
       &InterceptNavigationResponse,
       base::BindOnce(&DownloadManagerImpl::AddUrlDownloadHandler,
                      weak_factory_.GetWeakPtr()),
       weak_factory_.GetWeakPtr(), response, std::move(consumer_handle),
-      ssl_status);
+      ssl_status, frame_tree_node_id);
 }
 
 int DownloadManagerImpl::RemoveDownloadsByURLAndTime(

content/browser/download/download_manager_impl.h

@@ -147,7 +147,8 @@ class CONTENT_EXPORT DownloadManagerImpl : public DownloadManager,
   NavigationURLLoader::NavigationInterceptionCB GetNavigationInterceptionCB(
       const scoped_refptr<ResourceResponse>& response,
       mojo::ScopedDataPipeConsumerHandle consumer_handle,
-      const SSLStatus& ssl_status);
+      const SSLStatus& ssl_status,
+      int frame_tree_node_id);
 
  private:
   using DownloadSet = std::set<DownloadItem*>;

content/browser/download/resource_downloader.cc

@@ -9,6 +9,7 @@
 #include "content/common/throttling_url_loader.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "storage/browser/fileapi/file_system_context.h"
 
 namespace content {
@@ -16,29 +17,24 @@ namespace content {
 // This class is only used for providing the WebContents to DownloadItemImpl.
 class RequestHandle : public DownloadRequestHandleInterface {
  public:
-  RequestHandle(int render_process_id, int render_frame_id)
-      : render_process_id_(render_process_id),
-        render_frame_id_(render_frame_id) {}
+  RequestHandle(int frame_tree_node_id)
+      : frame_tree_node_id_(frame_tree_node_id) {}
   RequestHandle(RequestHandle&& other)
-      : render_process_id_(other.render_process_id_),
-        render_frame_id_(other.render_frame_id_) {}
+      : frame_tree_node_id_(other.frame_tree_node_id_) {}
 
   // DownloadRequestHandleInterface
   WebContents* GetWebContents() const override {
-    RenderFrameHost* host =
-        RenderFrameHost::FromID(render_process_id_, render_frame_id_);
-    if (host)
-      return WebContents::FromRenderFrameHost(host);
-    return nullptr;
+    DCHECK(IsBrowserSideNavigationEnabled());
+    return WebContents::FromFrameTreeNodeId(frame_tree_node_id_);
   }
+
   DownloadManager* GetDownloadManager() const override { return nullptr; }
   void PauseRequest() const override {}
   void ResumeRequest() const override {}
   void CancelRequest(bool user_cancel) const override {}
 
  private:
-  int render_process_id_;
-  int render_frame_id_;
+  int frame_tree_node_id_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestHandle);
 };
@@ -73,6 +69,7 @@ ResourceDownloader::InterceptNavigationResponse(
     const scoped_refptr<ResourceResponse>& response,
     mojo::ScopedDataPipeConsumerHandle consumer_handle,
     const SSLStatus& ssl_status,
+    int frame_tree_node_id,
     std::unique_ptr<ThrottlingURLLoader> url_loader,
     std::vector<GURL> url_chain,
     base::Optional<ResourceRequestCompletionStatus> completion_status) {
@@ -80,10 +77,9 @@ ResourceDownloader::InterceptNavigationResponse(
       delegate, std::move(resource_request),
       std::make_unique<DownloadSaveInfo>(), content::DownloadItem::kInvalidId,
       std::string(), false, false, false);
-  downloader->InterceptResponse(std::move(url_loader), response,
-                                std::move(consumer_handle), ssl_status,
-                                std::move(url_chain),
-                                std::move(completion_status));
+  downloader->InterceptResponse(
+      std::move(url_loader), response, std::move(consumer_handle), ssl_status,
+      frame_tree_node_id, std::move(url_chain), std::move(completion_status));
   return downloader;
 }
 
@@ -142,12 +138,14 @@ void ResourceDownloader::InterceptResponse(
     const scoped_refptr<ResourceResponse>& response,
     mojo::ScopedDataPipeConsumerHandle consumer_handle,
     const SSLStatus& ssl_status,
+    int frame_tree_node_id,
     std::vector<GURL> url_chain,
     base::Optional<ResourceRequestCompletionStatus> completion_status) {
   url_loader_ = std::move(url_loader);
   url_loader_->set_forwarding_client(&response_handler_);
   net::SSLInfo info;
   info.cert_status = ssl_status.cert_status;
+  frame_tree_node_id_ = frame_tree_node_id;
   response_handler_.SetURLChain(std::move(url_chain));
   response_handler_.OnReceiveResponse(response->head,
                                       base::Optional<net::SSLInfo>(info),
@@ -163,8 +161,8 @@ void ResourceDownloader::OnResponseStarted(
   download_create_info->download_id = download_id_;
   download_create_info->guid = guid_;
   if (resource_request_->origin_pid >= 0) {
-    download_create_info->request_handle.reset(new RequestHandle(
-        resource_request_->origin_pid, resource_request_->render_frame_id));
+    download_create_info->request_handle.reset(
+        new RequestHandle(frame_tree_node_id_));
   }
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,

content/browser/download/resource_downloader.h

@@ -38,6 +38,7 @@ class ResourceDownloader : public UrlDownloadHandler,
       const scoped_refptr<ResourceResponse>& response,
       mojo::ScopedDataPipeConsumerHandle consumer_handle,
       const SSLStatus& ssl_status,
+      int frame_tree_node_id,
       std::unique_ptr<ThrottlingURLLoader> url_loader,
       std::vector<GURL> url_chain,
       base::Optional<ResourceRequestCompletionStatus> completion_status);
@@ -72,6 +73,7 @@ class ResourceDownloader : public UrlDownloadHandler,
       const scoped_refptr<ResourceResponse>& response,
       mojo::ScopedDataPipeConsumerHandle consumer_handle,
       const SSLStatus& ssl_status,
+      int frame_tree_node_id,
       std::vector<GURL> url_chain,
       base::Optional<ResourceRequestCompletionStatus> completion_status);
 
@@ -99,6 +101,9 @@ class ResourceDownloader : public UrlDownloadHandler,
   // Callback to run after download starts.
   DownloadUrlParameters::OnStartedCallback callback_;
 
+  // Used to get WebContents in browser process.
+  int frame_tree_node_id_ = 0;
+
   base::WeakPtrFactory<ResourceDownloader> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceDownloader);

content/browser/frame_host/navigation_request.cc

@@ -1124,7 +1124,8 @@ void NavigationRequest::OnWillProcessResponseChecksComplete(
           BrowserContext::GetDownloadManager(browser_context));
       loader_->InterceptNavigation(
           download_manager->GetNavigationInterceptionCB(
-              response_, std::move(handle_), ssl_status_));
+              response_, std::move(handle_), ssl_status_,
+              frame_tree_node_->frame_tree_node_id()));
       OnRequestFailed(false, net::ERR_ABORTED, base::nullopt, false);
       return;
     }

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -926,11 +926,6 @@
 -ReferrerPolicyTest.HttpsContextMenuOrigin
 -ReferrerPolicyTest.HttpsContextMenuRedirect
 
-# crbug.com/781565
--DownloadTest.CloseNewTab1
--DownloadTest.CloseNewTab2
--DownloadTest.CloseNewTab3
-
 # http://crbug.com/783990
 # Add support for http auth.
 -LoginPromptBrowserTest.AllowCrossdomainPromptForSubframes