Revert "Click to Open PDF: Put placeholder HTML in IFRAMEs directly"

This reverts commit ad61fb27.

Reason for revert: New PDFPluginDisabledTest.IframePdfPlaceholderWithCSP test is failing in https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/5617



Original change's description:
> Click to Open PDF: Put placeholder HTML in IFRAMEs directly
> 
> Previously, when the user has a disabled PDF plugin (or no PDF plugin)
> and the website has an IFRAMEed PDF, when Click to Open PDF was enabled,
> we would inject a fake <object> tag into the IFRAME to force a PDF
> plugin placeholder to appear.
> 
> This approach is problematic with CSP, as CSP may forbid OBJECT tags
> from loading in IFRAMEs.
> 
> This CL instead injects the placeholder HTML directly into the IFRAME.
> 
> In the IFRAME case, the button is now a plain link, which should allow
> the user to click it and download the PDF even if CSP forbids <object>
> tags or JavaScript.
> 
> It still may trigger a CSP warning if IFRAME is prohibited from running
> JavaScript, but that only hurts keyboard-accessibility, and doesn't
> prevent the mainstream use case from working.
> 
> This CL also prevents the <object> tag from auto-opening the PDF after
> download, which was probably a mistake, since that overrides the user
> configurable "Always open with system viewer" option.
> 
> Bug: 887752, 879149, 878871
> Change-Id: I900c08331d2cfc96ea7cd1cd46ea594445b0920b
> Reviewed-on: https://chromium-review.googlesource.com/c/1246956
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Tommy Li <tommycli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597192}

TBR=thestig@chromium.org,tommycli@chromium.org,jochen@chromium.org,mkwst@chromium.org

Change-Id: I8a5554c0489ec76a4adb35ca93afef9c4354316e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 887752, 879149, 878871
Reviewed-on: https://chromium-review.googlesource.com/c/1265826Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597315}

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -47,7 +47,7 @@
 #include "chrome/common/buildflags.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_features.h"
-#include "chrome/common/pdf_util.h"
+#include "chrome/common/pdf_uma.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/safe_browsing/file_type_policies.h"
 #include "chrome/grit/generated_resources.h"

chrome/browser/pdf/pdf_extension_test.cc

@@ -77,7 +77,6 @@
 #include "ui/base/clipboard/clipboard.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/test/test_clipboard.h"
-#include "ui/events/keycodes/dom/keycode_converter.h"
 #include "ui/gfx/geometry/point.h"
 #include "url/gurl.h"
 
@@ -399,10 +398,15 @@ IN_PROC_BROWSER_TEST_P(PDFExtensionLoadTest, MAYBE_Load) {
   LoadAllPdfsTest("pdf", GetParam());
 }
 
