Thanks for the report.  My CL changed the process model behavior for some data: URLs, to fix a security bug with process sharing.  I'm not sure yet why that would interfere with this feature.

It looks like the PDF placeholder page is indeed a data: URL, but it's not clear why the button would stop working.  How is it implemented?  Can you point me to where to look?  Do you know if it depends on any special bindings?

Also, when was the feature enabled by default (in terms of which versions are affected)?  Before or after M69, when the change landed?

Hi - the placeholder page for IFRAMES is implemented with a navigation throttle that replaces the IFRAME with a stand-in <object> tag here:

https://cs.chromium.org/chromium/src/chrome/browser/plugins/pdf_iframe_navigation_throttle.cc?type=cs&q=pdf_iframe&g=0&l=94

When the user clicks the placeholder's OPEN button, it starts a download here:
https://cs.chromium.org/chromium/src/chrome/renderer/plugins/pdf_plugin_placeholder.cc?q=pdf_plugin_plac&sq=package:chromium&g=0&l=57
continues here:
https://cs.chromium.org/chromium/src/chrome/browser/plugins/pdf_plugin_placeholder_observer.cc?type=cs&sq=package:chromium&g=0&l=53

This feature was enabled by default for M68 a week ago. So likely M69 will be affected as well.

Thanks.  Looks like that launch was issue 799085?  Does this only affect users who enable chrome://settings/content/pdfDocuments manually / by policy?  (Just wondering if we should disable the feature until we work this out, if the button isn't working at all right now.  My change isn't something we can turn off via Finch, and it might take a while to get a fix merged.)

As for the behavior difference, it does look like my CL caused the data URL to load in an OOPIF unlike before, since it's a browser-initiated subframe navigation in PDFIFrameNavigationThrottle::WillProcessResponse (which I find a bit surprising):

  navigation_handle()->GetWebContents()->OpenURL(
      content::OpenURLParams(data_url, navigation_handle()->GetReferrer(),
                             navigation_handle()->GetFrameTreeNodeId(),
                             WindowOpenDisposition::CURRENT_TAB,
                             ui::PAGE_TRANSITION_AUTO_SUBFRAME, false));

I'm more puzzled why that affects whether the button works, though.

Oh, I see it.  We do get to PDFPluginPlaceholderObserver::OnOpenPDF when you click the button even when it's an OOPIF, but there's an OOPIF-related bug in the DownloadUrlParameters construction:

  std::unique_ptr<download::DownloadUrlParameters> params =
      std::make_unique<download::DownloadUrlParameters>(
          url, web_contents()->GetRenderViewHost()->GetProcess()->GetID(),
          web_contents()->GetRenderViewHost()->GetRoutingID(),
          render_frame_host->GetRoutingID(),
          storage_partition->GetURLRequestContext(), traffic_annotation);

We're mixing the top-level frame's RVH (process ID, routing ID) with the subframe's routing ID (in a different process).  There's a warning against this on the constructor, but I guess we just haven't hit this case before to find and fix it.  :)
https://cs.chromium.org/chromium/src/components/download/public/common/download_url_parameters.h?rcl=5f22b6409fd0b68743f43690e1c47abcc66e9c21&l=84

Changing web_contents() to render_frame_host in that code seems to make it work.  Do you know how to add a test for this?  I threw together a quick CL here, but feel free to toss it into another CL with a test if so.

https://chromium-review.googlesource.com/c/chromium/src/+/1212145

Only affects users with that setting enabled on Desktop only (Android takes a different codepath).

We should try to merge this change backwards as far as the TPMs will allow us.

But I can remotely Finch-kill the feature for Desktop users who are "left behind" on Monday.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f9f7845019e6d4aa6f157b43ec6a45564cc50aa

commit 3f9f7845019e6d4aa6f157b43ec6a45564cc50aa
Author: Charlie Reis <creis@chromium.org>
Date: Fri Sep 07 17:14:53 2018

Use matching RVH/RFH in DownloadUrlParameters for PDF plugin placeholder.

BUG= 881497 
TEST=See bug for repro steps.

Change-Id: I392788d31e638bc79bb8d35e0451c79af6ca3a26
Reviewed-on: https://chromium-review.googlesource.com/1212145
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589559}
[modify] https://crrev.com/3f9f7845019e6d4aa6f157b43ec6a45564cc50aa/chrome/browser/plugins/pdf_plugin_placeholder_observer.cc

Verified this is working in Windows Canary 71.0.3548.0.

Requesting merge to M70, since this is a noticeable user-facing regression (when this setting is enabled).  Fix is small and safe and has been baking since 71.0.3546.0.  I didn't see any new related crashes from that version.

Approving merge to M70 branch 3538 based on comment #6 and per offline chat with creis@ as abdulsyed@ is OOO. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/049e36937d5776b4e8871ef3dfaa7c7698f7cde7

commit 049e36937d5776b4e8871ef3dfaa7c7698f7cde7
Author: Charlie Reis <creis@chromium.org>
Date: Mon Sep 10 21:25:50 2018

[Merge to M70] Use matching RVH/RFH in DownloadUrlParameters for PDF plugin placeholder.

BUG= 881497 
TEST=See bug for repro steps.

Change-Id: I392788d31e638bc79bb8d35e0451c79af6ca3a26
Reviewed-on: https://chromium-review.googlesource.com/1212145
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589559}(cherry picked from commit 3f9f7845019e6d4aa6f157b43ec6a45564cc50aa)
Reviewed-on: https://chromium-review.googlesource.com/1217448
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#251}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/049e36937d5776b4e8871ef3dfaa7c7698f7cde7/chrome/browser/plugins/pdf_plugin_placeholder_observer.cc

Agreed with govind & creis that we should not merge this to 69, since it's too late in the game.

I will use Finch to disable Click to Open PDF on Desktop until 70.

By the way Charlie, I have not forgotten about our discussion of adding an end-to-end browsertest to guard against future regressions of this nature.

These tests are being reviewed here:
https://chromium-review.googlesource.com/c/chromium/src/+/1246956