Replicate feature policy container policies.

Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

Review-Url: https://codereview.chromium.org/2797813002
Cr-Commit-Position: refs/heads/master@{#465563}
Committed: https://chromium.googlesource.com/chromium/src/+/92f8c0b3ff53422eb761b53793bac0b424ec45df

[iclelland, 2017-04-04 19:28:52]

Description was changed from

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

BUG=682258
==========

to

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

[iclelland, 2017-04-04 19:29:30]

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

[commit-bot: I haz the power, 2017-04-04 19:30:12]

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-04-04 19:50:18]

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

[commit-bot: I haz the power, 2017-04-04 19:50:18]

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

[iclelland, 2017-04-04 20:23:38]

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

[commit-bot: I haz the power, 2017-04-04 20:24:26]

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-04-04 21:46:33]

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

[commit-bot: I haz the power, 2017-04-04 21:46:34]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-04-04 23:02:11]

iclelland@chromium.org changed reviewers:
+ lunalu@chromium.org, raymes@chromium.org

[iclelland, 2017-04-04 23:02:12]

Still cleaning this up a bit, but the basic idea is here.

[iclelland, 2017-04-05 02:18:55]

On 2017/04/04 23:02:12, iclelland wrote:
> Still cleaning this up a bit, but the basic idea is here.

This CL contains a number of tightly-related changes; I could split them up into
patches in a single CL, but they don't make sense as CLs on their own:

1. Add containerPolicy to FrameOwner and to HTMLFrameOwnerElement, and update
the computed policy when FP-related attributes change
2. Add pending_container_policy to FrameTreeNode, and committed container_policy
to FrameReplicationState
3. Add container policy alongside sandbox flags in replication methods, to
ensure that container policy is replicated the same way, and snapshotted at the
same time, as the sandbox attributes.
4. Use committed container policy when constructing full policies in the browser
and in out-of-process frames.
5. Rename "*SandboxFlags*" methods and IPC messages to "*FramePolicy*", to
express the idea that the sandbox flags and container policy together comprise
the Frame policy attributes.
6. Update tests

[iclelland, 2017-04-05 02:19:08]

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:328: // Validate that inherited
policy matches?
This isn't necessary; the container policy can be anything, really, unlike
sandbox flags. When combined with the parent policy, it will be enforced that
the child frame cannot pick up any features which the parent frame did not
already have access to.

https://codereview.chromium.org/2797813002/diff/20001/content/common/frame_ow...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2797813002/diff/20001/content/common/frame_ow...
content/common/frame_owner_properties.h:11: #include
"content/common/feature_policy/feature_policy.h"
Can be removed now.

[raymes, 2017-04-05 05:24:38]

Hey Ian, I had a look through. One high level question I have is why we
replicate the container policy in addition to replicating allow attributes, etc.
In theory couldn't we get away with only replicating one of them? Or maybe I'm
missing something? It does look like a similar thing is happening for sandbox
flags. This is where alexmos might be a better reviewer than me :)

You may want to break it up for the sake of other reviewers who are likely to
have more input than me, but it's up to you.

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:229: // Update this frame's
container policy.  This is used when a parent frame
nit: double space

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:414: ParsedFeaturePolicyHeader
pending_container_policy_;
Were we still planning to try to rename ParsedFeaturePolicyHeader to reflect
this usage? (I can't remember if we had a CL for that)

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3538: // TODO(iclelland):
Get the frame owner properties here to reset properly.
nit: Can this be removed now?

[raymes, 2017-04-05 05:43:22]

https://codereview.chromium.org/2797813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:229:
allowedFeatures(), SecurityOrigin::create(m_absoluteURL));
Hmm, how do payment request and fullscreen get factored in?

[iclelland, 2017-04-05 14:15:53]

On 2017/04/05 05:24:38, raymes wrote:
> Hey Ian, I had a look through. One high level question I have is why we
> replicate the container policy in addition to replicating allow attributes,
etc.
> In theory couldn't we get away with only replicating one of them? Or maybe I'm
> missing something?

No, you're not missing it; I actually do want to remove the replication of
allowedfeatures and the explicit allow attributes, they shouldn't be needed at
all anymore. This CL was big enough already :)

> It does look like a similar thing is happening for sandbox
> flags. This is where alexmos might be a better reviewer than me :)
> 
> You may want to break it up for the sake of other reviewers who are likely to
> have more input than me, but it's up to you.

[iclelland, 2017-04-05 14:51:13]

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:229: // Update this frame's
container policy.  This is used when a parent frame
On 2017/04/05 05:24:38, raymes wrote:
> nit: double space

Done.

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:414: ParsedFeaturePolicyHeader
pending_container_policy_;
On 2017/04/05 05:24:38, raymes wrote:
> Were we still planning to try to rename ParsedFeaturePolicyHeader to reflect
> this usage? (I can't remember if we had a CL for that)

I think that was one of the pending renames (pending finding a better name for
it). I don't see an oustanding CL for it though.

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2797813002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3538: // TODO(iclelland):
Get the frame owner properties here to reset properly.
On 2017/04/05 05:24:38, raymes wrote:
> nit: Can this be removed now?

Yes, thanks. Forgot to remove the comment when I TODID it.

https://codereview.chromium.org/2797813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:229:
allowedFeatures(), SecurityOrigin::create(m_absoluteURL));
On 2017/04/05 05:43:22, raymes wrote:
> Hmm, how do payment request and fullscreen get factored in?

Luna was doing that separately in https://codereview.chromium.org/2767983003/

[iclelland, 2017-04-05 14:53:20]

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

[iclelland, 2017-04-05 14:53:20]

+r alexmos -- could you PTAL? 

As I mentioned to raymes, if this is way too big to review for a single patch, I
can split it into separate patches in a single CL, but separate CLs (which need
to all make sense pass tests) may be difficult.

[lunalu1, 2017-04-05 22:30:03]

