Can you document how other browsers behave in these cases?

+-------------------------------+----------+---------------+-----------+---------------+
|                               |   Win1   | Win2          | Main page | Main page     |
|                               | can find | can find      | can find  | can find      |
|                               |   Win0   | Win0          | Win1      | Win2          |
+-------------------------------+----------+---------------+-----------+---------------+
| Chrome (r425087)              | yes      | !!!YES!!!     | yes       | !!!YES!!!     |
+-------------------------------+----------+---------------+-----------+---------------+
| IE 11.0.9600.18449            | yes      | no (access    | yes       | no (access    |
|                               |          | denied error) |           | denied error) |
+-------------------------------+----------+---------------+-----------+---------------+
| Firefox 48.0.2                | yes      | no (opens new | yes       | no (opens new |
|                               |          | win instead)  |           | win instead)  |
+-------------------------------+----------+---------------+-----------+---------------+
| Safari 10.0 (12607.1.50.0.10) | window.open always returns undefined???              |
+-------------------------------+------------------------------------------------------+

I note that the behavior is different with --site-per-process (tested when Win2 is cross-site + when main page [i.e. Win0] has a cross-site frame):

- (SAME) Win1 can find Win0
- (DIFFERENT) Win2 cannot find Win0 (opens new window instead)

- (SAME) Main page can find Win1
- (DIFFERENT) Main page cannot find Win2 (opens new window instead)

- (NEW) Cross-site frame from main page CANNOT find Win1 (isn't this a separate bug?  everything should be in the same browsing instance, right?)
- (NEW) Cross-site frame from main page cannot find Win2 (consistent with previous item and consistent with other browsers...)

I've opened  issue 658402  to track the concern from the 2nd-to-last-item in #c3.  This seems to be orthogonal / separate from the question of whether shift-click should stay in the same browsing instance or not.

The following change:

--- a/chrome/browser/ui/browser_navigator.cc
+++ b/chrome/browser/ui/browser_navigator.cc
@@ -350,9 +350,7 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
                                            const GURL& url) {
   WebContents::CreateParams create_params(
       params.browser->profile(),
-      params.source_site_instance
-          ? params.source_site_instance
-          : tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));
+      tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));

makes this bug go away (and also fixes  issue 23815  that says that ctrl-click should open in a *new* renderer process).

FWIW, using params.source_site_instance comes from r384655 by Daniel.  OTOH, all *ExtensionApiTest*Open* browser_tests are passing, so maybe it really can be removed.

Let's first see what the trybots say in https://crrev.com/2471663002 and then let's discuss further.

My recollection (possibly wrong) is that this was needed because CreateTargetContents is used by chrome::Navigate which is used by the chrome.tabs extension API when creating a new tab.

#6: I was tinkering with some nearby code recently, and I think that's right. For example, chrome.windows.create() passes the source_site_instance in WindowsCreateFunction::Run() in tabs_api.cc, and I *think* there are tests that expect the new tab to end up in the same BrowsingInstance when that's used.    For example, I think we would want web popups opened from extensions using that API to be kept in the same BrowsingInstance per  issue 590068 .

Charlie pointed out that middle-click behaves differently when anchor's target="_blank" - indeed the new contents seem to be in the same browsing instance in this case (i.e. window.opener is set + the window can find the opened via window.open('', 'name-of-middle-clicked-frame')).  This seems rather unexpected.  I've added a test to https://crrev.com/2471663002.

ChromeNavigationBrowserTest.CtrlClickOfTargetBlankAnchorShouldEndUpInNewProcess:

Expected: (main_contents->GetMainFrame()->GetProcess()) != (new_contents->GetMainFrame()->GetProcess()), actual: 0x619000158680 vs 0x619000158680
../../chrome/browser/chrome_navigation_browsertest.cc:281: Failure
Value of: window_opener_cast_to_bool
  Actual: true
