Yes, this is certainly a result of checking `X-Frame-Options` up in the browser process before we actually create a document (https://chromium.googlesource.com/chromium/src/+/26a6fc92ae361b4271f8f2197abe7eb063fc43ed). Looks like we might want to ignore `X-Frame-Options` if we're going to treat the response as a download, though it's not clear to me when we make that decision.

Nasko, Charlie, Camille: Any advice about where that check might be reasonably made? It looks like the decision happens in `MIMETypeResourceHandler`. Would it be enough to move that handler's creation down after the throttle handler's creation in `ResourceDispatcherHostImpl::AddStandardHandlers` (that is, move https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc&rcl=1462778281&l=1726 to https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc&rcl=1462778281&l=1758?)

Marking the above issue as RB-Stable as this is a recent regression.

Thank you!

I'm not quite sure. If you move the creation of the ThrottlingResourceHandler after the MIMETypeResourceHandler, this means none of the throttles (this includes SafeBrowsing for example) can execute before the MIMETypeResourceHandler potentially creates extra-state to handle the request. We'd need the opinion of someone from the net stack team I think.

Yeah, that sounds risky to me.  Adding asanka@ and mmenke@ for other ideas about comment 1 (i.e., ignoring the newly browser-enforced X-Frame-Options for downloads).

creis@: Ok. Bypassing SafeBrowsing would be a bad thing indeed. The other option is to special-case the header inside of AncestorThrottle. I can certainly do that; it's a bit hacky, but probably safer.

For navigation initiated downloads, we end up with two sets of throttles. One that's created at RDHI::AddStandardHandlers(), and one that's created at RDHI::CreateResourceHandlerForDownloads(). So the ResourceHandler stack for a navigation initiated download looks like this (in order of ownership):

  1. ThrottlingResourceHandler                       [A]
     
     Contains: NavigationResourceThrottle, <throttles provided by delegate in response to BeginRequest()>, PowerSaveBlockerResourceThrottle, ScheduledResourceRequest.

  2. MimeTypeResourceHandler

  3. ThrottlingResourceHandler                       [B]

     Contains: <throttles provided by delegate in response to DownloadStarting. Excludes standard resource throttles.>

  4. DownloadResourceHandler

Of these, delegate provided standard throttles live in [A].

ChromeResourceDispatcherHostDelegate::DownloadStarting already has the logic in place for adding all the standard throttles to a download request, but it doesn't add them for navigation initiated downloads since those throttles are already present in [A]. However, for explicit download requests (handling of <a download> elements, for instance), [A] would not exist, and the ResourceHandler stack looks like this:

  1. ThrottlingResourceHandler                       [C]

     Contains: <throttles provided by delegate in response to DownloadStarting()>

  2. DownloadResourceHandler

In this case, CRDHD::DownloadStarting() adds all the standard throttles to [C] via CRDHD::AppendStandardResourceThrottles. Hence the delegate provided throttles in [C] would be the union of the delegate provided throttles in [A] and [B].

What's proposed in #1 would cause the ThrottlingResourceHandler at [A] to be discarded by MimeTypeResourceHandler when switching to the alternate RH stack in preparation for a download. Hence all the delegate provided throttles in [A] would need to also exist in [B]. This can be accomplished by having the delegate's DownloadStarting() handler treat navigation initiated downloads the same as explicit downloads. Something that's actually simpler than the current scheme.

One complication of the scheme in #1 would be that for navigation initiated downloads, standard delegate provided throttles would be instantiated twice. Hence the ResourceThrottles would need to not carry state.

I put up https://codereview.chromium.org/1969743002 as the simplest thing that could possibly work. Is it a reasonable stopgap, or would a deeper refactoring along the lines of #7 be a better solution? If the latter, does anyone have time to do it before the branch? I don't. :)

I defer to Asanka here, but I don't think it's good enough - it doesn't work for files that are downloaded because of their mimetype (Regardless of whether it's sniffed or non-sniffed).