Thanks for doing this Ian. 
I just have one question:
I don't see container policy being created but rather just passed around. Where
is container policy being parsed?

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:207: // Set sandbox flags and make them
effective immediately, since initial
nit: update the comment so that it reflects on both sandbox flags and container
policy

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:373: bool did_change_flags =
nit: s/did_change_flags/did_change_frame_policy/

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:375: pending_container_policy_ !=
replication_state_.container_policy;
container policy is a white-list (WebVector<WebParsedFeaturePolicy>). I am
wondering if we have the same policy, but the items are out of order, or the
WebParsedFeaturePolicy is the same order, but inside the origins are in
different order. Do we need to update it?

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:376:
replication_state_.sandbox_flags = pending_sandbox_flags_;
We don't need to reassign the value, unless the value has been changed right? So
I am wondering if it would be more efficient to only reassign when the value is
changed. But that might be a few more lines of code (so probably not worth it).

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:211: // along with the origin of
the iframe's src attribute (which may be different
This is just a comment: we need layout tests for updating container policy by
updating allow="..." and test if the policy gets updated (which it should not).

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2224: const
GURL url1("http://foo1");
This might not be necessary, but I see this pattern of 
OnCreateChildFrame(
process()->GetNextRoutingID(), blink::WebTreeScopeType::Document,
std::string(), <some unique name>, blink::WebSandboxFlags::None,
ParsedFeaturePolicyHeader(), FrameOwnerProperties());

Do you think we can wrap it around with another method?

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3538:
ParsedFeaturePolicyHeader container_policy =
So I am not so sure how this work, if the current frame is the top frame (i.e.,
main frame), does it mean it will also have a container policy (but an empty one
instead)?

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

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:541: // Return early if
there were no pending sandbox flags updates.
nit: update the comment

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
content/common/feature_policy/feature_policy.cc:48: std::tie(rhs.feature,
rhs.matches_all_origins, rhs.origins);
If lhs.origins = [a.com, b.com, c.com], rhs.origins = [c.com, b.com, a.com], and
the rest are the same, do you think == will return true? Or should it return
false in this case?

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:131:
FeaturePolicyHeaderToWeb(replicated_state.container_policy),
same here

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:257: const ParsedFeaturePolicyHeader&
container_policy) {
Can we rename ParsedFeaturePolicyHeader to ParsedFeaturePolicy, or leave a TODO
here?

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:239:
updateContainerPolicy();
I don't think we need to update it here?

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:328: m_absoluteURL
= url;
it seems like this is the only place this url is used. Why can't we just pass
this url to updateContainerPolicy() so that we don't need to store it as a class
member?

[raymes, 2017-04-05 22:35:39]

On 2017/04/05 14:15:53, iclelland wrote:
> On 2017/04/05 05:24:38, raymes wrote:
> > Hey Ian, I had a look through. One high level question I have is why we
> > replicate the container policy in addition to replicating allow attributes,
> etc.
> > In theory couldn't we get away with only replicating one of them? Or maybe
I'm
> > missing something?
> 
> No, you're not missing it; I actually do want to remove the replication of
> allowedfeatures and the explicit allow attributes, they shouldn't be needed at
> all anymore. This CL was big enough already :)

Ah ok. Just to make sure I'm understanding correctly, we will be able to remove:
-allow_fullscreen
-allow_payment_request
-allowed_features

from FrameOwnerProperties. Is that true? If so, that sounds great :)

[raymes, 2017-04-05 22:41:02]

On 2017/04/05 22:35:39, raymes wrote:
> On 2017/04/05 14:15:53, iclelland wrote:
> > On 2017/04/05 05:24:38, raymes wrote:
> > > Hey Ian, I had a look through. One high level question I have is why we
> > > replicate the container policy in addition to replicating allow
attributes,
> > etc.
> > > In theory couldn't we get away with only replicating one of them? Or maybe
> I'm
> > > missing something?
> > 
> > No, you're not missing it; I actually do want to remove the replication of
> > allowedfeatures and the explicit allow attributes, they shouldn't be needed
at
> > all anymore. This CL was big enough already :)
> 
> Ah ok. Just to make sure I'm understanding correctly, we will be able to
remove:
> -allow_fullscreen
> -allow_payment_request
> -allowed_features
> 
> from FrameOwnerProperties. Is that true? If so, that sounds great :)

I also forgot to ask if you had thought about testing for the iframe attribute?

[raymes, 2017-04-05 22:41:04]

On 2017/04/05 22:35:39, raymes wrote:
> On 2017/04/05 14:15:53, iclelland wrote:
> > On 2017/04/05 05:24:38, raymes wrote:
> > > Hey Ian, I had a look through. One high level question I have is why we
> > > replicate the container policy in addition to replicating allow
attributes,
> > etc.
> > > In theory couldn't we get away with only replicating one of them? Or maybe
> I'm
> > > missing something?
> > 
> > No, you're not missing it; I actually do want to remove the replication of
> > allowedfeatures and the explicit allow attributes, they shouldn't be needed
at
> > all anymore. This CL was big enough already :)
> 
> Ah ok. Just to make sure I'm understanding correctly, we will be able to
remove:
> -allow_fullscreen
> -allow_payment_request
> -allowed_features
> 
> from FrameOwnerProperties. Is that true? If so, that sounds great :)

I also forgot to ask if you had thought about testing for the iframe attribute?

[alexmos, 2017-04-06 00:44:22]

Thanks, a couple of high-level questions below, and I'm also curious about some
of raymes's and lunalu's questions.

Is the goal here to expose the effective container feature policy in the browser
process and OOPIFs, because you don't currently know whether the replicated
'allow' attributes took effect, since they're replicated immediately?

