New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 647772 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Shift-clicking targeted links to RemoteFrames doesn't work

Project Member Reported by lfg@chromium.org, Sep 16 2016

Issue description

Version: ToT
OS: All

What steps will reproduce the problem?
(0) Open chrome using --site-per-process.
(1) navigate to http://csreis.github.io/tests/cross-site-iframe-simple.html
(2) Paste the following into the console for the top-level frame:
var i = document.querySelector('iframe');
var src = i.src;
i.src = 'about:blank';
setTimeout(function() {i.contentWindow.name = 'iframe'; i.src = src; }, 100);
var a = document.createElement('a');
a.innerHTML = 'link';
a.target='iframe';
a.href = src;
document.body.appendChild(a);

(3) Shift-click the 'link'.

What is the expected output?

New window navigates to the link.

What do you see instead?

The iframe navigates to the link.

 

Comment 1 by lfg@chromium.org, Sep 16 2016

To add some more information:
- Only CURRENT_TAB disposition works when navigating RemoteFrames (ctrl-click, etc should also be broken).

- This happens because RenderFrameProxyHost uses RequestTransferURL() instead of RequestOpenURL(). This was done in order to fix other issues due to some WebContentsDelegates not being implemented properly (alexmos@, can you link the other bugs?).

- This will likely be a problem for PlzNavigate -- Since navigations must go through the browser process, and the WebContentsDelegate is used to cancel some navigations, this will need to be addressed there.

Comment 2 by creis@chromium.org, Sep 16 2016

Components: UI>Browser>Navigation
For simpler repro steps, I added a name to the iframe on http://csreis.github.io/tests/cross-site-iframe-simple.html.  You can now repro as:

(0) Open chrome using --site-per-process.
(1) navigate to http://csreis.github.io/tests/cross-site-iframe-simple.html
(2) Paste the following into the console for the top-level frame:
  var a = document.createElement('a');
  a.innerHTML = 'link';
  a.target='frame1';
  a.href = 'http://csreis.github.io';
  document.body.appendChild(a);
(3) Shift-click the 'link'.
Here's some more information and context.  There are two different issues here: cross-process transfers, and remote frame navigations that went through RenderFrameHostImpl::OnOpenURL.  Originally both used the OpenURL paths.

- Several WebContentsDelegates don't implement OpenURLFromTab.  The original one that came to bite us was extensions::ExtensionHost, which caused transfers for OOPIFs in background pages to break ( issue 560510 ).  To fix this, we added a default implementation to WebContentsDelegate::OpenURLFromTab in r361724.  However, it turned out that some delegates actually relied on not implementing WebContentsDelegate::OpenURLFromTab to block navigations.  One example is sign-in pages in  issue 562389 .  So, we removed the default implementation in r367711.

- It also turned out that some WebContentsDelegates have issues in their OpenURLFromTab implementations.  E.g., PanelHost messes with dispositions, aborts some navigations, and doesn't pass all params to chrome::Navigate.  This was causing the Hangouts extension to break when we were ramping up our --isolate-extensions trial.  See  issue 568357  and especially comment 8.  I think DevTools had this kind of issue also.  As a result, Charlie landed r365443 to shortcut transfers to not go through OpenURLFromTab.  This is what allowed us to remove the default implementation in WebContentsDelegate::OpenURLFromTab in r367711 while keeping process transfers for OOPIFs in background pages working.

- After we fixed transfers to not use OpenURL, we realized that remote frame navigations were still using RFHI::OpenURL, which led to the same issues due to some delegates not having OpenURLFromTab implementations.  For example, see  issue 588096  for remote frame navigations in background pages.  To fix that, I had to switch RenderFrameProxyHost::OnOpenURL to use RequestTransferURL instead of OpenURL in r377832.

Overall, I agree that frames and proxies should be going through the same code path, and that we should fix the shift-clicking etc. for remote frames.  But it's going to be a tricky fix, as we'll need to keep all of the issues above in mind.

Comment 4 by creis@chromium.org, Sep 20 2016

Cc: lukasza@chromium.org
Comment 3: Yes, it's tricky to move back to OpenURL for those reasons.  There's a lot of complicated behavior in the delegates, and it's not clear all of it should apply to navigating a remote frame.