I think there will be other NavigationThrottle implementations that will benefit from processing after the MimeTypeResouceHandler has had a chance to run. As such, I think it is better to revert this one for now and come up with a solid solution for this before relanding it. Unless someone can indeed land proper fix before branch cut, which is in about 1.5 weeks.

I agree with reverting, though admittedly, I'm not sure what the priority is of shipping X-Frame-Options.  I think enough could go wrong that even if we could get a fix in within 1.5 weeks, that's not enough baking time.

Request someone to an update on the above issue ?

Appreciate the help.

Thank you!

Agree with #9. #8 doesn't handle cases where a response is treated as a download due to the effective Content-Type not being supported by the renderer or any plugins. An unfortunately common way to initiate downloads is to set the Content-Type to something like application/octet-stream. I don't know how common the intersection between that and XFO is, but I'd guess that it's non-trivial.

I'm guessing that the other implementations allow the download in this case?

I think mkwst@ might be out this week-- is someone else able to attempt a revert of r392032 before the branch cut?

I'll do the revert.

Actually, I won't.  It doesn't revert cleanly, and I don't have the time to dig through the WebKit code to figure out why.

Sorry for the delay, I'll take care of it. I'm not entirely here, but I'm here enough.

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

commit 21f43dc397b74d88f8d9d5837c12343b834890e6
Author: mkwst <mkwst@chromium.org>
Date: Thu May 19 07:28:12 2016

Revert "Introduce AncestorThrottle, which will process 'X-Frame-Options' headers."

This reverts commit 26a6fc92ae361b4271f8f2197abe7eb063fc43ed, which
broke our handling of 'Content-Disposition' and similar download-triggering
mechanisms for sites like Drive. Which makes me sad.

This revert leaves the new error code in place, as it had already hit the histogram
file, and we want to ensure that it doesn't start meaning something else inbetween
now and the time at which we reland this patch.

Original commit: https://chromium.googlesource.com/chromium/src/+/26a6fc92ae361b4271f8f2197abe7eb063fc43ed

BUG= 610284 , 555418 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/1988933003
Cr-Commit-Position: refs/heads/master@{#394707}

[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/components/error_page/common/localized_error.cc
[delete] https://crrev.com/8a956ecdb3d8794d1ddb0a4003e818bfbd19ebef/content/browser/frame_host/ancestor_throttle.cc
[delete] https://crrev.com/8a956ecdb3d8794d1ddb0a4003e818bfbd19ebef/content/browser/frame_host/ancestor_throttle.h
[delete] https://crrev.com/8a956ecdb3d8794d1ddb0a4003e818bfbd19ebef/content/browser/frame_host/ancestor_throttle_unittest.cc
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/content/browser/loader/navigation_resource_throttle.cc
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/content/content_browser.gypi
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/content/content_tests.gypi
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/content/public/browser/navigation_throttle.h
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/LayoutTests/http/tests/inspector/network/x-frame-options-deny-expected.txt
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/LayoutTests/http/tests/inspector/network/x-frame-options-deny.html
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-expected.txt
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-multiple-headers-conflict-expected.txt
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny-expected.txt
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/dom/DocumentInit.cpp
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/inspector/InspectorInstrumentationCustomInl.h
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/loader/DocumentLoader.h
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/core/loader/HttpEquiv.h
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
[modify] https://crrev.com/21f43dc397b74d88f8d9d5837c12343b834890e6/third_party/WebKit/Source/platform/network/HTTPParsers.h

Could you fix it please

Unable to repro this issue on Windows 7, MAC (10.11.4) & Ubuntu Trusty (14.04) for Google Chrome Canary Version - 52.0.2743.0

Screen-recording is attached.

Looks like this issue is fixed.

@rkote: Could you please confirm the same.

Thank you.

Deleted: 610284.mov 16.2 MB

610284.mov 16.2 MB Download

With respect to comment 20:

Issue is seems to be fixed in latest canary version 53.0.2746.0

Thank you.

Thanks for the update.Adding respective labels for the same and updating the status to Fixed.