New issue
Advanced search Search tips

Issue 697513 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Three plugin layout tests are timing out on Site Isolation Win

Project Member Reported by alex...@chromium.org, Mar 1 2017

Issue description

The following tests are timing out on Site Isolation Win FYI bot:

fast/frames/iframe-plugin-load-remove-document-crash.html
plugins/iframe-plugin-bgcolor.html
plugins/plugin-document-back-forward.html

These started in https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Win/builds/18179

I've confirmed that this is due to my r453800 and will investigate further.
 
Here's what's going on.  It appears that all these tests embed the Blink layout test plugin like this:

  <iframe src="data:application/x-blink-test-plugin,foo"></iframe>

Apparently, we've got a complicated policy in the renderer about whether or not data URLs should be resolved locally or punted to the browser process.  This is in WebURLLoaderImpl::Context::CanHandleDataURLRequestLocally(), which returns true for most cases, but returns false here, due to mime_util::IsSupportedMimeType() returning false for application/x-blink-test-plugin.

This causes the request to go out to the browser process (to DataProtocolHandler), where it asks whether a transfer is needed for "data:application/x-blink-test-plugin,foo".  IsRendererTransferNeededForNavigation says yes, we go on the transfer path and into UpdateStateForNavigate().  When we ask RFHM::CanSubframeSwapProcess(), that returns false, because the destination is a unique origin, there's no source_instance (we lost it as part of the transfer) or dest_instance, and it's not a server redirect.  So, we return the current RFH from UpdateStateForNavigate.  So far, all of this is the same with and without my fix from r453800.  The difference is that without my fix, we returned the current RFH as the first thing, before the Transfer() call or the work to cancel the pending RFH.  After r453800, we end up also calling Transfer() on the transfer_navigation_handle_ (even though the transfer isn't really going to happen, due to CanSubframeSwapProcess), and that screws up the load because we're now expecting to transfer away from the current RFH, whereas the load will in fact stay in it.

I don't know how big of a problem this is outside of tests.  Regular data URLs loaded in local frames would be serviced in the renderer, so they won't hit this.  Navigating proxies to data URLs implies that the frame's current RFH will be different from the destination RFH (since data URLs stay in the source SiteInstance, i.e., whoever navigated the frame), so that needs to swap RFHs and there'd be no problem.  Pointing a local frame to a data URL with a weird mime type apparently leads to a download; I haven't found an unsupported mime type that would allow me to load text in an iframe.  So the broken cases would be data URLs for plugins, though I don't know if anyone is actually embedding things like Flash objects using data URLs.  Don't know if I'm missing any other cases where this would matter.

I'll probably disable the tests for now while I think about the fix.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 2 2017

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

commit a50c1f9a0756dcbcfe1939bf0a1e699bcfe9f9b2
Author: alexmos <alexmos@chromium.org>
Date: Thu Mar 02 01:12:28 2017

Disable plugin layout tests that are timing out on Site Isolation Win

BUG= 697513 
NOTRY=true

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

[modify] https://crrev.com/a50c1f9a0756dcbcfe1939bf0a1e699bcfe9f9b2/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 9 2017

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

commit 71e8d307789f1cf492f4803bdceab898e8105a47
Author: alexmos <alexmos@chromium.org>
Date: Thu Mar 09 19:08:13 2017

Fix transfers for data URLs with plugin mime types.

Typically, when a local frame navigates to a data URL, the request is
serviced in the renderer without going up to the browser process.
However, this isn't true for some data URLs, such as those with plugin
mime types.  Those go up to the browser, where they stalled due to
conflicting logic that first decided that a transfer is needed, marked
the current RFH as transferring, but then decided that a process
transfer isn't allowed and kept the load in the current RFH.

When the process transfer is prevented by CanSubframeSwapProcess, the
load will always stay in the current RFH.  This CL tries to recognize
scenarios where this happens for transfers that also starts in the
current RFH, so that we can avoid calling Transfer() for the current
RFH unnecessarily.

BUG= 697513 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/71e8d307789f1cf492f4803bdceab898e8105a47/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/71e8d307789f1cf492f4803bdceab898e8105a47/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Status: Fixed (was: Started)
Should be fixed.  The plugin tests aren't failing on Site Isolation Win anymore (starting in build 18265) after they were reenabled in r455821.

Comment 5 by creis@chromium.org, Apr 27 2017

Note: It turns out this bug affected redirects to hosted apps within subframes as well.  This caused affected subframes that redirected to Google Docs URLs in  issue 715756 , and Google Calendar URLs in issue 710668.  The same fix seems to work for those cases, so we're looking into merging it to M58 in  issue 715756 .
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 28 2017

Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0454f5814f0bcd28398d93ec6700fc34adae20a0

commit 0454f5814f0bcd28398d93ec6700fc34adae20a0
Author: Charles Reis <creis@chromium.org>
Date: Fri Apr 28 17:17:15 2017

Fix transfers for data URLs with plugin mime types.

Typically, when a local frame navigates to a data URL, the request is
serviced in the renderer without going up to the browser process.
However, this isn't true for some data URLs, such as those with plugin
mime types.  Those go up to the browser, where they stalled due to
conflicting logic that first decided that a transfer is needed, marked
the current RFH as transferring, but then decided that a process
transfer isn't allowed and kept the load in the current RFH.

When the process transfer is prevented by CanSubframeSwapProcess, the
load will always stay in the current RFH.  This CL tries to recognize
scenarios where this happens for transfers that also starts in the
current RFH, so that we can avoid calling Transfer() for the current
RFH unnecessarily.

BUG= 697513 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2729523003
Cr-Commit-Position: refs/heads/master@{#455821}
(cherry picked from commit 71e8d307789f1cf492f4803bdceab898e8105a47)

Review-Url: https://codereview.chromium.org/2845223002 .
Cr-Commit-Position: refs/branch-heads/3029@{#781}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/0454f5814f0bcd28398d93ec6700fc34adae20a0/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/0454f5814f0bcd28398d93ec6700fc34adae20a0/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Sign in to add a comment