Enforce all subframes to be in the same BrowsingInstance as the parent.

Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e8e623d5bf2b4ab72b66fbb6b6e649c66adbd46f
Cr-Commit-Position: refs/heads/master@{#363292}

[nasko, 2015-12-03 17:59:53]

Patchset #1 (id:1) has been deleted

[nasko, 2015-12-04 16:55:26]

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

[nasko, 2015-12-04 16:55:27]

Hey Charlie,
Can you review this CL for me? It is the simpler fix for the extensions
BrowsingInstance bug.
Thanks in advance!
Nasko

[nasko, 2015-12-04 16:56:08]

Description was changed from

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
==========

to

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2015-12-04 18:23:23]

Great.  LGTM with nits.

https://codereview.chromium.org/1495993002/diff/70001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1495993002/diff/70001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1272: // Subframe must
stay in the same BrowsingInstance as its parent.
nit: A subframe

Do we want a TODO for "Ensure that we still trigger SiteInstance swaps in
subframes for the cases checked below"?

https://codereview.chromium.org/1495993002/diff/70001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1495993002/diff/70001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:58: // Any child frame must
be in the same BrowsingInstace as its parent.
nit: BrowsingInstance

[nasko, 2015-12-04 18:32:33]

nasko@chromium.org changed reviewers:
+ sky@chromium.org

[nasko, 2015-12-04 18:32:33]

sky@, can you do an OWNERS stamp on this? It is a trivial update of expectations
in a test.

https://codereview.chromium.org/1495993002/diff/70001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1495993002/diff/70001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1272: // Subframe must
stay in the same BrowsingInstance as its parent.
On 2015/12/04 18:23:23, Charlie Reis wrote:
> nit: A subframe
> 
> Do we want a TODO for "Ensure that we still trigger SiteInstance swaps in
> subframes for the cases checked below"?

Done.

https://codereview.chromium.org/1495993002/diff/70001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1495993002/diff/70001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:58: // Any child frame must
be in the same BrowsingInstace as its parent.
On 2015/12/04 18:23:23, Charlie Reis wrote:
> nit: BrowsingInstance

Done.

[nasko, 2015-12-04 18:32:46]

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

[commit-bot: I haz the power, 2015-12-04 18:36:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495993002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495993002/90001

[nasko, 2015-12-04 20:01:55]

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

[nasko, 2015-12-04 20:01:56]

Adding Devlin to for visibility and to ensure this is the desired behavior.

[Devlin, 2015-12-04 20:04:41]

LGTM

[commit-bot: I haz the power, 2015-12-04 20:07:33]

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

[commit-bot: I haz the power, 2015-12-04 20:07:34]

Dry run: This issue passed the CQ dry run.

[sky, 2015-12-04 20:33:53]

LGTM

[nasko, 2015-12-04 20:54:14]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-12-04 20:54:14]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1495993002/#ps90001
(title: "Fixes based on Charlie's review.")

[commit-bot: I haz the power, 2015-12-04 20:54:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495993002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495993002/90001

[commit-bot: I haz the power, 2015-12-04 21:00:36]

Description was changed from

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2015-12-04 21:00:37]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2015-12-04 21:01:32]

Description was changed from

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enforce all subframes to be in the same BrowsingInstance as the parent.

Parent and child frames must be part of the same "unit of related
browsing contexts" per the HTML spec. This rule is currently broken when
subframes are allowed to perform cross-process navigations. This CL is
implementing a simple fix for this and adding an enforcement of this rule
to WebContentsObserverSanityChecker. A more comprehensive implementation
can be done at a later time.

BUG=522302
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e8e623d5bf2b4ab72b66fbb6b6e649c66adbd46f
Cr-Commit-Position: refs/heads/master@{#363292}
==========

[commit-bot: I haz the power, 2015-12-04 21:01:33]

Patchset 5 (id:??) landed as
https://crrev.com/e8e623d5bf2b4ab72b66fbb6b6e649c66adbd46f
Cr-Commit-Position: refs/heads/master@{#363292}