Generally, I've been wanting to find a way to clean up sandbox flags replication
for quite some time now.  They sound like they should belong in
FrameOwnerProperties, but the pending/active distinction makes that difficult. 
It sounds like now we actually want two different kinds of FrameOwnerProperties
- those that take effect immediately (margins, scrolling, etc.) and are
replicated as such, and those that take effect only on next navigation (sandbox
flags and now feature policy).  For the latter, we should think about making it
simpler to add another attribute to that list in the future (it's a lot of
plumbing currently, as evidenced by this CL).  (Or is the thinking that all
future stuff should be added to feature policy instead?)

Do you think it makes sense to further combine sandbox flags with feature policy
and return something that includes them both (ContainerPolicy?) from 
pending_container_policy() and effective_container_policy()?  And similarly, use
a structure that includes both to pass around in createChildFrame, etc?  It'd be
nice to have only one generalized notion of pending/effective "container" things
on FTN and RFH.

Also, what are the use cases for needing both pending and effective container
feature policy in the browser process?  (I'm especially curious if we actually
need to expose the pending policy, or if it's just an implementation detail of
the effective policy.)

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:210:
added_node->SetPendingContainerPolicy(container_policy);
If we're combining the commit step, perhaps it also makes sense to combine this
with SetPendingSandboxFlags?

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:411: // allowfullscreen,
allowpaymentrequest, allow or src attributes are changed,
I thought allowfullscreen took effect immediately, or at least it used to. 
Would this be changing as part of combining it with feature policy?

https://codereview.chromium.org/2797813002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:819: // conatainer policy.
nit: container

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:229:
allowedFeatures(), SecurityOrigin::create(m_absoluteURL));
Is creating an origin directly from the src URL really the right thing for
feature policy?  Would it work for cases like about:blank which inherits the
parent origin, iframe sandbox flags which would typically make the frame's real
origin unique, etc?

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:133: KURL
m_absoluteURL;
Why do we need to track this?  Could you get it instead just by looking up the
src attribute?  (I recall some Blink folks weren't fond of tracking too many
(slightly) different kinds of URLs, at least on Document.  dcheng@ knows the
historical perspective on that)

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:156: BLINK_EXPORT void
setFrameOwnerContainerPolicy(
Does it make sense to also merge this with setFrameOwnerSandboxFlags?

[iclelland, 2017-04-06 03:38:45]

On 2017/04/06 00:44:22, alexmos wrote:
> Thanks, a couple of high-level questions below, and I'm also curious about
some
> of raymes's and lunalu's questions.
> 
> Is the goal here to expose the effective container feature policy in the
browser
> process and OOPIFs, because you don't currently know whether the replicated
> 'allow' attributes took effect, since they're replicated immediately?

That's exactly it. I had started by adding containerPolicy to FrameOwner, and
replicating it like the allow attributes, but realized that it's much closer to
sandbox flags, because it should be snapshotted in the same way on navigation.

I apologize for being insufficiently bold here :) I was hoping to make it easier
to review, but came to some of the same conclusions that you mentioned below:
There really are just two classes of attributes, live and snapshotted, and it
would make sense to explicitly group them, and hopefully make it easier to
extend in the future.

> Generally, I've been wanting to find a way to clean up sandbox flags
replication
> for quite some time now.  They sound like they should belong in
> FrameOwnerProperties, but the pending/active distinction makes that difficult.

> It sounds like now we actually want two different kinds of
FrameOwnerProperties
> - those that take effect immediately (margins, scrolling, etc.) and are
> replicated as such, and those that take effect only on next navigation
(sandbox
> flags and now feature policy).  For the latter, we should think about making
it
> simpler to add another attribute to that list in the future (it's a lot of
> plumbing currently, as evidenced by this CL). 

I'll see if I can make that happen, and either update this or submit it as a new
CL.

> (Or is the thinking that all future stuff should be added to feature policy
instead?)

I don't know if *everything* is going to be added there; most of the sandbox
flags will likely end up as policy-controlled feature, but I can certainly
imagine attributes that wouldn't fit into that framework.

> 
> Do you think it makes sense to further combine sandbox flags with feature
policy
> and return something that includes them both (ContainerPolicy?) from 
> pending_container_policy() and effective_container_policy()?  And similarly,
use
> a structure that includes both to pass around in createChildFrame, etc?  It'd
be
> nice to have only one generalized notion of pending/effective "container"
things
> on FTN and RFH.

Definitely -- I've named that concept "frame policy" here, being the composite
of the sandbox flags and the FP container policy. I don't think it would take
much to combine those two into a single structure.

> Also, what are the use cases for needing both pending and effective container
> feature policy in the browser process?  (I'm especially curious if we actually
> need to expose the pending policy, or if it's just an implementation detail of
> the effective policy.)

I don't think there are any such use cases; the pending_container_policy()
getter method could easily be removed; I added it there for completeness,
really.

[iclelland, 2017-04-07 20:53:44]

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

[commit-bot: I haz the power, 2017-04-07 20:54:10]

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-04-07 22:26:50]

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

[commit-bot: I haz the power, 2017-04-07 22:26:50]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-04-09 03:25:54]

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:207: // Set sandbox flags and make them
effective immediately, since initial
On 2017/04/05 22:30:03, loonybear wrote:
> nit: update the comment so that it reflects on both sandbox flags and
container
> policy

Done.

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:210:
added_node->SetPendingContainerPolicy(container_policy);
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> If we're combining the commit step, perhaps it also makes sense to combine
this
> with SetPendingSandboxFlags?

I hadn't done this; SetPendingSandboxFlags is also called from
WebContents::Create for a special case with popups, where I don't think we also
want or need to change the container policy explicitly.

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:373: bool did_change_flags =
On 2017/04/05 22:30:03, loonybear wrote:
> nit: s/did_change_flags/did_change_frame_policy/

Separated into two appropriately-named flags instead, in light of the comment
below.

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:376:
replication_state_.sandbox_flags = pending_sandbox_flags_;
On 2017/04/05 22:30:03, loonybear wrote:
> We don't need to reassign the value, unless the value has been changed right?
So
> I am wondering if it would be more efficient to only reassign when the value
is
> changed. But that might be a few more lines of code (so probably not worth
it).

