NOT YET READY: Making chrome.windows.create establish an actual "opener" relationship.

Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.

[Łukasz Anforowicz, 2017-06-01 22:02:44]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:
1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other.
2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)

BUG=713888
==========

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:
1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other.
2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)

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

[Łukasz Anforowicz, 2017-06-01 22:03:14]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 22:04:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-01 22:39:46]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:
1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other.
2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)

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

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other.

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).

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

[commit-bot: I haz the power, 2017-06-01 22:51:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 22:51:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-06-01 22:58:42]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 22:59:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 02:26:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 02:26:46]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-02 16:24:53]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other.

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).

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

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


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

[Łukasz Anforowicz, 2017-06-02 16:47:53]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).

After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489)


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


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

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

[Łukasz Anforowicz, 2017-06-02 16:48:14]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2017-06-02 16:48:14]

creis@, could you PTAL?

[Charlie Reis, 2017-06-05 21:06:28]

[+nasko to CC]

Thanks!  LGTM with some nits and a sanity check below.

CL description nit: "not quite entirely unlike" -> "not quite entirely like"
(Double negative seems to have the wrong meaning here.)

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:573: navigate_params.opener =
render_frame_host();
Worth keeping a comment saying that the intention is to stay in the same
BrowsingInstance.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:2198: // Verify that the old and
new tab are in the same process and SiteInstance.
nit: Remove "and SiteInstance" if we aren't checking that.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:2202: // Verify the old and new
contents are in the same browsing instance.
nit: BrowsingInstance

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.cc (left):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.cc:362: ? params.source_site_instance
Was this really only used for chrome.windows.create?  Huh.  I suppose having
this process decision line up with whether an opener is defined is ok, though it
might help to have a comment to that effect.

One sanity check: It appears we're changing the default behavior, but I'm not
sure if that results in an observable change.  Before,
force_new_process_for_new_contents defaulted to false, and thus we would use
params.source_site_instance for any case that didn't explicitly set it to true. 
Now, opener defaults to null, so we use GetSiteInstanceForNewTab for any case
that doesn't explicitly define it.

The cases I know about are chrome.windows.create (which set it to false before
and now sets the opener, preserving behavior) and RenderFrameHostImpl::OnOpenURL
(which set it to true before and doesn't set the opener, preserving behavior).

There are over 100 calls to Navigate, so it's possible others could be affected.
 It looks like the only one that assigns source_site_instance is
chrome.windows.create, though, so I *think* all the rest hit the
GetSiteInstanceForNewTab call before and will after as well.  Does that sound
right?

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_params.h (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_params.h:136: // Allows setting the opener
for the case when new web contents are created
nit: WebContents

[Łukasz Anforowicz, 2017-06-05 21:51:52]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite entirely unlike an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

[Łukasz Anforowicz, 2017-06-05 22:06:54]

On 2017/06/05 21:06:28, Charlie Reis wrote:
> [+nasko to CC]
> 
> Thanks!  LGTM with some nits and a sanity check below.

Ok - thanks.
> 
> CL description nit: "not quite entirely unlike" -> "not quite entirely like"
> (Double negative seems to have the wrong meaning here.)
> 

Sorry about that.  I've tweaked the description to be more straighforward.  I
was trying to force a reference to the Hitchhiker's Guide to the Galaxy, but I
think I missed a comma and otherwise messed it up.

http://www.goodreads.com/quotes/20153-after-a-fairly-shaky-start-to-the-day-a...:

“After a fairly shaky start to the day, Arthur's mind was beginning to
reassemble itself from the shell-shocked fragments the previous day had left him
with.
He had found a Nutri-Matic machine which had provided him with a plastic cup
filled with a liquid that was almost, but not quite, entirely unlike tea.
The way it functioned was very interesting. When the Drink button was pressed it
made an instant but highly detailed examination of the subject's taste buds, a
spectroscopic analysis of the subject's metabolism and then sent tiny
experimental signals down the neural pathways to the taste centers of the
subject's brain to see what was likely to go down well. However, no one knew
quite why it did this because it invariably delivered a cupful of liquid that
was almost, but not quite, entirely unlike tea.”

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:573: navigate_params.opener =
render_frame_host();
On 2017/06/05 21:06:28, Charlie Reis wrote:
> Worth keeping a comment saying that the intention is to stay in the same
> BrowsingInstance.

Done.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:2198: // Verify that the old and
new tab are in the same process and SiteInstance.
On 2017/06/05 21:06:28, Charlie Reis wrote:
> nit: Remove "and SiteInstance" if we aren't checking that.

Done.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:2202: // Verify the old and new
contents are in the same browsing instance.
On 2017/06/05 21:06:28, Charlie Reis wrote:
> nit: BrowsingInstance

Done.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.cc (left):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.cc:362: ? params.source_site_instance
On 2017/06/05 21:06:28, Charlie Reis wrote:
> Was this really only used for chrome.windows.create?  Huh.  I suppose having
> this process decision line up with whether an opener is defined is ok, though
it
> might help to have a comment to that effect.

I've added a comment here.
> 
> One sanity check: It appears we're changing the default behavior, but I'm not
> sure if that results in an observable change.  Before,
> force_new_process_for_new_contents defaulted to false, and thus we would use
> params.source_site_instance for any case that didn't explicitly set it to
true. 
> Now, opener defaults to null, so we use GetSiteInstanceForNewTab for any case
> that doesn't explicitly define it.
> 
> The cases I know about are chrome.windows.create (which set it to false before
> and now sets the opener, preserving behavior) and
RenderFrameHostImpl::OnOpenURL
> (which set it to true before and doesn't set the opener, preserving behavior).
> 
> There are over 100 calls to Navigate, so it's possible others could be
affected.
>  It looks like the only one that assigns source_site_instance is
> chrome.windows.create, though, so I *think* all the rest hit the
> GetSiteInstanceForNewTab call before and will after as well.  Does that sound
> right?

Right - the main thing here is that chrome::NavigateParams::source_site_instance
was/is only set by 1) chrome.windows.create API and 2)
NavigatorImpl::RequestOpenURL (via content::OpenURLParams).  The new behavior
should be matching expectations in these two cases.

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_params.h (right):

https://codereview.chromium.org/2921753002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_params.h:136: // Allows setting the opener
for the case when new web contents are created
On 2017/06/05 21:06:28, Charlie Reis wrote:
> nit: WebContents

Done.

[Łukasz Anforowicz, 2017-06-05 22:08:48]

lukasza@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Łukasz Anforowicz, 2017-06-05 22:08:49]