That said, we just found a way forward on one of the problems while discussing  issue 646261 .  OpenURLFromTab isn't really the right place to be be blocking navigations (e.g., since that also blocks downloads and 204s, which wouldn't commit).  Instead, things like ExtensionViewHost and the signin case should be returning false from WebContentsDelegate::ShouldTransferNavigation.

I'm guessing that doesn't solve all the problems (e.g., PanelHost?), but maybe it makes it more tractable to switch back to RequestOpenURL in RenderFrameProxyHost.

Comment 5 by lfg@chromium.org, Sep 20 2016

How would returning false on ExtensionViewHost::ShouldTransferNavigation work for navigations from RenderFrameProxies? For example, if a chrome-extension url has a web iframe, then all navigations to the iframe coming from the extension page would be blocked, which doesn't seem desirable.

Or do you mean returning false in ShouldTransferNavigation, and switching RenderFrameProxyHost back to RequestOpenURL at the same time, so that the transfer fails? Wouldn't that cause other issues?

Comment 6 by creis@chromium.org, Sep 20 2016

Comment 5: Hmm, we do seem to need a distinction between main frames and subframes here.  ExtensionViewHosts (and the signin case) aren't supposed to navigate to web pages in the main frame, which is probably why OpenURLFromTab didn't support CURRENT_TAB.

However, you're right that this would probably block OOPIFs in those pages as well, which isn't desirable.  Maybe ShouldTransferNavigation should take a is_main_frame parameter?

(That's true for fixing  issue 646261 , and I think it's true for RenderFrameProxyHosts as well.  Other tabs generally don't have remote frame references to ExtensionViewHosts, but if they did, they shouldn't be allowed to navigate the ExtensionViewHost to a web page via RenderFrameProxyHost.  Thus, we could switch RenderFrameProxyHost to use RequestOpenURL if we can fix the remaining issues.)
nasko@ pointed out that navigating a new window doesn't necessarily require going through a RenderFrameProxy::navigate - if we can trigger the navigation from within the same renderer, then maybe we can avoid the trouble of not handling the disposition when going via RenderFrameProxyHost::OnOpenURL / RequestTransferURL.

In other words - since Ctrl-click (or Shift-click) seems to override/ignore "target" attribute of an anchor/link, then there is no need to hande this click via the remote/target frame.
Cc: -lukasza@chromium.org nasko@chromium.org
Owner: lukasza@chromium.org
Status: Started (was: Available)
I've verified that nasko's idea from #c7 does fix the bug.  I have a WIP CL @ https://codereview.chromium.org/2410303006.  Let me try to figure out how to add a test.
The CL from #c9 first appeared in 56.0.2897.0.  I've verified that the repro steps no longer work in 56.0.2899.0 (Official Build) canary (64-bit).

Still - I am not sure if I should close this bug, because we are still investigating a regression related to the CL from #c9 -  issue 658701 .
Status: Fixed (was: Started)
We understand  issue 658701  + there is no other known fallout from r426483, so I think we can mark this bug as fixed.
lfg@, were there any other issues that needed follow-up here?

- Any other issues that would have us reconsider calling RequestTransferURL vs RequestOpenURL from RenderFrameProxyHost::OnOpenURL?

- Any other follow-ups needed by issue 643156 (Extension popup is wrong size after framebusting with --isolate-extensions enabled)

Comment 13 by lfg@chromium.org, Oct 27 2016

Agreed, I think this fixes all known issues, any follow-up work should be a separate bug.

I've also verified (in 56.0.2903.0 with --site-per-process enabled and using repro steps from #c2) that an anchor targeting a remote frame works fine not only when ctrl-clicking or middle-clicking, but also when opening in a new tab or window from the anchor's context menu (alexmos@ just pointed out to me that for some other things [like web-accessible-resources] the behavior differs between middle-clicking and going through the context menu).
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 3 2016

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

commit 452ce50129657e65181973f1e49e3c963a445e0a
Author: alexmos <alexmos@chromium.org>
Date: Thu Nov 03 17:06:16 2016

Clean up source_site_instance usage in Navigator::RequestOpenURL.

Passing source_site_instance is unnecessary/redundant now that proxy
navigations don't use this, and the only call site is
RenderFrameHostImpl::OpenURL, which always passes in the
current RFH's SiteInstance.

BUG= 656752 ,  647772 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/security_exploit_browsertest.cc

Sign in to add a comment