Done.

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:411: // allowfullscreen,
allowpaymentrequest, allow or src attributes are changed,
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> I thought allowfullscreen took effect immediately, or at least it used to. 
> Would this be changing as part of combining it with feature policy?

Yes, it would -- there was an intent to make this change sent a while ago, and
making it part of feature policy will actually make the change.

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3538:
ParsedFeaturePolicyHeader container_policy =
On 2017/04/05 22:30:03, loonybear wrote:
> So I am not so sure how this work, if the current frame is the top frame
(i.e.,
> main frame), does it mean it will also have a container policy (but an empty
one
> instead)?

Yes, the main frame should have an empty container policy (basically a
std::vector with 0 elements)

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

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:541: // Return early if
there were no pending sandbox flags updates.
On 2017/04/05 22:30:03, loonybear wrote:
> nit: update the comment

Done.

https://codereview.chromium.org/2797813002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:819: // conatainer policy.
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> nit: container

Done.

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:131:
FeaturePolicyHeaderToWeb(replicated_state.container_policy),
On 2017/04/05 22:30:03, loonybear wrote:
> same here

Sorry, not sure what this refers to

https://codereview.chromium.org/2797813002/diff/40001/content/renderer/render...
content/renderer/render_frame_proxy.cc:257: const ParsedFeaturePolicyHeader&
container_policy) {
On 2017/04/05 22:30:03, loonybear wrote:
> Can we rename ParsedFeaturePolicyHeader to ParsedFeaturePolicy, or leave a
TODO
> here?

I'll file another CL to rename that, to avoid making this any larger than
necessary

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:229:
allowedFeatures(), SecurityOrigin::create(m_absoluteURL));
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> Is creating an origin directly from the src URL really the right thing for
> feature policy?  Would it work for cases like about:blank which inherits the
> parent origin, iframe sandbox flags which would typically make the frame's
real
> origin unique, etc?

The way we have defined it in the spec, specifying a frame policy like `<iframe
src="url" allow="feature">` should construct a policy that enables the feature
in the frame, just for the origin of the document specified by "url". If the
framed document subsequently navigates itself to a different domain, then the
feature will not be enabled in the navigated-to document. (There are other
mechanisms for specifying that all origins should be allowed, but the default is
to respect the src attribute as the intention of the parent document).

about:blank is an interesting case that I hadn't considered. Perhaps rather than
storing m_absoluteURL, I should factor that out into a method here, something
like 'getEffectiveFeaturePolicyOrigin' that would handle all of the edge cases
and return the origin that should be used.

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:239:
updateContainerPolicy();
On 2017/04/05 22:30:03, loonybear wrote:
> I don't think we need to update it here?

Thanks; that was older code from a previous iteration I should have removed.
Done.

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:328: m_absoluteURL
= url;
On 2017/04/05 22:30:03, loonybear wrote:
> it seems like this is the only place this url is used. Why can't we just pass
> this url to updateContainerPolicy() so that we don't need to store it as a
class
> member?

Thanks for catching that -- we don't need to anymore, so I've removed it.

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:133: KURL
m_absoluteURL;
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> Why do we need to track this?  Could you get it instead just by looking up the
> src attribute?  (I recall some Blink folks weren't fond of tracking too many
> (slightly) different kinds of URLs, at least on Document.  dcheng@ knows the
> historical perspective on that)

My concern here was that the absolute URL isn't just what's in the src
attribute, various kinds of relative URLs need to combine with the parent
frame's base to determine how the src attribute is interpreted.

I was thinking that that meant we needed to hold on to it for longer than just
in |HTMLFrameOwnerElement::loadOrRedirectSubframe|, but as Luna pointed out, the
new code doesn't actually use it anywhere else, so I don't think we need to
store it.

I've removed it, and, since this class doesn't have any other concept of
location, moved the responsibility for calculating the relevant origin into
HTMLFrameElementBase.

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2797813002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:156: BLINK_EXPORT void
setFrameOwnerContainerPolicy(
On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> Does it make sense to also merge this with setFrameOwnerSandboxFlags?

It does, and it'll go a step further towards just combining them into a single
struct later. Done.

[iclelland, 2017-04-10 19:59:35]

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

[commit-bot: I haz the power, 2017-04-10 20:00:12]

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-04-10 22:11:08]

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

[commit-bot: I haz the power, 2017-04-10 22:11:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2017-04-11 01:17:50]

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:210:
added_node->SetPendingContainerPolicy(container_policy);
On 2017/04/09 03:25:54, iclelland wrote:
> On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> > If we're combining the commit step, perhaps it also makes sense to combine
> this
> > with SetPendingSandboxFlags?
> 
> I hadn't done this; SetPendingSandboxFlags is also called from
> WebContents::Create for a special case with popups, where I don't think we
also
> want or need to change the container policy explicitly.

Ack.  I was thinking you could pass in an empty policy for that special case,
but I also agree it might not make much sense to be setting a container policy
on a root frame (unlike sandbox flags).

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:411: // allowfullscreen,
allowpaymentrequest, allow or src attributes are changed,
On 2017/04/09 03:25:54, iclelland wrote:
> On 2017/04/06 00:44:22, alexmos (OOO on 4-7) wrote:
> > I thought allowfullscreen took effect immediately, or at least it used to. 
> > Would this be changing as part of combining it with feature policy?
> 
> Yes, it would -- there was an intent to make this change sent a while ago, and
> making it part of feature policy will actually make the change.

Acknowledged.

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
content/common/feature_policy/feature_policy.cc:48: std::tie(rhs.feature,
rhs.matches_all_origins, rhs.origins);
On 2017/04/05 22:30:03, loonybear wrote:
> If lhs.origins = [a.com, b.com, c.com], rhs.origins = [c.com, b.com, a.com],
and
> the rest are the same, do you think == will return true? Or should it return
> false in this case?

