Fix downloading pdfs with network service.

When the network service is enabled, we wouldn't download responses
that had a plugin associated with their mime type. This makes the
PluginResponseInterceptorUrlLoaderThrottle check if the response
is meant to be a download before intercepting it.

Bug: 855539
Change-Id: I8b70abb2a7e24c27b5a6866a0f09c24f59c12e05
Reviewed-on: https://chromium-review.googlesource.com/1119581
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572399}

chrome/browser/plugins/pdf_iframe_navigation_throttle.cc

@@ -9,12 +9,12 @@
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pdf_uma.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/download_utils.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/escape.h"
-#include "net/http/http_content_disposition.h"
 #include "net/http/http_response_headers.h"
 #include "ppapi/buildflags/buildflags.h"
 
@@ -77,13 +77,10 @@ PDFIFrameNavigationThrottle::WillProcessResponse() {
   if (mime_type != kPDFMimeType)
     return content::NavigationThrottle::PROCEED;
 
-  // Following the same logic as navigational_loader_util, we MUST download
-  // responses marked as attachments rather than showing a placeholder.
-  std::string disposition;
-  if (response_headers->GetNormalizedHeader("content-disposition",
-                                            &disposition) &&
-      !disposition.empty() &&
-      net::HttpContentDisposition(disposition, std::string()).is_attachment()) {
+  // We MUST download responses marked as attachments rather than showing
+  // a placeholder.
+  if (content::download_utils::MustDownload(navigation_handle()->GetURL(),
+                                            response_headers, mime_type)) {
     return content::NavigationThrottle::PROCEED;
   }
 

chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/extensions/api/streams_private/streams_private_api.h"
 #include "chrome/browser/plugins/plugin_utils.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_utils.h"
 #include "content/public/browser/stream_info.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/common/transferrable_url_loader.mojom.h"
@@ -29,6 +30,11 @@ void PluginResponseInterceptorURLLoaderThrottle::WillProcessResponse(
     const GURL& response_url,
     const network::ResourceResponseHead& response_head,
     bool* defer) {
+  if (content::download_utils::MustDownload(
+          response_url, response_head.headers.get(), response_head.mime_type)) {
+    return;
+  }
+
   std::string extension_id = PluginUtils::GetExtensionIdForMimeType(
       resource_context_, response_head.mime_type);
   if (extension_id.empty())

chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle_browsertest.cc

+// Copyright 2018 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 "base/files/file_path.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/common/chrome_content_client.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_manager.h"
+#include "content/public/browser/plugin_service.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "services/network/public/cpp/features.h"
+#include "url/gurl.h"
+
+using content::WebContents;
+
+class PluginResponseInterceptorURLLoaderThrottleBrowserTest
+    : public extensions::ExtensionApiTest {
+ public:
+  PluginResponseInterceptorURLLoaderThrottleBrowserTest() {}
+  ~PluginResponseInterceptorURLLoaderThrottleBrowserTest() override {}
+
+  void SetUpOnMainThread() override {
+    extensions::ExtensionApiTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+    base::FilePath test_data_dir;
+    base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
+    embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
+    embedded_test_server()->StartAcceptingConnections();
+  }
+
+  void TearDownOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete());
+    extensions::ExtensionApiTest::TearDownOnMainThread();
+  }
+
+  WebContents* GetActiveWebContents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  int CountPDFProcesses() {
+    int result = -1;
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&PluginResponseInterceptorURLLoaderThrottleBrowserTest::
+                           CountPDFProcessesOnIOThread,
+                       base::Unretained(this), base::Unretained(&result)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+    return result;
+  }
+
+  void CountPDFProcessesOnIOThread(int* result) {
+    auto* service = content::PluginService::GetInstance();
+    *result = service->CountPpapiPluginProcessesForProfile(
+        base::FilePath::FromUTF8Unsafe(ChromeContentClient::kPDFPluginPath),
+        browser()->profile()->GetPath());
+  }
+};
+
+class DownloadObserver : public content::DownloadManager::Observer {
+ public:
+  DownloadObserver() {}
+  ~DownloadObserver() override {}
+
+  const GURL& GetLastUrl() {
+    // Wait until the download has been created.
+    download_run_loop_.Run();
+    return last_url_;
+  }
+
+  // content::DownloadManager::Observer implementation.
+  void OnDownloadCreated(content::DownloadManager* manager,
+                         download::DownloadItem* item) override {
+    last_url_ = item->GetURL();
+    download_run_loop_.Quit();
+  }
+
+ private:
+  content::NotificationRegistrar registrar_;
+  base::RunLoop download_run_loop_;
+  GURL last_url_;
+};
+
+IN_PROC_BROWSER_TEST_F(PluginResponseInterceptorURLLoaderThrottleBrowserTest,
+                       DownloadPdf) {
+  // Register a download observer.
+  WebContents* web_contents = GetActiveWebContents();
+  content::BrowserContext* browser_context = web_contents->GetBrowserContext();
+  content::DownloadManager* download_manager =
+      content::BrowserContext::GetDownloadManager(browser_context);
+  DownloadObserver download_observer;
+  download_manager->AddObserver(&download_observer);
+
+  // Navigate to a PDF that's marked as an attachment and test that it
+  // is downloaded.
+  GURL url(embedded_test_server()->GetURL("/download.pdf"));
+  ui_test_utils::NavigateToURL(browser(), url);
+  ASSERT_EQ(url, download_observer.GetLastUrl());
+
+  // Didn't launch a PPAPI process.
+  EXPECT_EQ(0, CountPDFProcesses());
+
+  // Cancel the download to shutdown cleanly.
+  download_manager->RemoveObserver(&download_observer);
+  std::vector<download::DownloadItem*> downloads;
+  download_manager->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  downloads[0]->Cancel(false);
+}

chrome/test/BUILD.gn

@@ -654,6 +654,7 @@ test("browser_tests") {
       "../browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc",
       "../browser/plugins/flash_permission_browsertest.cc",
       "../browser/plugins/plugin_power_saver_browsertest.cc",
+      "../browser/plugins/plugin_response_interceptor_url_loader_throttle_browsertest.cc",
       "../browser/policy/cloud/cloud_policy_browsertest.cc",
       "../browser/policy/cloud/cloud_policy_manager_browsertest.cc",
       "../browser/policy/cloud/cloud_policy_test_utils.cc",

chrome/test/data/download.pdf.mock-http-headers

+HTTP/1.1 200 OK
+content-type: application/pdf
+content-disposition: attachment; filename="download.pdf"

content/browser/BUILD.gn

@@ -984,6 +984,8 @@ jumbo_source_set("browser") {
     "loader/data_pipe_to_source_stream.h",
     "loader/detachable_resource_handler.cc",
     "loader/detachable_resource_handler.h",
+    "loader/download_utils_impl.cc",
+    "loader/download_utils_impl.h",
     "loader/global_routing_id.h",
     "loader/intercepting_resource_handler.cc",
     "loader/intercepting_resource_handler.h",
@@ -1000,8 +1002,6 @@ jumbo_source_set("browser") {
     "loader/mojo_async_resource_handler.h",
     "loader/navigation_loader_interceptor.cc",
     "loader/navigation_loader_interceptor.h",
-    "loader/navigation_loader_util.cc",
-    "loader/navigation_loader_util.h",
     "loader/navigation_url_loader.cc",
     "loader/navigation_url_loader.h",
     "loader/navigation_url_loader_delegate.h",

content/browser/devtools/devtools_url_interceptor_request_job.cc

@@ -9,7 +9,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/devtools/protocol/network_handler.h"
 #include "content/browser/devtools/protocol/page.h"
-#include "content/browser/loader/navigation_loader_util.h"
+#include "content/browser/loader/download_utils_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "ipc/ipc_channel.h"
 #include "net/base/completion_once_callback.h"
@@ -552,8 +552,8 @@ bool IsDownload(net::URLRequest* orig_request, net::URLRequest* subrequest) {
   std::string mime_type;
   subrequest->GetMimeType(&mime_type);
   return req_info->allow_download() &&
-         navigation_loader_util::IsDownload(
-             orig_request->url(), subrequest->response_headers(), mime_type);
+         download_utils::IsDownload(orig_request->url(),
+                                    subrequest->response_headers(), mime_type);
 }
 
 }  // namespace

content/browser/devtools/devtools_url_loader_interceptor.cc

@@ -10,7 +10,7 @@
 #include "base/unguessable_token.h"
 #include "content/browser/devtools/protocol/network_handler.h"
 #include "content/browser/frame_host/frame_tree_node.h"
-#include "content/browser/loader/navigation_loader_util.h"
+#include "content/browser/loader/download_utils_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/system/data_pipe_drainer.h"
@@ -1154,7 +1154,7 @@ void InterceptionJob::OnReceiveResponse(
   const network::ResourceRequest& request = create_loader_params_->request;
   request_info->is_download =
       request_info->is_navigation && request.allow_download &&
-      (is_download_ || navigation_loader_util::IsDownload(
+      (is_download_ || download_utils::IsDownload(
                            request.url, head.headers.get(), head.mime_type));
   NotifyClient(std::move(request_info));
 }

content/browser/loader/navigation_loader_util.cc → content/browser/loader/download_utils_impl.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/loader/navigation_loader_util.h"
+#include "content/browser/loader/download_utils_impl.h"
 
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
@@ -10,13 +10,12 @@
 #include "net/http/http_response_headers.h"
 #include "third_party/blink/public/common/mime_util/mime_util.h"
 #include "url/gurl.h"
-#include "url/origin.h"
 
 namespace content {
-namespace navigation_loader_util {
+namespace download_utils {
 
 bool MustDownload(const GURL& url,
-                  net::HttpResponseHeaders* headers,
+                  const net::HttpResponseHeaders* headers,
                   const std::string& mime_type) {
   if (headers) {
     std::string disposition;
@@ -43,7 +42,7 @@ bool MustDownload(const GURL& url,
 }
 
 bool IsDownload(const GURL& url,
-                net::HttpResponseHeaders* headers,
+                const net::HttpResponseHeaders* headers,
                 const std::string& mime_type) {
   if (MustDownload(url, headers, mime_type))
     return true;
@@ -54,5 +53,5 @@ bool IsDownload(const GURL& url,
   return !headers || headers->response_code() / 100 == 2;
 }
 
-}  // namespace navigation_loader_util
+}  // namespace download_utils
 }  // namespace content

content/browser/loader/navigation_loader_util.h → content/browser/loader/download_utils_impl.h

@@ -2,33 +2,21 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CONTENT_BROWSER_LOADER_NAVIGATION_LOADER_UTIL_H_
-#define CONTENT_BROWSER_LOADER_NAVIGATION_LOADER_UTIL_H_
+#ifndef CONTENT_BROWSER_LOADER_DOWNLOAD_UTILS_IMPL_H_
+#define CONTENT_BROWSER_LOADER_DOWNLOAD_UTILS_IMPL_H_
 
-#include "base/optional.h"
-
-#include <string>
-
-class GURL;
-namespace net {
-class HttpResponseHeaders;
-}
+#include "content/public/browser/download_utils.h"
 
 namespace content {
-namespace navigation_loader_util {
-
-// Returns true if the given response must be downloaded because of the headers.
-bool MustDownload(const GURL& url,
-                  net::HttpResponseHeaders* headers,
-                  const std::string& mime_type);
+namespace download_utils {
 
 // Determines whether given response would result in a download.
 // Note this doesn't handle the case when a plugin exists for the |mime_type|.
 bool IsDownload(const GURL& url,
-                net::HttpResponseHeaders* headers,
+                const net::HttpResponseHeaders* headers,
                 const std::string& mime_type);
 
-}  // namespace navigation_loader_util
+}  // namespace download_utils
 }  // namespace content
 
-#endif  // CONTENT_BROWSER_LOADER_NAVIGATION_LOADER_UTIL_H_
+#endif  // CONTENT_BROWSER_LOADER_DOWNLOAD_UTILS_IMPL_H_

content/browser/loader/navigation_url_loader_impl.cc

@@ -22,7 +22,6 @@
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/loader/navigation_loader_interceptor.h"
-#include "content/browser/loader/navigation_loader_util.h"
 #include "content/browser/loader/navigation_url_loader_delegate.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
@@ -45,6 +44,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/download_utils.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/navigation_data.h"
 #include "content/public/browser/navigation_ui_data.h"
@@ -899,7 +899,7 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
     bool is_stream;
     std::unique_ptr<NavigationData> cloned_navigation_data;
     if (IsLoaderInterceptionEnabled()) {
-      bool must_download = navigation_loader_util::MustDownload(
+      bool must_download = download_utils::MustDownload(
           url_, head.headers.get(), head.mime_type);
       bool known_mime_type = blink::IsSupportedMimeType(head.mime_type);
 

content/public/browser/BUILD.gn

@@ -121,6 +121,7 @@ jumbo_source_set("browser_sources") {
     "download_manager_delegate.cc",
     "download_manager_delegate.h",
     "download_request_utils.h",
+    "download_utils.h",
     "favicon_status.cc",
     "favicon_status.h",
     "file_url_loader.h",

content/public/browser/download_utils.h

+// Copyright 2018 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.
+
+#ifndef CONTENT_PUBLIC_BROWSER_DOWNLOAD_UTILS_H_
+#define CONTENT_PUBLIC_BROWSER_DOWNLOAD_UTILS_H_
+
+#include "content/common/content_export.h"
+
+#include <string>
+
+class GURL;
+namespace net {
+class HttpResponseHeaders;
+}
+
+namespace content {
+namespace download_utils {
+
+// Returns true if the given response must be downloaded because of the headers.
+CONTENT_EXPORT bool MustDownload(const GURL& url,
+                                 const net::HttpResponseHeaders* headers,
+                                 const std::string& mime_type);
+
+}  // namespace download_utils
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_DOWNLOAD_UTILS_H_