Shift-clicking targeted links to RemoteFrames doesn't work |
|||||
Issue descriptionVersion: 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.
,
Sep 16 2016
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'.
,
Sep 16 2016
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.
,
Sep 20 2016
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.
,
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?
,
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.)
,
Oct 12 2016
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.
,
Oct 12 2016
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.
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e7250ebe260957a67131667cdde6b7b838a9b1f5 commit e7250ebe260957a67131667cdde6b7b838a9b1f5 Author: lukasza <lukasza@chromium.org> Date: Thu Oct 20 15:43:51 2016 Avoiding going through RenderFrameProxy when targeting a *new* frame. BUG= 647772 Review-Url: https://chromiumcodereview.appspot.com/2410303006 Cr-Commit-Position: refs/heads/master@{#426483} [modify] https://crrev.com/e7250ebe260957a67131667cdde6b7b838a9b1f5/chrome/browser/chrome_site_per_process_browsertest.cc [add] https://crrev.com/e7250ebe260957a67131667cdde6b7b838a9b1f5/chrome/test/data/frame_tree/anchor_targeting_remote_frame.html [modify] https://crrev.com/e7250ebe260957a67131667cdde6b7b838a9b1f5/third_party/WebKit/Source/core/loader/FrameLoader.cpp
,
Oct 24 2016
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 .
,
Oct 26 2016
We understand issue 658701 + there is no other known fallout from r426483, so I think we can mark this bug as fixed.
,
Oct 26 2016
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)
,
Oct 27 2016
Agreed, I think this fixes all known issues, any follow-up work should be a separate bug.
,
Nov 2 2016
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).
,
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 |
|||||
Comment 1 by lfg@chromium.org
, Sep 16 2016