rdevlin.cronin@, could you PTAL?

[Charlie Reis, 2017-06-05 22:22:08]

On 2017/06/05 22:06:54, Łukasz A. wrote:
> On 2017/06/05 21:06:28, Charlie Reis wrote:
> > CL description nit: "not quite entirely unlike" -> "not quite entirely like"
> > (Double negative seems to have the wrong meaning here.)
> > 
> 
> Sorry about that.  I've tweaked the description to be more straighforward.  I
> was trying to force a reference to the Hitchhiker's Guide to the Galaxy, but I
> think I missed a comma and otherwise messed it up.
> 
>
http://www.goodreads.com/quotes/20153-after-a-fairly-shaky-start-to-the-day-a...:
> 
> “After a fairly shaky start to the day, Arthur's mind was beginning to
> reassemble itself from the shell-shocked fragments the previous day had left
him
> with.
> He had found a Nutri-Matic machine which had provided him with a plastic cup
> filled with a liquid that was almost, but not quite, entirely unlike tea.
> The way it functioned was very interesting. When the Drink button was pressed
it
> made an instant but highly detailed examination of the subject's taste buds, a
> spectroscopic analysis of the subject's metabolism and then sent tiny
> experimental signals down the neural pathways to the taste centers of the
> subject's brain to see what was likely to go down well. However, no one knew
> quite why it did this because it invariably delivered a cupful of liquid that
> was almost, but not quite, entirely unlike tea.”

Ha!  I missed the reference at first.  I admit that the slightly dizzying
phrasing there makes you wonder what the sentence really meant, which nicely
matches the situation in the code.  :)

Anyway, LGTM.  Devlin, I'll be curious if you see any downside to exposing the
opener here.  Might be worth reading
https://bugs.chromium.org/p/chromium/issues/detail?id=713888#c7 for context.

[Łukasz Anforowicz, 2017-06-05 23:46:22]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 23:47:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 03:29:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 03:29:36]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-08 17:09:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 17:09:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 18:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 18:15:25]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-09 14:50:19]

On 2017/06/05 22:22:08, Charlie Reis wrote:
> Devlin, I'll be curious if you see any downside to exposing the
> opener here.  Might be worth reading
> https://bugs.chromium.org/p/chromium/issues/detail?id=713888#c7 for context.

Sorry for the delay - week of summits.  I do have a few concerns here; I've
added a comment to the bug so we can discuss there.

[Łukasz Anforowicz, 2017-06-09 20:57:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 20:57:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 22:00:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 22:00:04]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Łukasz Anforowicz, 2017-06-14 03:52:59]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

[Łukasz Anforowicz, 2017-06-14 15:33:08]

Description was changed from

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========

to

==========
Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

BUG=713888
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=See above.
==========