Set frame policy correctly in newly created renderer proxies

Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were being set only in the browser, after the RenderFrameHost and any associated proxies were created, and were not being subsequently replicated to those proxies afterwards. This caused the feature policies constructed in remote frames to be incorrect.

Separately, container policies were not being replicated correctly to provisional render frames, so a frame which performed a cross-site navigation to an existing site instance would have the wrong policy after the navigation was committed.

This patch fixes both of those issues, and reinstates the disabled tests on the site-isolation trybots.

BUG=716085, 718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2955573003
Cr-Commit-Position: refs/heads/master@{#482968}
Committed: https://chromium.googlesource.com/chromium/src/+/098da75cd0415c2e21a7ee78e47a75abdfabf461

[iclelland, 2017-06-23 16:24:14]

Description was changed from

==========
Set frame policy correctly in newly created renderer proxies

BUG=
==========

to

==========
Set frame policy correctly in newly created renderer proxies

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

[iclelland, 2017-06-23 16:24:16]

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

[commit-bot: I haz the power, 2017-06-23 16:24:23]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-23 17:08:05]

Description was changed from

==========
Set frame policy correctly in newly created renderer proxies

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

to

==========
Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were
being set only in the browser, after the RenderFrameHost and any associated
proxies were created, and were not being subsequently replicated to those
proxies afterwards. This caused the feature policies constructed in remote
frames to be incorrect.

BUG=716085,718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2017-06-23 17:18:46]

iclelland@chromium.org changed reviewers:
+ alexmos@chromium.org

[iclelland, 2017-06-23 17:18:47]

+r alexmos -- this needs tests, but there currently isn't a way to query the
RenderFrameProxy directly from a browsertest, and it's the renderer-side objects
which aren't being synchronized correctly that I'd need to test.

I have an internals interface for reporting the composed Feature Policy that I'm
adding for layout tests, which might be suitable, but if you can think of a
better way to test this from inside of a SitePerProcessBrowsertest, I'm happy to
listen.

[commit-bot: I haz the power, 2017-06-23 17:44:13]

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

[commit-bot: I haz the power, 2017-06-23 17:44:13]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-06-23 18:23:55]

+site-isolation-reviews

Thanks for fixing this!  The fix looks good; a couple of suggestions below.

For testing, weren't there some layout tests that were failing because of this,
and if so, can we reenable them here?  I'm fine relying on those and/or layout
tests using your internals interface for coverage, as I also don't see an easy
way to test the feature policy bits of this in a SitePerProcessBrowserTest. 
Perhaps we can test sandbox flags instead, since they use the same path, so
likely also affected?  We could try relying on inheritance from a remote parent:
I haven't thought this all the way through, but maybe something along the lines
of:
1. start with a(a(b)), where the middle a is sandbox via <iframe sandbox>.
2. create an unsandboxed child frame from the sandboxed frame, to get a(a(b,a)).
This initializes the new proxy's in process B with empty sandbox flags.
3. create another unsandboxed child frame: a(a(b,a(a)))
4. navigate the new frame from (3) to b: a(a(b,a(b)))
5. check if that new B frame is sandboxed after it loads. It should inherit the
right sandbox flags from its remote parent (where incorrect flags were used to
create it in step 2, but which itself should have inherited sandbox flags from
its sandboxed parent).

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:202:
last_committed_entry->ClearStaleFrameEntriesForNewFrame(new_node.get());
Hmm, ClearStaleFrameEntriesForNewFrame calls InSameTreePosition, which walks up
the new node's ancestor chain.  I think that still works, since the FTN
constructor initializes the parent() pointer, but seems a bit fragile, since the
parent doesn't yet know about the new child at this point.  Can this block be
moved to be done after the AddChild, to minimize the logic that runs without the
new FTN fully in the tree?

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:206: // empty document in the frame.
Can we update this comment to explain why those need to be done before the call
to AddChild?

[iclelland, 2017-06-24 03:48:56]

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

[commit-bot: I haz the power, 2017-06-24 03:49:07]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-24 03:57:23]

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

[commit-bot: I haz the power, 2017-06-24 03:57:30]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-24 03:57:44]

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:202:
last_committed_entry->ClearStaleFrameEntriesForNewFrame(new_node.get());
On 2017/06/23 18:23:55, alexmos wrote:
> Hmm, ClearStaleFrameEntriesForNewFrame calls InSameTreePosition, which walks
up
> the new node's ancestor chain.  I think that still works, since the FTN
> constructor initializes the parent() pointer, but seems a bit fragile, since
the
> parent doesn't yet know about the new child at this point.  Can this block be
> moved to be done after the AddChild, to minimize the logic that runs without
the
> new FTN fully in the tree?