+1, just bumping this up to ensure we don't forget this.  I'm also curious what
happens if you declare a duplicate policy, as in ["b.com", "b.com"] - should
that be considered equal to ["b.com"]?

It probably doesn't matter much in places like CommitPendingFramePolicy, but I
do wonder if there are reasons to think about the whitelist as a vector rather
than a set.

https://codereview.chromium.org/2797813002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:326: pending_container_policy_ =
container_policy;
This is expected to only done for subframe nodes, right?  Can we add a DCHECK
and update the comment in the header about that?

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
nit: use IsSandboxed(kSandboxOrigin)

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
Checking data: explicitly might not actually be necessary, as
SecurityOrigin::Create should handle it when checking
ShouldTreatAsUniqueOrigin().  Please double-check - I haven't followed all the
latest plumbing.  It's probably best to leave this decision to
SecurityOrigin::Create anyway, in case other schemes with unique origins are
added later.

I also know file URLs can lead to effectively unique origins (depending on a
setting which I think is on by default - see last case in
SecurityOrigin::IsSameSchemeHostPort).  Does that matter here?  A unit test for
all these corner cases would be nice.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:174: if
(url.ProtocolIsJavaScript() || url.IsAboutBlankURL())
Do you also need to handle the case where |url| is empty (i.e., no "src"
specified)?  

I wonder if we should call ShouldInheritSecurityOriginFromOwner instead to
maintain a central place for this decision.

Also, what about srcdoc iframes?

[iclelland, 2017-04-11 17:41:33]

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

[iclelland, 2017-04-11 17:41:43]

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:375: pending_container_policy_ !=
replication_state_.container_policy;
On 2017/04/05 22:30:03, loonybear wrote:
> container policy is a white-list (WebVector<WebParsedFeaturePolicy>). I am
> wondering if we have the same policy, but the items are out of order, or the
> WebParsedFeaturePolicy is the same order, but inside the origins are in
> different order. Do we need to update it?

We don't need to, in that case, but in practice that's not going to happen until
we have something like the full JSON-allow-syntax on iframe elements. And I
believe that even if it does, there is no harm in updating with a
semantically-identical container policy anyway.

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2797813002/diff/40001/content/common/feature_...
content/common/feature_policy/feature_policy.cc:48: std::tie(rhs.feature,
rhs.matches_all_origins, rhs.origins);
On 2017/04/11 01:17:49, alexmos wrote:
> On 2017/04/05 22:30:03, loonybear wrote:
> > If lhs.origins = [a.com, b.com, c.com], rhs.origins = [c.com, b.com, a.com],
> and
> > the rest are the same, do you think == will return true? Or should it return
> > false in this case?
> 
> +1, just bumping this up to ensure we don't forget this.  I'm also curious
what
> happens if you declare a duplicate policy, as in ["b.com", "b.com"] - should
> that be considered equal to ["b.com"]?
> 
> It probably doesn't matter much in places like CommitPendingFramePolicy, but I
> do wonder if there are reasons to think about the whitelist as a vector rather
> than a set.

I think that the reason may have just been that vectors round-trip better over
IPC and between WTF/WebKit/Content. I don't think it matters here (yet) as this
is only used to tell when the policy might have changed, and there isn't much
cost if we get that wrong and trigger an update for an essentially unchanged
policy.

I'll add a TODO and file a crbug to make something more robust -- either check
here for equivalence, or what's probably more efficient, normalize when we parse
the policy in the first place -- I'd hate to have to make this an O(n^2)
comparison when we could just maintain the whitelists in a sorted state.

https://codereview.chromium.org/2797813002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2797813002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:326: pending_container_policy_ =
container_policy;
On 2017/04/11 01:17:49, alexmos wrote:
> This is expected to only done for subframe nodes, right?  Can we add a DCHECK
> and update the comment in the header about that?

That's correct; there is no situation where the container policy should be
modified on the main frame. Done.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
On 2017/04/11 01:17:49, alexmos wrote:
> Checking data: explicitly might not actually be necessary, as
> SecurityOrigin::Create should handle it when checking
> ShouldTreatAsUniqueOrigin().  Please double-check - I haven't followed all the
> latest plumbing.  It's probably best to leave this decision to
> SecurityOrigin::Create anyway, in case other schemes with unique origins are
> added later.

You're right there, thanks. SecurityOrigin::Create will return a unique origin
for data: (and javascript:, and about:, and chrome-native:) schemes.

> I also know file URLs can lead to effectively unique origins (depending on a
> setting which I think is on by default - see last case in
> SecurityOrigin::IsSameSchemeHostPort).  Does that matter here?

Frames at file URLs shouldn't inherit their origin from their owner; they have
their own. If that origin ends up being used in the container policy (by <iframe
allow="featurename"> in the parent), then the parent is choosing to grant access
to the feature in that frame, whether it's treated as same- or cross- origin.

> A unit test for all these corner cases would be nice.

Definitely. I'll add some.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
On 2017/04/11 01:17:49, alexmos wrote:
> nit: use IsSandboxed(kSandboxOrigin)

I thought that was only defined on SecurityContext? I looked, but it didn't seem
useful here. (We're not operating on the actual document in the frame, just on
the way the frame is defined by the owner)

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:174: if
(url.ProtocolIsJavaScript() || url.IsAboutBlankURL())
On 2017/04/11 01:17:49, alexmos wrote:
> Do you also need to handle the case where |url| is empty (i.e., no "src"
> specified)?  
> 
> I wonder if we should call ShouldInheritSecurityOriginFromOwner instead to
> maintain a central place for this decision.

That's a good idea -- it's not currently exported from DocumentLoader.cpp,
though, so maybe I should move it somewhere; it could be a static method in
Document.

> Also, what about srcdoc iframes?

We haven't specified what happens in that case -- I think that it should inherit
from the owner, unless it's also sandboxed, in which case it has a unique origin
that can't currently be referenced by FP anyway. Srcdoc frames should be handled
by ShouldInheritSecurityOriginFromOwner; I'll update to use that.