Expected: false
../../chrome/browser/chrome_navigation_browsertest.cc:294: Failure
Value of: location_of_opened_window
  Actual: "http://127.0.0.1:45736/frame_tree/anchor_to_same_site_location.html"
Expected: "about:blank"

Glad to see us moving forward here, though the targeted case is a little tricky.

I think we should look closer at the targeted link case to see whether it can be changed.  For example, Firefox sets window.opener in both target=_blank and target=foo cases (when left clicking), but it leaves them null if you middle click.  I think it might make a lot of sense for Chrome to match that, making it easier to switch processes in this case (and thus fix  issue 23815 ).

Maybe we should compare across all browsers and/or see if the spec has something to say on it?

Interestingly, the tryjobs at https://crrev.com/2471663002/#ps40001 turned out mostly green.  The only red result was the new test for the target="_blank" pointed out in #c8.

I guess the next step is to figure out if extension apis (e.g. chrome.windows.create) have test coverage wrt staying in the same browsing instance (and either adding the coverage, or checking why the tests continue to pass with the changes described in #c5).

I've just found out that the changes from https://codereview.chromium.org/2471663002/patch/40001/50002 regress  https://crbug.com/597750  :-(

This means that the desired behavior is:
1a. Ctrl-click and 1b. Shift-Ctrl-clik => new process and browsing instance
2. chrome.tabs.create => same browsing instance

We probably need to somehow differentiate between #1 and #2 in CreateTargetContents in chrome/browser/ui/browser_navigator.cc despite the fact that both 1b and 2 use the same window disposition (NEW_FOREGROUND_TAB).

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

commit 69e056006f20f2973eafb2e95b7ea4395f3065db
Author: lukasza <lukasza@chromium.org>
Date: Mon Apr 10 21:29:59 2017

Tests of ctrl-click can't postMessage to opener - using custom text instead.

This CL fixes/refactors two layout tests that used to depend on ability
to find the opener via window.open, even in case of ctrl-click or
shift-click.  This dependency is incorrect and will be broken once
 https://crbug.com/658386  is fixed.

The tests had to be refactored so that they don't depend on ability to
communicate back with the main test window.  After this CL the tests
report their results via testRunner.setCustomTextOutput (rather than
sending the results back to the main window using window.postMessage).

In the 2 tests here, the custom text dump is being set from a secondary
test window - the CL also needs to relax the condition that limited
propagation of LayoutTestRuntimeFlags to the main test window.  The
intent of the original condition should have covered both secondary
and main windows (i.e. the intent was to just check if the renderer
has received a LayoutTestRuntimeFlags).  It seems desirable overall
to also propagate LayoutTestRuntimeFlags changes originating from
secondary test windows.

BUG= 658386 

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

[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/content/shell/test_runner/test_interfaces.cc
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/content/shell/test_runner/test_interfaces.h
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/content/shell/test_runner/web_test_interfaces.cc
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/content/shell/test_runner/web_test_interfaces.h
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/navigation/post-with-modifier-expected.txt
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/navigation/post-with-modifier.html
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/navigation/resources/post-with-modifier.php
[add] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-vs-shift-click-expected.txt
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-vs-shift-click.html
[modify] https://crrev.com/69e056006f20f2973eafb2e95b7ea4395f3065db/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-src-vs-shift-click-target.html

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

commit f206da906ef1e94ee03feeb67e11a82c565baa3b
Author: lukasza <lukasza@chromium.org>
Date: Thu Apr 20 23:26:51 2017

New WebContents created via ctrl-click should be in a new process.

This CL puts web contents created via ctrl-click (or shift-click, etc.)
into a new process (fixing  https://crbug.com/23815 ) and makes sure the
new web contents are in a new browsing instance (fixing
 https://crbug.com/658386  and bringing consistency with the behavior of
other browsers).

This CL also adds a test that verifies that the new web contents created
by chrome.windows.create API are in the same "browsing instance" as the
caller (i.e. that the windows can find and script each other).  This is
a regression test for  https://crbug.com/597750  that was broken by my
earlier attempts to fix  https://crbug.com/23815  (and AFAICT the
regression wasn't caught by existing tests / tryjobs).  FWIW, I've also
manually tested the Hangouts extension when launching Chrome (built with
this CL) with --isolate-extensions flag.

BUG= 23815 ,  597750 ,  658386 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/ui/browser_navigator_params.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/browser/ui/browser_navigator_params.h
[add] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/chrome/test/data/frame_tree/anchor_to_same_site_location.html
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/browser/frame_host/navigator.h
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/public/browser/page_navigator.cc
[modify] https://crrev.com/f206da906ef1e94ee03feeb67e11a82c565baa3b/content/public/browser/page_navigator.h

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

commit be2f0da0b064edc7e59d08129538a09d3b2f30c1
Author: lukasza <lukasza@chromium.org>
Date: Tue Apr 25 00:43:00 2017

WebContents created via ctrl-click should be in a new process (target=_blank).

Previous CL (https://crrev.com/2686943002) started putting WebContents
created via ctrl-click into a new process.  One case that was still
broken is target="_blank" (or target="non-existant-name").

At first glance, it might seem arbitrary to decide which directive
(ctrl-click VS target=_blank) wins and influences 1) process allocation
behavior and 2) window.opener / browsing instance behavior.  In reality,
ctrl-click should win because:
1. This is the behavior desired by users (e.g. see
    https://crbug.com/23815 ).
2. This is the behavior of other browsers (e.g. see
    https://crbug.com/658386#c9 ).

Before this CL, FrameLoader::load would ignore ctrl-click /
NavigationPolicyNewForegroundTab and go through createWindowForRequest
path if the named target frame didn't exist when the navigation is
initiated.  After this CL, ctrl-click "wins".

This CL has to tweak 2 layout tests affected by the change.  In both
cases, the test expectation that a background-tab-navigation is handled
by createView is no longer correct - now the navigation goes through
OpenURL path rather than through CreateWindowForRequest.  Accomodating
the 2 tests also required adding support for handling
WindowOpenDisposition::NEW_BACKGROUND_TAB in
content::Shell::OpenURLFromTab.

NOPRESUBMIT usage has been discussed in https://crrev.com/2680353005/#msg50

BUG= 23815 ,  658386 
NOPRESUBMIT=true

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

[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/chrome/test/data/frame_tree/anchor_to_same_site_location.html
[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/content/shell/browser/shell.cc
[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/third_party/WebKit/LayoutTests/fast/events/isolated-worlds-override-keystate-expected.txt
[delete] https://crrev.com/6aee4d67dcc67850c8ec7b9fc181cb7d91b55734/third_party/WebKit/LayoutTests/fast/loader/create-view-target-blank-expected.txt
[add] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank-expected.txt
[rename] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/third_party/WebKit/LayoutTests/fast/loader/middle-click-target-blank.html
[modify] https://crrev.com/be2f0da0b064edc7e59d08129538a09d3b2f30c1/third_party/WebKit/Source/core/loader/FrameLoader.cpp

 Issue 718204  has been merged into this issue.

Note that there is some web compat risk here, see https://bugzilla.mozilla.org/show_bug.cgi?id=1357845 for one such report.

From what I've read I think the risk is manageable and we don't need to change anything.  But please link to any site compat bugs caused as a result of this change.  If we have substantial reports of issues we may need to revert temporarily and do a more careful compat analysis / blink intent for the change in behavior.

Adding 'Blink>WindowDialog' to indicate that the behavior of window.open has changed in M-60 to make it easier to find for people searching for such changes (I resorted to requesting a bisect in  issue 718204 ).

My analysis on  issue 718204  was incorrect; I misunderstood the test case. We match Firefox and Safari now, but Edge matches our old behavior.