Done.

https://codereview.chromium.org/2955573003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:206: // empty document in the frame.
On 2017/06/23 18:23:55, alexmos wrote:
> Can we update this comment to explain why those need to be done before the
call
> to AddChild?

Done.

[commit-bot: I haz the power, 2017-06-24 04:17:56]

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

[commit-bot: I haz the power, 2017-06-24 04:17:59]

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

[iclelland, 2017-06-24 11:05:36]

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

[commit-bot: I haz the power, 2017-06-24 11:05:42]

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-24 12:40:12]

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

[commit-bot: I haz the power, 2017-06-24 12:40:13]

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...)

[iclelland, 2017-06-27 19:51:22]

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

[commit-bot: I haz the power, 2017-06-27 19:51:43]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-27 20:28:50]

Description was changed from

==========
Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were
being set only in the browser, after the RenderFrameHost and any associated
proxies were created, and were not being subsequently replicated to those
proxies afterwards. This caused the feature policies constructed in remote
frames to be incorrect.

BUG=716085,718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were
being set only in the browser, after the RenderFrameHost and any associated
proxies were created, and were not being subsequently replicated to those
proxies afterwards. This caused the feature policies constructed in remote
frames to be incorrect.

Separately, container policies were not being replicated correctly to
provisional render frames, so a frame which performed a cross-site navigation to
an existing site instance would have the wrong policy after the navigation was
committed.

This patch fixes both of those issues, and reinstates the disabled tests on the
site-isolation trybots.

BUG=716085,718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2017-06-27 20:31:41]

alexmos -- can you PTAL again? Thanks!

(The issue you raised in
https://bugs.chromium.org/p/chromium/issues/detail?id=716085#c35 was also in
play here, so I've corrected that as well, which gets the failing tests to start
passing again)

[alexmos, 2017-06-27 20:59:07]

LGTM

[commit-bot: I haz the power, 2017-06-27 21:59:45]

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

[commit-bot: I haz the power, 2017-06-27 21:59:46]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-06-27 22:58:06]

iclelland@chromium.org changed reviewers:
+ pfeldman@chromium.org

[iclelland, 2017-06-27 22:58:07]

+r pfeldman -- can you PTAL as OWNER in WebKit/* ? Thanks!

[iclelland, 2017-06-28 03:45:44]

iclelland@chromium.org changed reviewers:
+ jochen@chromium.org
- pfeldman@chromium.org

[iclelland, 2017-06-28 03:45:44]

n/m, pfeldman is OOO for another week or so -- jochen, do you mind taking a
quick look?

[jochen (gone - plz use gerrit), 2017-06-28 07:39:19]

lgtm

[iclelland, 2017-06-28 13:41:57]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2017-06-28 13:42:08]

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-28 13:46:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498657317735350,
"parent_rev": "a961f5f372eef024f722a49f3e921c62fb2f9f3a", "commit_rev":
"098da75cd0415c2e21a7ee78e47a75abdfabf461"}

[commit-bot: I haz the power, 2017-06-28 13:47:07]

Description was changed from

==========
Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were
being set only in the browser, after the RenderFrameHost and any associated
proxies were created, and were not being subsequently replicated to those
proxies afterwards. This caused the feature policies constructed in remote
frames to be incorrect.

Separately, container policies were not being replicated correctly to
provisional render frames, so a frame which performed a cross-site navigation to
an existing site instance would have the wrong policy after the navigation was
committed.

This patch fixes both of those issues, and reinstates the disabled tests on the
site-isolation trybots.

BUG=716085,718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were
being set only in the browser, after the RenderFrameHost and any associated
proxies were created, and were not being subsequently replicated to those
proxies afterwards. This caused the feature policies constructed in remote
frames to be incorrect.

Separately, container policies were not being replicated correctly to
provisional render frames, so a frame which performed a cross-site navigation to
an existing site instance would have the wrong policy after the navigation was
committed.

This patch fixes both of those issues, and reinstates the disabled tests on the
site-isolation trybots.

BUG=716085,718160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2955573003
Cr-Commit-Position: refs/heads/master@{#482968}
Committed:
https://chromium.googlesource.com/chromium/src/+/098da75cd0415c2e21a7ee78e47a...
==========

[commit-bot: I haz the power, 2017-06-28 13:47:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/098da75cd0415c2e21a7ee78e47a...