[commit-bot: I haz the power, 2017-04-11 17:42:16]

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-04-11 19:39:04]

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

[commit-bot: I haz the power, 2017-04-11 19:39:05]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-04-13 02:09:16]

Thanks, this looks good with a couple of nits below.  Were you planning to add a
browser test to cover FrameReplicationState::container_policy, and that the new
pending/effective distinction is working correctly?  I'm thinking of something
similar to DynamicSandboxFlags* tests, and/or extending/repurposing your
existing site_per_process_browsertests for the old replication of "allow"
attributes.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
On 2017/04/11 17:41:43, iclelland wrote:
> On 2017/04/11 01:17:49, alexmos wrote:
> > nit: use IsSandboxed(kSandboxOrigin)
> 
> I thought that was only defined on SecurityContext? I looked, but it didn't
seem
> useful here. (We're not operating on the actual document in the frame, just on
> the way the frame is defined by the owner)

Ah, right, this is for a frame owner, not a document.  This is fine.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
On 2017/04/11 17:41:43, iclelland wrote:
> On 2017/04/11 01:17:49, alexmos wrote:
> > Checking data: explicitly might not actually be necessary, as
> > SecurityOrigin::Create should handle it when checking
> > ShouldTreatAsUniqueOrigin().  Please double-check - I haven't followed all
the
> > latest plumbing.  It's probably best to leave this decision to
> > SecurityOrigin::Create anyway, in case other schemes with unique origins are
> > added later.
> 
> You're right there, thanks. SecurityOrigin::Create will return a unique origin
> for data: (and javascript:, and about:, and chrome-native:) schemes.

The ProtocolIsData() check still seems to be there, even though you removed it
from the comment.

> > I also know file URLs can lead to effectively unique origins (depending on a
> > setting which I think is on by default - see last case in
> > SecurityOrigin::IsSameSchemeHostPort).  Does that matter here?
> 
> Frames at file URLs shouldn't inherit their origin from their owner; they have
> their own. If that origin ends up being used in the container policy (by
<iframe
> allow="featurename"> in the parent), then the parent is choosing to grant
access
> to the feature in that frame, whether it's treated as same- or cross- origin.

Acknowledged.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:174: if
(url.ProtocolIsJavaScript() || url.IsAboutBlankURL())
On 2017/04/11 17:41:43, iclelland wrote:
> On 2017/04/11 01:17:49, alexmos wrote:
> > Do you also need to handle the case where |url| is empty (i.e., no "src"
> > specified)?  
> > 
> > I wonder if we should call ShouldInheritSecurityOriginFromOwner instead to
> > maintain a central place for this decision.
> 
> That's a good idea -- it's not currently exported from DocumentLoader.cpp,
> though, so maybe I should move it somewhere; it could be a static method in
> Document.

Yes, I assumed that we'd need to export it more widely.  This looks ok to me;
I'll defer to a Blink owner on whether there's a better place to expose it.

> > Also, what about srcdoc iframes?
> 
> We haven't specified what happens in that case -- I think that it should
inherit
> from the owner, unless it's also sandboxed, in which case it has a unique
origin
> that can't currently be referenced by FP anyway. Srcdoc frames should be
handled
> by ShouldInheritSecurityOriginFromOwner; I'll update to use that.

Acknowledged.

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp (right):

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:67:
effective_origin->IsSameSchemeHostPort(document->GetSecurityOrigin()));
nit: I'd also check that effective_origin->IsUnique() is true (and similarly in
the sandbox srcdoc test)

[iclelland, 2017-04-13 19:03:24]

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

[commit-bot: I haz the power, 2017-04-13 19:04:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-04-13 19:05:32]

Yes, I've added a test to site_per_processs_browsertests, and I'm in the process
of adding a few more.

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:170: if
(url.ProtocolIsData() || (GetSandboxFlags() & kSandboxOrigin))
On 2017/04/13 02:09:15, alexmos wrote:
> On 2017/04/11 17:41:43, iclelland wrote:
> > On 2017/04/11 01:17:49, alexmos wrote:
> > > Checking data: explicitly might not actually be necessary, as
> > > SecurityOrigin::Create should handle it when checking
> > > ShouldTreatAsUniqueOrigin().  Please double-check - I haven't followed all
> the
> > > latest plumbing.  It's probably best to leave this decision to
> > > SecurityOrigin::Create anyway, in case other schemes with unique origins
are
> > > added later.
> > 
> > You're right there, thanks. SecurityOrigin::Create will return a unique
origin
> > for data: (and javascript:, and about:, and chrome-native:) schemes.
> 
> The ProtocolIsData() check still seems to be there, even though you removed it
> from the comment.

Thanks for catching that. Done.

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp (right):

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:67:
effective_origin->IsSameSchemeHostPort(document->GetSecurityOrigin()));
On 2017/04/13 02:09:15, alexmos wrote:
> nit: I'd also check that effective_origin->IsUnique() is true (and similarly
in
> the sandbox srcdoc test)

Done. (And I'd added that to the ContainerPolicy tests below in the latest patch
as well)

[iclelland, 2017-04-13 20:16:46]

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

[commit-bot: I haz the power, 2017-04-13 20:17:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-04-13 20:54:05]

Thanks for writing all this Ian!
lgtm + nit

https://codereview.chromium.org/2797813002/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2797813002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1003:
FeaturePolicyHeaderToWeb(replicated_state.container_policy),
This is not important, but some point in the future, is is worth it to rename
FeaturePolicyHeaderToWeb to FeaturePolicyToWeb?

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2797813002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5130: // If a Document is the
initial "about:blank" document The origin and
nit: s/The/, the

[commit-bot: I haz the power, 2017-04-13 21:04:59]

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

[commit-bot: I haz the power, 2017-04-13 21:05:00]

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

