Chrome has to be started using --site-per-process to trigger this bug. This should be apparent from the "remote frame" terminology, but I explicitly state it - just in case.

Asanka: Weren't you looking into a failure of downloads to work with site isolation?

It also looks as if Chrome Canary comes up with --site-per-process enabled, eventhough chrome://flags says it's not. Toggling the switch back and forth to "Disabled" and relaunching Chrome turns it off.

#2: We switched to using the WebContentsGetter for downloads instead of using the ids. The code path in question starts in //content at RenderMessageFilter::OnDownloadUrl().

It appears the ViewHostMsg_DownloadUrl IPC never makes it to RenderMessageFilter.

Ah, that explains it. It is due to the swapped out nature of RenderViewImpl for subframes. If we convert this message to be routed through the RenderFrame IPC channel, it will work just fine.

#7: Yup. The RenderViewImpl is created in a swapped out state and doesn't route any IPC messages out.

I'll take a whack at it early next week. Please feel free to take this bug if anyone else wants to start. But update the bug before doing so.

I'm going to try to poke at this.

\o/

By the way, while we are reworking the IPCs, it would be great if we could determine the interface origin and the value of the @download attribute from the receiving side. Currently the interface origin is derived from |referrer| and |suggested_name| is assumed to always be the value of @download. This is important for correctly implementing the name determination algorithm in https://www.w3.org/TR/html5/links.html#downloading-resources .

That always referrer.url.GetOrigin() == |interface origin| is not a good assumption. We'll need to pass the interface origin separately. |suggested_name| could be documented to be the @download value since no other code paths result in a non-empty |suggested_name|.

Modified version of the test with a canvas element in the frame. This illustrates a similar bug with "save as" for data URLs in OOPIF.

Deleted: index.html 1.5 KB

index.html 1.5 KB View Download

My work-in-progress patch is here:
https://codereview.chromium.org/1836973003/

This seems to work in manual testing but there is a FIXME line in render_frame_message_filter.cc where I fudge an ID.

I started removing the render view ID from the download system so I can remove this, and this change looks reasonable. The exception is that this feeds into ResourceDispatcherHost which is very complicated and covered separately by issue 472869.

Most of ResourceDispatcherHost uses render frame ids at this point. The main culprits of using RVIDs are consumers of the ResourceRequestInfo::GetRouteID() method. In particular I believe downloads don't do anything in RDHI with the id except populate that structure.

Off the top of my head, I remember that extensions, webview, host zoom map, load state notifications, and resource scheduler are still using the RVIDs off the ResourceRequestInfo.

Do you think it's safe to populate the RDH from downloads with a -1 RVH ID?

+creis for #16.

Downloads will just pass along the RVH ID to RDHI::BeginDownload where it's used to construct a ResourceRequestInfo.

I don't think I have enough knowledge of the network stack to answer comment 16.  @csharrison, do you know?

