chrome/test/data/frame_tree/anchor_to_same_site_location.html - Issue 2686943002: New WebContents created via ctrl-click should be in a new process.

Unified Diff: chrome/test/data/frame_tree/anchor_to_same_site_location.html

Issue 2686943002: New WebContents created via ctrl-click should be in a new process. (Closed)

Patch Set: . Created 3 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/extensions/api/tabs/tabs_test.cc ('K') | « chrome/browser/extensions/api/tabs/tabs_test.cc ('k') | content/browser/frame_host/navigator_impl.cc » ('j') | content/browser/frame_host/navigator_impl.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/test/data/frame_tree/anchor_to_same_site_location.html

diff --git a/chrome/test/data/frame_tree/anchor_to_same_site_location.html b/chrome/test/data/frame_tree/anchor_to_same_site_location.html

new file mode 100644

index 0000000000000000000000000000000000000000..b5527d73bca472cb8aaefc261e3d83df2560b0bb

--- /dev/null

+++ b/chrome/test/data/frame_tree/anchor_to_same_site_location.html

@@ -0,0 +1,21 @@

+<!doctype html>

+<html>

+<head>

+<script>

+window.name = "main_contents";

+function simulateClick(anchor_name, properties) {

+ var evt = new MouseEvent("click", properties);

+ target = document.getElementById(anchor_name);

+ return target.dispatchEvent(evt);

+</script>

+</head>

+<body>

+This page helps testing shift-clicking or ctrl-clicking an anchor/link

+that points to the same site. See also https://crbug.com/238156.

+<hr>

+<a href="/title1.html"

+ id="test-anchor-no-target">Test link to click while holding ctrl key</a>

Łukasz Anforowicz 2017/02/09 16:53:01 Charlie was pointing out that another interesting

alexmos 2017/02/09 22:58:36 Interesting, so ctrl-click on <a target="_blank">

Łukasz Anforowicz 2017/02/10 00:38:18 Yes, the new window/tab has: - same process - same

Yes, the new window/tab has: - same process - same site-instance - window.opener is set - window.open is able to find the opener by name creis@ pointed out in https://crbug.com/658386#c9 that this is different from the behavior in Firefox (which clears out window.opener in this case).

I am not sure, but I've verified that in case of target="..." we go through createWindowForRequest in Blink. We don't go through createWindowForRequest (and go through OpenURL instead) in case of ctrl-click without target="...".

This is a great question - thanks for asking. It turns out that in case of an existing name, things work as expected (no opener, different process, window.open doesn't find the opener, etc.). I see in FrameLoader::load that createWindowForRequest path is only taken if the target frame doesn't exist yet and otherwise, we end up in OpenURL path: #2 0x7f7aeb4de036 content::RenderFrameImpl::OpenURL() #3 0x7f7aeb4dd464 content::RenderFrameImpl::loadURLExternally() #4 0x7f7adeb3bba1 blink::FrameLoaderClientImpl::loadURLExternally() #5 0x7f7adacd08fb blink::FrameLoader::shouldContinueForNavigationPolicy() #6 0x7f7adacd0f6e blink::FrameLoader::checkLoadCanStart() #7 0x7f7adaccb61b blink::FrameLoader::startLoad() #8 0x7f7adacbf003 blink::FrameLoader::load() #9 0x7f7ad99fc499 blink::HTMLAnchorElement::handleClick() For non-existent targets we take a different path in FrameLoader::load (and return early - createWindowForRequest handles continuing the navigation): #2 0x7fcdb9de6acf blink::createWindowForRequest() #3 0x7fcdb9ce7100 blink::FrameLoader::load() #4 0x7fcdb8a24499 blink::HTMLAnchorElement::handleClick() The case with target="_blank" is important to fix as well (AFAICT this is what https://crbug.com/23815 deals with [according to #c1]), but I hope to do this in a separate CL (probably tweaking how FrameLoader::load decides whether to take the createWindowForRequest path).

Łukasz Anforowicz 2017/02/10 01:13:50 BTW: I am trying to address the target=_blank case

On 2017/02/10 00:38:18, Łukasz Anforowicz wrote: > On 2017/02/09 22:58:36, alexmos wrote: > > On 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > > > Charlie was pointing out that another interesting scenario here is > > > target="_blank". Currently this scenario still leads to the old process > being > > > reused. > > > > Interesting, so ctrl-click on <a target="_blank"> would reuse the process? > > Yes, the new window/tab has: > - same process > - same site-instance > - window.opener is set > - window.open is able to find the opener by name > > creis@ pointed out in https://crbug.com/658386#c9 that this is different from > the behavior in Firefox (which clears out window.opener in this case). > > > Do you know why? > > I am not sure, but I've verified that in case of target="..." we go through > createWindowForRequest in Blink. We don't go through createWindowForRequest > (and go through OpenURL instead) in case of ctrl-click without target="...". > > > What if the target is something else, like an existing (or > > non-existing) name? What's the behavior of ctrl-click there? > > This is a great question - thanks for asking. It turns out that in case of an > existing name, things work as expected (no opener, different process, > window.open doesn't find the opener, etc.). I see in FrameLoader::load that > createWindowForRequest path is only taken if the target frame doesn't exist yet > and otherwise, we end up in OpenURL path: > > #2 0x7f7aeb4de036 content::RenderFrameImpl::OpenURL() > #3 0x7f7aeb4dd464 content::RenderFrameImpl::loadURLExternally() > #4 0x7f7adeb3bba1 blink::FrameLoaderClientImpl::loadURLExternally() > #5 0x7f7adacd08fb blink::FrameLoader::shouldContinueForNavigationPolicy() > #6 0x7f7adacd0f6e blink::FrameLoader::checkLoadCanStart() > #7 0x7f7adaccb61b blink::FrameLoader::startLoad() > #8 0x7f7adacbf003 blink::FrameLoader::load() > #9 0x7f7ad99fc499 blink::HTMLAnchorElement::handleClick() > > For non-existent targets we take a different path in FrameLoader::load (and > return early - createWindowForRequest handles continuing the navigation): > > #2 0x7fcdb9de6acf blink::createWindowForRequest() > #3 0x7fcdb9ce7100 blink::FrameLoader::load() > #4 0x7fcdb8a24499 blink::HTMLAnchorElement::handleClick() > > > The case with target="_blank" is important to fix as well (AFAICT this is what > https://crbug.com/23815 deals with [according to #c1]), but I hope to do this in > a separate CL (probably tweaking how FrameLoader::load decides whether to take > the createWindowForRequest path). >

BTW: I am trying to address the target=_blank case in https://codereview.chromium.org/2680353005 (I am currently waiting for tryjob results before sending it out for broader review - either to you or to japhet@).

+</body>

+</html>