[alexmos, 2017-04-14 00:33:43]

On 2017/04/13 19:05:32, iclelland wrote:
> Yes, I've added a test to site_per_processs_browsertests, and I'm in the
process
> of adding a few more.

Thanks, the new tests look good.  Are you all finished with them?  Let me know
if I should take a look at the latest PS or wait.

[iclelland, 2017-04-14 03:29:21]

On 2017/04/14 00:33:43, alexmos wrote:
> On 2017/04/13 19:05:32, iclelland wrote:
> > Yes, I've added a test to site_per_processs_browsertests, and I'm in the
> process
> > of adding a few more.
> 
> Thanks, the new tests look good.  Are you all finished with them?  Let me know
> if I should take a look at the latest PS or wait.

I think I'm good for now; I'm going to write up a plan to add more comprehensive
tests soon, but PTAL. Thanks!

[alexmos, 2017-04-14 23:42:45]

Thanks for adding the tests!  LGTM with nits, and let's follow up on removing
the other replication for "allow" attributes, and merging sandbox flags and
"allow" into a snapshotted attributes structure where it makes sense.

https://codereview.chromium.org/2797813002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:220: return
pending_container_policy_;
You could also use FRIEND_TEST_ALL_PREFIXES and check pending_container_policy_
from the test directly.

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp (right):

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:142: // Test that
a iframes with relative src urls correctly construct their origin
nit: remove "a" before iframes

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:183:
EXPECT_EQ(container_policy.size(), 0UL);
nit: reverse order
(should be (expected, actual) - also in a bunch of places below.  This matters
from failure output)

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:244: const
WebParsedFeaturePolicy& container_policy1 =
nit: can probably drop the "1" here, also below

[iclelland, 2017-04-15 03:35:36]

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

[commit-bot: I haz the power, 2017-04-15 03:35:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-04-15 03:36:07]

https://codereview.chromium.org/2797813002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2797813002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:220: return
pending_container_policy_;
On 2017/04/14 23:42:45, alexmos wrote:
> You could also use FRIEND_TEST_ALL_PREFIXES and check
pending_container_policy_
> from the test directly.

Now that's a handy macro, thanks!

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp (right):

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:142: // Test that
a iframes with relative src urls correctly construct their origin
On 2017/04/14 23:42:45, alexmos wrote:
> nit: remove "a" before iframes

Done.

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:183:
EXPECT_EQ(container_policy.size(), 0UL);
On 2017/04/14 23:42:45, alexmos wrote:
> nit: reverse order
> (should be (expected, actual) - also in a bunch of places below.  This matters
> from failure output)

Thanks, all fixed now.

https://codereview.chromium.org/2797813002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLIFrameElementTest.cpp:244: const
WebParsedFeaturePolicy& container_policy1 =
On 2017/04/14 23:42:45, alexmos wrote:
> nit: can probably drop the "1" here, also below

Definitely, thanks. Done. (And below)

[iclelland, 2017-04-15 03:37:13]

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

[iclelland, 2017-04-15 03:37:14]

+r pfeldman -- can you PTAL as OWNER in third_party/WebKit?

[commit-bot: I haz the power, 2017-04-15 03:59:01]

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

[commit-bot: I haz the power, 2017-04-15 03:59:02]

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

[iclelland, 2017-04-15 16:06:04]

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

[commit-bot: I haz the power, 2017-04-15 16:06:14]

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-04-15 16:32:38]

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

[commit-bot: I haz the power, 2017-04-15 16:32:39]

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

[iclelland, 2017-04-16 04:11:29]

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

[commit-bot: I haz the power, 2017-04-16 04:11:37]

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-04-16 05:47:35]

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

[commit-bot: I haz the power, 2017-04-16 05:47:36]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-04-17 17:17:48]

Description was changed from

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

to

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

[pfeldman, 2017-04-17 17:17:48]

pfeldman@chromium.org changed reviewers:
+ dcheng@chromium.org, mkwst@chromium.org

[pfeldman, 2017-04-17 17:18:12]

someone from isolation / security should take a look at this.

[iclelland, 2017-04-18 13:24:40]

On 2017/04/17 17:18:12, pfeldman wrote:
> someone from isolation / security should take a look at this.

sgtm; I figured alexmos was the SME on isolation, but happy to have more eyes.
(mkwst, could you PTAL from security perspective?)