(Actually CC'ing csharrison just in case)

Thanks for the ping. Whether it's safe to use an invalid route is up to the consumers of RVIDs off the ResourceRequestInfo. I don't know enough about the consumers (esp webview/extensions) to know for sure what they expect.

My guess is that it would work though. Most of the consumers use the ids to reconstruct RenderViewHosts/WebContents. They should all have checks to make sure that the RVH didn't go away. Hopefully this will make invalid RVIDs safe to use.

ccing nasko@ who iirc has been following up on consumers of RVIDs.
re-ccing asanka@.

Poking at the places that use the RVH id from ResourceRequestInfo the only affected areas are chrome signin and the webRequest extension API. I don't know if the latter will be impacted if we send MSG_ROUTING_NONE as the id. 

Adding rdevlin.cronin@ to ensure it won't break the webRequest API, as I'm not sure if it processes downloads.

@21 We use the RFID from the ResourceRequestInfo rather than the RVID - will that also be affected?  I don't think webRequest directly listens to downloads, but it could potentially also depend on the download (e.g. I imagine we could perform a network request before determining that the response is a download?)

Ah, the usage within webRequest seems to be limited to <webview> cases if I got it right (usage in ExtractRequestRoutingInfo) - https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/api/web_request/web_request_api.cc&rcl=1462965594&l=171.

It seems we are ok then for all usages of the RVH id to pass MSG_ROUTING_NONE.

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

commit 760f7448c5798bfd55734e412f09f562e3a469b2
Author: brettw <brettw@chromium.org>
Date: Mon May 23 21:19:22 2016

Updates to DownloadUrlParameters in preparation for OOPIF changes

This should have no behavior change except for theoretical crash fix.

Renames DownloadUrlParameters::FromWebContents to CreateForWebContentsMainFrame to be more clear what this actually does. Clean up the constructors. WebContentsImpl::SaveFrameWithHeaders is a bit different. The functionality is the same but I separated out the frame computation code so that in the future we can replace the frame with the right one and the rest of the function will be correct (we won't want to use the main frame in the future).

Add null check for RenderFrameHost in a RenderViewContextMenu that was previously not checking for null. This is a potential crash if the frame disappears when the menu is open.

Make the RenderViewContextMenu construct a DownloadUrlParameters object referencing the frame doing the downloading rather than the main frame of the page.

Rename some child_id variables to process_id which makes more sense to me.

Add some comments clarifying which RenderViewHost should be used for DownloadUrlParameters and ResourceRequestInfo.

Remove "temporary" comment about RenderViewHost from render_frame_host.h. Charlie says this will not go away (this was confusing me and made me thing that the relationship between some of these things is different than it is) but will likely just be renamed in the future).

BUG= 596283 , 482049

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

[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/download/download_browsertest.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/extensions/api/downloads/downloads_api.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/extensions/api/streams_private/streams_private_apitest.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/plugins/plugin_installer.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/chrome/browser/renderer_context_menu/render_view_context_menu.h
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/android/download_controller_android_impl.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/download/download_browsertest.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/download/download_item_impl.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/download/drag_download_file.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/indexed_db/indexed_db_internals_ui.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/loader/resource_dispatcher_host_impl.h
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/public/browser/download_url_parameters.cc
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/public/browser/download_url_parameters.h
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/public/browser/render_frame_host.h
[modify] https://crrev.com/760f7448c5798bfd55734e412f09f562e3a469b2/content/public/browser/resource_request_info.h

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

commit 8f508f442da78a72c61cfec1dc4c77df57406cbd
Author: brettw <brettw@chromium.org>
Date: Mon Jun 20 21:34:21 2016

Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message doesn't get received. Moving the download messages to the FrameHost allows download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks data_url.is_valid() before interrogating the GURL (which would otherwise assert for invalid URLs).

BUG= 596283 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/components/renderer_context_menu/render_view_context_menu_base.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/components/test_runner/pixel_dump.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/frame_host/render_frame_message_filter.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/renderer_host/render_message_filter.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/browser/renderer_host/render_view_host_unittest.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/common/frame_messages.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/common/view_messages.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/public/browser/render_frame_host.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/public/browser/render_view_host.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_frame_impl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_frame_impl_browsertest.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_view_impl.cc
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/content/renderer/render_view_impl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/Source/web/tests/WebViewTest.cpp
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/public/web/WebLocalFrame.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/public/web/WebView.h
[modify] https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd/third_party/WebKit/public/web/WebViewClient.h

Confirmed that <a download> links work inside OOPIFs in 53.0.2774.2, so I'll mark this fixed.  Thanks!

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

commit 9b526a95cf600ccd10b03b7881c957ba2487ae3e
Author: asanka <asanka@chromium.org>
Date: Wed Jul 13 14:48:40 2016

[Downloads] Add a browser test for <a download> with cross origin iframe.

R=brettw@chromium.org
BUG= 596283 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/browser/download/download_browsertest.cc
[add] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/test/data/download/iframe-host.html
[add] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/test/data/download/iframe-host.html.mock-http-headers

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

commit 9b526a95cf600ccd10b03b7881c957ba2487ae3e
Author: asanka <asanka@chromium.org>
Date: Wed Jul 13 14:48:40 2016

[Downloads] Add a browser test for <a download> with cross origin iframe.

R=brettw@chromium.org
BUG= 596283 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/browser/download/download_browsertest.cc
[add] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/test/data/download/iframe-host.html
[add] https://crrev.com/9b526a95cf600ccd10b03b7881c957ba2487ae3e/content/test/data/download/iframe-host.html.mock-http-headers