-class DownloadAwaiter : public content::DownloadManager::Observer {
+class DisablePluginHelper : public content::DownloadManager::Observer {
  public:
-  DownloadAwaiter() {}
-  ~DownloadAwaiter() override {}
+  DisablePluginHelper() {}
+  ~DisablePluginHelper() override {}
+
+  void DisablePlugin(Profile* profile) {
+    profile->GetPrefs()->SetBoolean(
+        prefs::kPluginsAlwaysOpenPdfExternally, true);
+  }
 
   const GURL& GetLastUrl() {
     // Wait until the download has been created.
@@ -423,126 +427,33 @@ class DownloadAwaiter : public content::DownloadManager::Observer {
   GURL last_url_;
 };
 
-// Tests behavior when the PDF plugin is disabled in preferences.
-class PDFPluginDisabledTest : public PDFExtensionTest {
- public:
-  PDFPluginDisabledTest() {}
-
- protected:
-  void SetUpOnMainThread() override {
-    PDFExtensionTest::SetUpOnMainThread();
-
-    content::BrowserContext* browser_context =
-        GetActiveWebContents()->GetBrowserContext();
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, DisablePlugin) {
+  // Disable the PDF plugin.
+  WebContents* web_contents = GetActiveWebContents();
+  content::BrowserContext* browser_context = web_contents->GetBrowserContext();
   Profile* profile = Profile::FromBrowserContext(browser_context);
-    profile->GetPrefs()->SetBoolean(prefs::kPluginsAlwaysOpenPdfExternally,
-                                    true);
-
-    content::DownloadManager* download_manager =
-        content::BrowserContext::GetDownloadManager(browser_context);
-    download_awaiter_ = std::make_unique<DownloadAwaiter>();
-    download_manager->AddObserver(download_awaiter_.get());
-  }
+  DisablePluginHelper helper;
+  helper.DisablePlugin(profile);
 
-  void TearDownOnMainThread() override {
-    content::BrowserContext* browser_context =
-        GetActiveWebContents()->GetBrowserContext();
+  // Register a download observer.
   content::DownloadManager* download_manager =
       content::BrowserContext::GetDownloadManager(browser_context);
-    download_manager->RemoveObserver(download_awaiter_.get());
+  download_manager->AddObserver(&helper);
 
-    // Cancel all downloads to shut down cleanly.
-    std::vector<download::DownloadItem*> downloads;
-    download_manager->GetAllDownloads(&downloads);
-    for (auto* item : downloads) {
-      item->Cancel(false);
-    }
-
-    PDFExtensionTest::TearDownOnMainThread();
-  }
-
-  size_t GetNumberOfDownloads() {
-    content::BrowserContext* browser_context =
-        GetActiveWebContents()->GetBrowserContext();
-    content::DownloadManager* download_manager =
-        content::BrowserContext::GetDownloadManager(browser_context);
-
-    std::vector<download::DownloadItem*> downloads;
-    download_manager->GetAllDownloads(&downloads);
-    return downloads.size();
-  }
-
-  const GURL& AwaitAndGetLastDownloadedUrl() {
-    return download_awaiter_->GetLastUrl();
-  }
-
- private:
-  std::unique_ptr<DownloadAwaiter> download_awaiter_;
-};
-
-IN_PROC_BROWSER_TEST_F(PDFPluginDisabledTest, DirectNavigationToPDF) {
   // Navigate to a PDF and test that it is downloaded.
-  GURL pdf_url(embedded_test_server()->GetURL("/pdf/test.pdf"));
-  ui_test_utils::NavigateToURL(browser(), pdf_url);
-
-  // Validate that we downloaded a single PDF and didn't launch the PDF plugin.
-  EXPECT_EQ(pdf_url, AwaitAndGetLastDownloadedUrl());
-  EXPECT_EQ(1u, GetNumberOfDownloads());
-  EXPECT_EQ(0, CountPDFProcesses());
-}
-
-IN_PROC_BROWSER_TEST_F(PDFPluginDisabledTest, EmbedPdfPlaceholderWithCSP) {
-  // Navigate to a page with CSP that uses <embed> to embed a PDF as a plugin.
-  GURL embed_page_url =
-      embedded_test_server()->GetURL("/pdf/pdf_embed_csp.html");
-  ui_test_utils::NavigateToURL(browser(), embed_page_url);
-
-  // Fake a click on the <embed>, then press Enter to trigger the download.
-  gfx::Point point_in_pdf(100, 100);
-  content::SimulateRoutedMouseClickAt(
-      GetActiveWebContents(), kDefaultKeyModifier,
-      blink::WebMouseEvent::Button::kLeft, point_in_pdf);
-  content::SimulateKeyPress(GetActiveWebContents(), ui::DomKey::ENTER,
-                            ui::DomCode::ENTER, ui::VKEY_RETURN, false, false,
-                            false, false);
+  GURL url(embedded_test_server()->GetURL("/pdf/test.pdf"));
+  ui_test_utils::NavigateToURL(browser(), url);
+  ASSERT_EQ(url, helper.GetLastUrl());
 
-  // Validate that we downloaded a single PDF and didn't launch the PDF plugin.
-  GURL pdf_url(embedded_test_server()->GetURL("/pdf/test.pdf"));
-  EXPECT_EQ(pdf_url, AwaitAndGetLastDownloadedUrl());
-  EXPECT_EQ(1u, GetNumberOfDownloads());
+  // Didn't launch a PPAPI process.
   EXPECT_EQ(0, CountPDFProcesses());
-}
-
-IN_PROC_BROWSER_TEST_F(PDFPluginDisabledTest, IframePdfPlaceholderWithCSP) {
-  // Navigate to a page that uses <iframe> to embed a PDF as a plugin.
-  GURL iframe_page_url =
-      embedded_test_server()->GetURL("/pdf/pdf_iframe_csp.html");
-  ui_test_utils::NavigateToURL(browser(), iframe_page_url);
-
-  // Pass an Enter keystroke to the child <iframe>.
-  int keys_passed = 0;
-  for (auto* host : GetActiveWebContents()->GetAllFrames()) {
-    if (host != GetActiveWebContents()->GetMainFrame()) {
-      content::NativeWebKeyboardEvent key_event(
-          blink::WebKeyboardEvent::kRawKeyDown,
-          blink::WebInputEvent::kNoModifiers,
-          blink::WebInputEvent::GetStaticTimeStampForTests());
-      key_event.windows_key_code = ui::VKEY_RETURN;
-      key_event.native_key_code =
-          ui::KeycodeConverter::DomCodeToNativeKeycode(ui::DomCode::ENTER);
-      key_event.dom_code = static_cast<int>(ui::DomCode::ENTER);
-      key_event.dom_key = ui::DomKey::ENTER;
-      host->GetView()->GetRenderWidgetHost()->ForwardKeyboardEvent(key_event);
-      keys_passed++;
-    }
-  }
-  ASSERT_EQ(1, keys_passed);
 
-  // Validate that we downloaded a single PDF and didn't launch the PDF plugin.
-  GURL pdf_url(embedded_test_server()->GetURL("/pdf/test.pdf"));
-  EXPECT_EQ(pdf_url, AwaitAndGetLastDownloadedUrl());
-  EXPECT_EQ(1u, GetNumberOfDownloads());
-  EXPECT_EQ(0, CountPDFProcesses());
+  // Cancel the download to shutdown cleanly.
+  download_manager->RemoveObserver(&helper);
+  std::vector<download::DownloadItem*> downloads;
+  download_manager->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  downloads[0]->Cancel(false);
 }
 
 // We break PDFExtensionLoadTest up into kNumberLoadTestParts.

chrome/browser/plugins/pdf_iframe_navigation_throttle.cc

@@ -9,7 +9,7 @@
 #include "base/feature_list.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_features.h"
-#include "chrome/common/pdf_util.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"
@@ -91,7 +91,10 @@ PDFIFrameNavigationThrottle::WillProcessResponse() {
   if (!base::FeatureList::IsEnabled(features::kClickToOpenPDFPlaceholder))
     return content::NavigationThrottle::PROCEED;
 
-  std::string html = GetPDFPlaceholderHTML(navigation_handle()->GetURL());
+  std::string html = base::StringPrintf(
+      R"(<body style="margin: 0;"><object data="%s" type="application/pdf" )"
+      R"(style="width: 100%%; height: 100%%;"></object></body>)",
+      navigation_handle()->GetURL().spec().c_str());
   GURL data_url("data:text/html," + net::EscapePath(html));
 
   navigation_handle()->GetWebContents()->OpenURL(

chrome/browser/plugins/pdf_plugin_placeholder_observer.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "chrome/common/render_messages.h"
+#include "components/download/public/common/download_item.h"
 #include "components/download/public/common/download_url_parameters.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/child_process_security_policy.h"
@@ -19,6 +20,18 @@
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "ppapi/buildflags/buildflags.h"
 
+namespace {
+
+#if BUILDFLAG(ENABLE_PLUGINS)
+void OnDownloadStarted(download::DownloadItem* item,
+                       download::DownloadInterruptReason interrupt_reason) {
+  if (item && interrupt_reason == download::DOWNLOAD_INTERRUPT_REASON_NONE)
+    item->SetOpenWhenComplete(true);
+}
+#endif  // BUILDFLAG(ENABLE_PLUGINS)
+
+}  // namespace
+
 PDFPluginPlaceholderObserver::PDFPluginPlaceholderObserver(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents) {}
@@ -88,6 +101,7 @@ void PDFPluginPlaceholderObserver::OnOpenPDF(
   params->set_referrer(referrer.url);
   params->set_referrer_policy(
       content::Referrer::ReferrerPolicyForUrlRequest(referrer.policy));
+  params->set_callback(base::Bind(&OnDownloadStarted));
 
   content::BrowserContext::GetDownloadManager(
       web_contents()->GetBrowserContext())

chrome/common/BUILD.gn

@@ -165,8 +165,8 @@ static_library("common") {
     "page_load_metrics/page_load_timing.h",
     "page_load_metrics/page_track_decider.cc",
     "page_load_metrics/page_track_decider.h",
-    "pdf_util.cc",
-    "pdf_util.h",
+    "pdf_uma.cc",
+    "pdf_uma.h",
     "pref_names_util.cc",
     "pref_names_util.h",
     "prerender_messages.h",

chrome/common/DEPS

@@ -37,7 +37,6 @@ include_rules = [
   "+components/printing/common",
   "+components/safe_browsing/proto/csd.pb.h",
   "+components/safe_browsing/web_ui/constants.h",
-  "+components/strings/grit/components_strings.h",
   "+components/translate/core/common",
   "+components/url_formatter",
   "+components/variations/net",

chrome/common/pdf_uma.cc

+// Copyright 2017 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 "chrome/common/pdf_uma.h"
+
+#include "base/metrics/histogram_macros.h"
+
+void ReportPDFLoadStatus(PDFLoadStatus status) {
+  UMA_HISTOGRAM_ENUMERATION("PDF.LoadStatus", status,
+                            PDFLoadStatus::kPdfLoadStatusCount);
+}

chrome/common/pdf_util.h → chrome/common/pdf_uma.h

@@ -2,12 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_COMMON_PDF_UTIL_H_
-#define CHROME_COMMON_PDF_UTIL_H_
-
-#include <string>
-
-class GURL;
+#ifndef CHROME_COMMON_PDF_UMA_H_
+#define CHROME_COMMON_PDF_UMA_H_
 
 // Must be kept in sync with PDFLoadStatus enum in histograms.xml.
 // This enum should be treated as append-only.
@@ -23,9 +19,6 @@ enum class PDFLoadStatus {
 
 void ReportPDFLoadStatus(PDFLoadStatus status);
 
-// Returns the HTML contents of the placeholder.
-std::string GetPDFPlaceholderHTML(const GURL& pdf_url);
-
 constexpr char kPDFMimeType[] = "application/pdf";
 
-#endif  // CHROME_COMMON_PDF_UTIL_H_
+#endif  // CHROME_COMMON_PDF_UMA_H_

chrome/common/pdf_util.cc

-// Copyright 2017 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 "chrome/common/pdf_util.h"
-
-#include "base/metrics/histogram_macros.h"
-#include "chrome/grit/renderer_resources.h"
-#include "components/strings/grit/components_strings.h"
-#include "ui/base/l10n/l10n_util.h"
-#include "ui/base/resource/resource_bundle.h"
-#include "ui/base/webui/jstemplate_builder.h"
-#include "ui/base/webui/web_ui_util.h"
-#include "url/gurl.h"
-
-void ReportPDFLoadStatus(PDFLoadStatus status) {
-  UMA_HISTOGRAM_ENUMERATION("PDF.LoadStatus", status,
-                            PDFLoadStatus::kPdfLoadStatusCount);
-}
-
-std::string GetPDFPlaceholderHTML(const GURL& pdf_url) {
-  std::string template_html = ui::ResourceBundle::GetSharedInstance()
-                                  .GetRawDataResource(IDR_PDF_PLUGIN_HTML)
-                                  .as_string();
-  webui::AppendWebUiCssTextDefaults(&template_html);
-
-  base::DictionaryValue values;
-  values.SetString("fileName", pdf_url.ExtractFileName());
-  values.SetString("open", l10n_util::GetStringUTF8(IDS_ACCNAME_OPEN));
-  values.SetString("pdfUrl", pdf_url.spec());
-
-  return webui::GetI18nTemplateHtml(template_html, &values);
-}

chrome/renderer/chrome_content_renderer_client.cc

@@ -29,7 +29,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/constants.mojom.h"
 #include "chrome/common/crash_keys.h"
-#include "chrome/common/pdf_util.h"
+#include "chrome/common/pdf_uma.h"
 #include "chrome/common/pepper_permission_util.h"
 #include "chrome/common/plugin.mojom.h"
 #include "chrome/common/prerender_types.h"

chrome/renderer/plugins/pdf_plugin_placeholder.cc

@@ -4,21 +4,19 @@
 
 #include "chrome/renderer/plugins/pdf_plugin_placeholder.h"
 
-#include "chrome/common/pdf_util.h"
+#include "chrome/common/pdf_uma.h"
 #include "chrome/common/render_messages.h"
+#include "chrome/grit/renderer_resources.h"
+#include "components/strings/grit/components_strings.h"
 #include "content/public/renderer/render_thread.h"
 #include "gin/object_template_builder.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/base/webui/jstemplate_builder.h"
+#include "ui/base/webui/web_ui_util.h"
 
 gin::WrapperInfo PDFPluginPlaceholder::kWrapperInfo = {gin::kEmbedderNativeGin};
 
-// static
-PDFPluginPlaceholder* PDFPluginPlaceholder::CreatePDFPlaceholder(
-    content::RenderFrame* render_frame,
-    const blink::WebPluginParams& params) {
-  std::string html_data = GetPDFPlaceholderHTML(params.url);
-  return new PDFPluginPlaceholder(render_frame, params, html_data);
-}
-
 PDFPluginPlaceholder::PDFPluginPlaceholder(content::RenderFrame* render_frame,
                                            const blink::WebPluginParams& params,
                                            const std::string& html_data)
@@ -26,6 +24,22 @@ PDFPluginPlaceholder::PDFPluginPlaceholder(content::RenderFrame* render_frame,
 
 PDFPluginPlaceholder::~PDFPluginPlaceholder() {}
 
+PDFPluginPlaceholder* PDFPluginPlaceholder::CreatePDFPlaceholder(
+    content::RenderFrame* render_frame,
+    const blink::WebPluginParams& params) {
+  std::string template_html = ui::ResourceBundle::GetSharedInstance()
+                                  .GetRawDataResource(IDR_PDF_PLUGIN_HTML)
+                                  .as_string();
+  webui::AppendWebUiCssTextDefaults(&template_html);
+
+  base::DictionaryValue values;
+  values.SetString("fileName", GURL(params.url).ExtractFileName());
+  values.SetString("open", l10n_util::GetStringUTF8(IDS_ACCNAME_OPEN));
+  std::string html_data = webui::GetI18nTemplateHtml(template_html, &values);
+
+  return new PDFPluginPlaceholder(render_frame, params, html_data);
+}
+
 v8::Local<v8::Value> PDFPluginPlaceholder::GetV8Handle(v8::Isolate* isolate) {
   return gin::CreateHandle(isolate, this).ToV8();
 }

chrome/renderer/resources/plugins/pdf_plugin.html

@@ -5,21 +5,9 @@
     <meta name="viewport"
       content="initial-scale=1, minimum-scale=1, width=device-width">
     <script>
-      // If this page is embedded in a WebViewPlugin, the 'window.plugin'
-      // object is externally defined, and we trigger the download that way.
-      if (window.plugin !== undefined) {
-        window.addEventListener('DOMContentLoaded', function () {
-          let button = document.getElementById('open-button');
-          button.addEventListener('click', function(e) {
-            window.plugin.openPDF();
-            e.preventDefault();
-          });
-        });
-      }
-
       window.onkeydown = function(e) {
         if (e.key == 'Enter' || e.key == ' ') {
-          document.getElementById('open-button').click();
+          plugin.openPDF();
           e.preventDefault();
         }
       };
@@ -54,9 +42,7 @@
         <div id="main-message">
           <h1>$i18n{fileName}</h1>
         </div>
-        <a href="$i18n{pdfUrl}" target="_blank">
-          <button id="open-button" tabindex="1">$i18n{open}</button>
-        </a>
+        <button onclick="plugin.openPDF()">$i18n{open}</a>
       </div>
     </div>
   </body>

chrome/test/data/pdf/pdf_embed_csp.html

-<html>
-  <head>
-    <title>TabWithEmbeddedPdf</title>
-    <meta http-equiv="Content-Security-Policy"
-        content="default-src 'none'; img-src 'self'; object-src 'self'">
-  </head>
-  <body>
-    <embed type="application/pdf" src="test.pdf">
-  </body>
-</html>

chrome/test/data/pdf/pdf_iframe_csp.html

-<html>
-  <head>
-    <title>TabWithEmbeddedPdf</title>
-    <meta http-equiv="Content-Security-Policy" content="object-src 'none'">
-  </head>
-  <body>
-    <iframe src="test.pdf"></iframe>
-  </body>
-</html>