[dcheng, 2017-04-18 16:26:37]

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameOwner.h (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameOwner.h:94: return container_policy;
Nit: it's unclear to me why we need a static local for this; we expect this to
be called basically only for tests. I would just return WebParsedFeaturePolicy()
and not bother with the static local.

(Ditto for 89-90, but it's OK to leave that for now, since that's not changed by
this CL)

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:169: KURL url =
GetDocument().CompleteURL(url_);
Nit: move this down to line 173

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.h (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.h:47: const
RefPtr<SecurityOrigin> GetOriginForFeaturePolicy() const override;
Nit: const RefPtr<> doesn't do much here other than suppressing some potential
optimizations, so remove the const.

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:206:
sandbox_flags_ = flags;
How come this doesn't need to refresh container_policy_? Couldn't setting
sandbox flags change the origin used for feature policy?

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:137: RemoteFrameOwner* remoteOwner =
ToRemoteFrameOwner(owner);
Nit: remote_owner

Or maybe just merge this into line 135

[iclelland, 2017-04-18 19:18:18]

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

[commit-bot: I haz the power, 2017-04-18 19:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-04-18 19:19:44]

Thanks, dcheng! (I would have asked you explicitly to take a look, but your
username suggested you're OOO for a couple of weeks)

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameOwner.h (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameOwner.h:94: return container_policy;
On 2017/04/18 16:26:37, dcheng (OOO through May 2) wrote:
> Nit: it's unclear to me why we need a static local for this; we expect this to
> be called basically only for tests. I would just return
WebParsedFeaturePolicy()
> and not bother with the static local.
> 
> (Ditto for 89-90, but it's OK to leave that for now, since that's not changed
by
> this CL)

It needs to return a reference (the other non-test implementations return a
reference to the real policy), and so can't just use a temporary. I could add an
actual member var, with basically the same effect, but I thought that following
the existing pattern made sense.

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:169: KURL url =
GetDocument().CompleteURL(url_);
On 2017/04/18 16:26:37, dcheng (OOO through May 2) wrote:
> Nit: move this down to line 173

Done.

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.h (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.h:47: const
RefPtr<SecurityOrigin> GetOriginForFeaturePolicy() const override;
On 2017/04/18 16:26:37, dcheng (OOO through May 2) wrote:
> Nit: const RefPtr<> doesn't do much here other than suppressing some potential
> optimizations, so remove the const.

Done, thanks.

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:206:
sandbox_flags_ = flags;
On 2017/04/18 16:26:37, dcheng (OOO through May 2) wrote:
> How come this doesn't need to refresh container_policy_? Couldn't setting
> sandbox flags change the origin used for feature policy?

That's a really good point, it should update that if the allow-same-origin flag
changes. I've added a test in site_per_process_browsertests to ensure that the
policy origin is changed properly.

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/2797813002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:137: RemoteFrameOwner* remoteOwner =
ToRemoteFrameOwner(owner);
On 2017/04/18 16:26:37, dcheng (OOO through May 2) wrote:
> Nit: remote_owner
> 
> Or maybe just merge this into line 135

Whoops. (Didn't use the rebase-helper tool on this CL, so that slipped through.)

Rewrote to match the same block in SetFrameOwnerProperties below.

[iclelland, 2017-04-18 19:25:19]

https://codereview.chromium.org/2797813002/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2797813002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9547: // Test dynamic updates to
iframe sandbox attribute correctly set the replicated
self nit: "Test that dynamic updates..."

https://codereview.chromium.org/2797813002/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9558: // Validate that the
effectiive container policy contains a single non-unique
self nit: effective

[dcheng, 2017-04-18 19:46:53]

Blink LGTM with two comments addressed.  I talked with iclelland offline about
the semantics of GetOriginForFeaturePolicy(). 

1) I found the comment on GetOriginForFeaturePolicy() confusing. To me, it seems
like this is the default origin we want to bind the feature policy to if there
is explicit origin specified with the policy. Maybe it'd be possible to reword
the comment a little bit.

2) If possible, let's make GetOriginForFeaturePolicy() protected throughout the
entire chain. It's protected on FrameOwner, but public on HTMLFrameElementBase.
HTMLFrameElementBase can just friend a test helper for test access.

(Sorry for the confusion; I set my OOO after this review landed in my queue to
make sure I don't have to finish too many reviews the day I'm leaving. I leave
it up to you if you feel my LGTM is sufficient)

[commit-bot: I haz the power, 2017-04-18 20:56:19]

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

[commit-bot: I haz the power, 2017-04-18 20:56:21]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-04-18 21:00:23]

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

[commit-bot: I haz the power, 2017-04-18 21:00:56]

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-04-18 23:19:00]

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

[commit-bot: I haz the power, 2017-04-18 23:19:01]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-04-19 02:32:44]

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

[iclelland, 2017-04-19 02:32:45]

On 2017/04/18 19:46:53, dcheng (OOO through May 2) wrote:
> Blink LGTM with two comments addressed.  I talked with iclelland offline about
> the semantics of GetOriginForFeaturePolicy(). 
> 
> 1) I found the comment on GetOriginForFeaturePolicy() confusing. To me, it
seems
> like this is the default origin we want to bind the feature policy to if there
> is explicit origin specified with the policy. Maybe it'd be possible to reword
> the comment a little bit.
> 
> 2) If possible, let's make GetOriginForFeaturePolicy() protected throughout
the
> entire chain. It's protected on FrameOwner, but public on
HTMLFrameElementBase.
> HTMLFrameElementBase can just friend a test helper for test access.
> 
> (Sorry for the confusion; I set my OOO after this review landed in my queue to
> make sure I don't have to finish too many reviews the day I'm leaving. I leave
> it up to you if you feel my LGTM is sufficient)

Both done; thanks for the the review.

+r thestig -- can you approve the change in components/printing ? Thanks!

[Lei Zhang, 2017-04-19 05:22:26]

lgtm

[Mike West, 2017-04-19 08:45:30]

LGTM, though it looks like I'm too late to the party. :(

[iclelland, 2017-04-19 12:36:32]

On 2017/04/19 08:45:30, Mike West wrote:
> LGTM, though it looks like I'm too late to the party. :(

I still appreciate it; thanks for coming! :)

[iclelland, 2017-04-19 12:36:41]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-04-19 12:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, lunalu@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2797813002/#ps260001
(title: "Addressing review comments")

[commit-bot: I haz the power, 2017-04-19 12:37:01]

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-04-19 12:43:10]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1492605401175130,
"parent_rev": "c24f121f6d56c1447d4a28d2954d8b7b8c4a30cb", "commit_rev":
"92f8c0b3ff53422eb761b53793bac0b424ec45df"}

[commit-bot: I haz the power, 2017-04-19 12:43:21]

Description was changed from

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

to

==========
Replicate feature policy container policies.

This CL adds a persistent parsed container policy to
HTMLFrameOwnerElement, and replicates it alongside of sandbox attributes
to remote frames. In most cases, the sandbox and FP attributes should be
processed at the same time, so the IPC and methods for them have been
combined. The combination of sandbox and FP is referred to as the "frame
policies".

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

Review-Url: https://codereview.chromium.org/2797813002
Cr-Commit-Position: refs/heads/master@{#465563}
Committed:
https://chromium.googlesource.com/chromium/src/+/92f8c0b3ff53422eb761b53793ba...
==========

[commit-bot: I haz the power, 2017-04-19 12:43:23]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/92f8c0b3ff53422eb761b53793ba...