New issue
Advanced search Search tips

Issue 718160 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 623682



Sign in to add a comment

Feature policy container policy not replicated to remote frames

Project Member Reported by iclell...@chromium.org, May 3 2017

Issue description

In OOPIFs, the container policy in remote frames in the frame tree should be taken from the replicated container policy, and not calculated from other frame owner properties.

This has shown up in the SitePerProcessInteractiveBrowserTests for Fullscreen, where the policy is not being constructed correctly for frames with remote ancestors, because the container policy is incorrectly replicated.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 5 2017

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

commit 3a69e4860efab03bba98d13bbcf4883fae7a3bc0
Author: iclelland <iclelland@chromium.org>
Date: Fri May 05 15:39:53 2017

Use replicated container policy in remote frames

BUG= 718160 

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

[modify] https://crrev.com/3a69e4860efab03bba98d13bbcf4883fae7a3bc0/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp

Cc: alex...@chromium.org
There is still an issue here somewhere with remote frames with remote frame owners.

I've seen failures in an A->B->C frame embedding scenario where the container policy for C is not replicated properly in the renderer for site A. In this case,  both the frame in question and its frame owner are remote.

I'll have to see if I can come up with a reliable repro case, but something is not being replicated, either at all, or just not early enough.

Comment 3 by creis@chromium.org, Jun 7 2017

Cc: lunalu@chromium.org
Might be related to comments 31-35 on  issue 716085 ?  That was also describing a feature policy replication problem.

Comment 4 by lunalu@chromium.org, Jun 12 2017

Cc: -lunalu@chromium.org loonyb...@chromium.org
It appears that what is happening is this:

Frame structure: A->B->C. All Iframe tags contain an allow attribute; all different origins.
- Frame A gets its policy created correctly.
- Frame A embeds frame B; policy is created correctly in browser, in renderer b, and the container policy is correct in the frameowner element in renderer a.
- Frame B embeds frame C: Container policy is correct in the frame owner element in renderer B. RenderFrameHost for rendererB receives the CreateChildFrame message, calls FrameTree::AddFrame.
- FrameTree::AddFrame makes a new FrameTreeNode with an empty container policy, then AddChild creates a proxy for Renderer A, but still with the empty container policy.
- Then AddChild sets and commits the frame policy, but doesn't send that to the proxies, since it is calling FrameTreeNode::CommitPendingFramePolicy rather than RenderFrameHostManager::CommitPendingFramePolicy. The new proxy never gets the updated container policy.
#5: Thanks for the analysis, nice find!  It does seem like FrameTree::AddFrame should be calling added_node->render_manager()->CommitPendingFramePolicy(), rather than added_node->CommitPendingFramePolicy(), due to the child frame proxies that might have been created in AddChild.  Alternatively, could we perhaps plumb container policy into FTN constructor, where they can be used to initialize the replicated state before it's sent out to proxies created by AddChild?  I think this is what we do for FrameOwnerProperties, and it would avoid the extra IPC to update container policy.  Seems like we should do this for sandbox flags too, OTOH that might be a little trickier because we also need to ensure that the parent's sandbox flags are inherited in SetPendingSandboxFlags.
Passing in the frame policy (container policy + sandbox flags) to the FTN constructor seems like a good idea; they're right there, and requiring the extra pass to initialized them separately stuck me as odd.

Alternatively, check out https://codereview.chromium.org/2955573003/ -- the simplest solution, I think, might just be to separate the two steps of creating the FTN and creating the RFH/RFP objects, and set the container policy in between those two.
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 28 2017

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

commit 098da75cd0415c2e21a7ee78e47a75abdfabf461
Author: iclelland <iclelland@chromium.org>
Date: Wed Jun 28 13:46:50 2017

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}

[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/public/web/WebLocalFrame.h

Status: Fixed (was: Started)

Sign in to add a comment