Replicate feature policy headers to remote frames

Replicate feature policy headers to remote frames

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

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

[iclelland, 2016-11-06 11:59:45]

Description was changed from

==========
[WIP] NOT FOR COMMIT YET - Start replicating feature policy headers to remote
frames

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

to

==========
[WIP] NOT FOR COMMIT YET - Start replicating feature policy headers to remote
frames

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

[raymes, 2016-11-07 02:16:00]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2016-11-07 02:16:01]

https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:88: std::vector<std::string>
feature_policy_headers;
Does this really need to be a vector of string, as opposed to just string? Can
we just replicate the frame's own header? Or am I missing something?

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:229:
web_frame_->resetReplicatedContentSecurityPolicy();
This calls the wrong thing by the looks of it!

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:231:
web_frame_->addReplicatedFeaturePolicyHeader(
Similarly here, I think we could just get rid of the array and do
setReplicatedFeaturePolicyHeader rather than addReplicatedFeaturePolicyHeader.

The same in all the other places where we have add (as opposed to set).

If we're able to do that then reset probably is unnecessary

[iclelland, 2016-11-07 03:37:03]

https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:88: std::vector<std::string>
feature_policy_headers;
On 2016/11/07 02:16:01, raymes wrote:
> Does this really need to be a vector of string, as opposed to just string? Can
> we just replicate the frame's own header? Or am I missing something?

I was thinking of the case of multiple HTTP headers, which need to be coalesced
into a single policy later, but that's handled already by ResourceResponse, so
there's no need to do that here.

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:229:
web_frame_->resetReplicatedContentSecurityPolicy();
On 2016/11/07 02:16:01, raymes wrote:
> This calls the wrong thing by the looks of it!

Quite likely :) This is all very very WIP; not nearly ready for review. I wanted
to make sure you were aware of the space I'd been working in, so we didn't
duplicate efforts.

https://codereview.chromium.org/2483703002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_proxy.cc:231:
web_frame_->addReplicatedFeaturePolicyHeader(
On 2016/11/07 02:16:01, raymes wrote:
> Similarly here, I think we could just get rid of the array and do
> setReplicatedFeaturePolicyHeader rather than addReplicatedFeaturePolicyHeader.
> 
> The same in all the other places where we have add (as opposed to set).
> 
> If we're able to do that then reset probably is unnecessary

Agreed. Since the headers are already fully received and combined by this point,
there's no need for the vector.

[iclelland, 2016-11-07 21:32:50]

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

[commit-bot: I haz the power, 2016-11-07 21:33:31]

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

[iclelland, 2016-11-07 21:40:31]

Description was changed from

==========
[WIP] NOT FOR COMMIT YET - Start replicating feature policy headers to remote
frames

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

to

==========
Replicate feature policy headers to remote frames

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

[iclelland, 2016-11-07 21:40:31]

iclelland@chromium.org changed reviewers:
+ creis@chromium.org, mkwst@chromium.org

[iclelland, 2016-11-07 21:42:25]

iclelland@chromium.org changed reviewers:
- creis@chromium.org

[iclelland, 2016-11-07 21:42:25]

+r mkwst -- can you PTAL?
(Especially as OWNER for frame_messages and WebKit, but also for Feature-policy
generally)

[raymes, 2016-11-07 23:10:25]

lg - this is cool :)

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3128: return;
Hmm - we will want this to be replicated to the browser even if site isolation
is disabled. Do you know if that will still happen with this check in place?
Should we remove it? alexmos@ might know.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:454: Vector<String>
messages;
I think we can ignore messages here because appropriate errors will be displayed
when the header is parsed in its own frame. Is that true? Should we add a
comment to that effect?

[commit-bot: I haz the power, 2016-11-07 23:20:12]

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

[commit-bot: I haz the power, 2016-11-07 23:20:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2016-11-08 03:13:41]

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3128: return;
On 2016/11/07 23:10:25, raymes wrote:
> Hmm - we will want this to be replicated to the browser even if site isolation
> is disabled. Do you know if that will still happen with this check in place?
> Should we remove it? alexmos@ might know.

That's a good point - once we start using it for browser-side features /
permissions, then we will want to send this to the browser. I can remove it
here, or we can remove it later, once it's used for something other than site
isolation.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:454: Vector<String>
messages;
On 2016/11/07 23:10:25, raymes wrote:
> I think we can ignore messages here because appropriate errors will be
displayed
> when the header is parsed in its own frame. Is that true? Should we add a
> comment to that effect?

Agreed; that's why we don't do anything with the messages var. It's passed by
reference to setHeaderPolicy, though, so I needed to pass *something*. I can
rename it to _, unless it makes more sense to provide a 1-arg version of
setHeaderPolicy just for this case.

[Mike West, 2016-11-08 12:12:22]

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

[Mike West, 2016-11-08 12:12:23]

In general, this looks reasonable. I have some suggestions in comments, but I'll
defer to alexmos@ for the mechanics (as that's exactly who I ask to review my
feeble attempts to replicate things :) ).

Happy to stamp things once he's happy with the general shape.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:603: }
An approach that you might wish to consider would be moving this up to the
browser process entirely. That is, rather than parsing the header in Blink and
pushing a policy up through the browser, you could parse the policy in the
browser, store it on the frame tree, and forcibly apply parent's policy to child
frames when the browser created them for each navigation.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:453: parentFeaturePolicy,
childSecurityContext->getSecurityOrigin()));
It surprised me that you did all this work both here and in FrameLoader. It
seems like you should be able to replicate over a complete policy (including all
the parent policies). That is, the LocalFrame should populate itself in whatever
way it needs to, and then replicate that complete state.

(Note that CSP might not be the best example in it's current implementation:
it's fairly verbose, sending one IPC per header (in
`ContentSecurityPolicy::reportAccumulatedHeaders`)).

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:454: Vector<String>
messages;
On 2016/11/08 at 03:13:41, iclelland wrote:
> On 2016/11/07 23:10:25, raymes wrote:
> > I think we can ignore messages here because appropriate errors will be
displayed
> > when the header is parsed in its own frame. Is that true? Should we add a
> > comment to that effect?
> 
> Agreed; that's why we don't do anything with the messages var. It's passed by
reference to setHeaderPolicy, though, so I needed to pass *something*. I can
rename it to _, unless it makes more sense to provide a 1-arg version of
setHeaderPolicy just for this case.

1. ContentSecurityPolicy makes itself responsible for sending messages to the
console when it's bound to an execution context. Since it never binds to an
execution context in a remote frame, no messages are duplicated. Perhaps that
kind of model could be useful here? Look at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp...
if you're curious.

2. Aesthetically, I think it would be nicer to have a messageless version of
`setHeaderPolicy`, but it's fine either way. If you keep it here, perhaps you
could rename the variable to something like |unused_messages|?

[iclelland, 2016-11-08 14:25:45]

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:603: }
On 2016/11/08 12:12:23, Mike West wrote:
> An approach that you might wish to consider would be moving this up to the
> browser process entirely. That is, rather than parsing the header in Blink and
> pushing a policy up through the browser, you could parse the policy in the
> browser, store it on the frame tree, and forcibly apply parent's policy to
child
> frames when the browser created them for each navigation.

Since a frame can also set policy for itself -- it can disable synchronous
script loading, or remove v8 features, I think we'll need to have the policy set
before we can continue with parsing the document; moving it to the browser
process would mean that we have to block on that process switch (twice), which I
was hoping to avoid.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:453: parentFeaturePolicy,
childSecurityContext->getSecurityOrigin()));
On 2016/11/08 12:12:23, Mike West wrote:
> It surprised me that you did all this work both here and in FrameLoader. It
> seems like you should be able to replicate over a complete policy (including
all
> the parent policies). That is, the LocalFrame should populate itself in
whatever
> way it needs to, and then replicate that complete state.
> 
> (Note that CSP might not be the best example in it's current implementation:
> it's fairly verbose, sending one IPC per header (in
> `ContentSecurityPolicy::reportAccumulatedHeaders`)).

I could certainly factor this out into a method on SecurityContext that takes a
parent frame (or the parent SecurityContext) and use it in both places.

It seemed natural to do it this way, using the JSON header itself, rather than
re-serializing the complete policy. I'll give the other way some serious
thought, though, to make sure it can handle all of the cases it needs to.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:454: Vector<String>
messages;
On 2016/11/08 12:12:23, Mike West wrote:
> On 2016/11/08 at 03:13:41, iclelland wrote:
> > On 2016/11/07 23:10:25, raymes wrote:
> > > I think we can ignore messages here because appropriate errors will be
> displayed
> > > when the header is parsed in its own frame. Is that true? Should we add a
> > > comment to that effect?
> > 
> > Agreed; that's why we don't do anything with the messages var. It's passed
by
> reference to setHeaderPolicy, though, so I needed to pass *something*. I can
> rename it to _, unless it makes more sense to provide a 1-arg version of
> setHeaderPolicy just for this case.
> 
> 1. ContentSecurityPolicy makes itself responsible for sending messages to the
> console when it's bound to an execution context. Since it never binds to an
> execution context in a remote frame, no messages are duplicated. Perhaps that
> kind of model could be useful here? Look at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp...
> if you're curious.

I like that approach, thanks!

> 
> 2. Aesthetically, I think it would be nicer to have a messageless version of
> `setHeaderPolicy`, but it's fine either way. If you keep it here, perhaps you
> could rename the variable to something like |unused_messages|?

I'll make a unary method, and call a private method with nullptr from it (if
it's still necessary, given (1))

[alexmos, 2016-11-09 01:16:55]

Looks reasonable to me as well.  Just a few comments, and it seems you need to
look into the test failures?

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3128: return;
On 2016/11/08 03:13:41, iclelland wrote:
> On 2016/11/07 23:10:25, raymes wrote:
> > Hmm - we will want this to be replicated to the browser even if site
isolation
> > is disabled. Do you know if that will still happen with this check in place?
> > Should we remove it? alexmos@ might know.
> 
> That's a good point - once we start using it for browser-side features /
> permissions, then we will want to send this to the browser. I can remove it
> here, or we can remove it later, once it's used for something other than site
> isolation.

I'd say just remove this now.  

AreCrossProcessFramesPossible would be true if any kind of OOPIF mode is active,
and it's already true for lots of canary/dev/beta users in our field trial for
--isolate-extensions.  The value of this on ToT had been true for a while; we
had to flip it to false recently but anticipate turning it back on after branch
point.

I think it's ok to just always send the IPC, as down the road, if your
browser-side code starts relying on the header and you forget to remove this,
you might have a nasty surprise if ToT value of this is |true|, but our field
trials make it |false| for some users.

On that note, if you plan to use this on the browser side, it'd be good to have
a browsertest to verify the feature_policy_header on FrameTreeNode (see the CSP
and other replication tests in SitePerProcessBrowserTest).  Probably fine if you
want to add it later though, given that you have end-to-end coverage with your
layout test.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/cross-origin-feature-disabled.php
(right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/cross-origin-feature-disabled.php:9:
testRunner.dumpChildFramesAsText();
Are these two calls needed?  Doesn't seem like you have any expected.txt files.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/cross-origin-feature-disabled.php:17:
<iframe
src="http://127.0.0.1:8000/feature-policy/resources/cross-origin-iframe-for-feature-enabled.html"></iframe>
Did you intend for this second one to be a same-origin or cross-origin frame? 
(i.e., should it be using localhost, like the first one?)

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/cross-origin-iframe-for-feature-disabled.html
(right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/cross-origin-iframe-for-feature-disabled.html:10:
assert_false("write" in document);
Looks like the test is failing without --site-per-process, and crashing with it.

The crash seems to be in BlinkTestController::OnLayoutDumpResponse - if you need
any help with that, you can ping lukasza@, who worked on OOPIF support for
layout test harness.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:862: if
(m_webFrame->client()) {
nit: { not needed

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void
WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader(
Just to double-check: will this work in the case where we have
A-embed-B-embed-C, A has a feature policy header, B does *not* have the header,
and we're initializing the frame tree in process C?  

We'll first create the root frame's proxy, which will call this and create the
right feature policy.  Next, we'll create the middle child's proxy, and still
call this, but with the default empty header value (since we never called
DidSetFeaturePolicyHeader).  Will this do the right thing with the empty header
value, i.e., inherit the parent's policy?  (Lastly, the bottom child's
LocalFrame will initialize its policy using the middle child's policy.)

It might be worth adding another test to cover this case (remote-remote-local).

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:453: parentFeaturePolicy,
childSecurityContext->getSecurityOrigin()));
On 2016/11/08 14:25:45, iclelland wrote:
> On 2016/11/08 12:12:23, Mike West wrote:
> > It surprised me that you did all this work both here and in FrameLoader. It
> > seems like you should be able to replicate over a complete policy (including
> all
> > the parent policies). That is, the LocalFrame should populate itself in
> whatever
> > way it needs to, and then replicate that complete state.
> > 
> > (Note that CSP might not be the best example in it's current implementation:
> > it's fairly verbose, sending one IPC per header (in
> > `ContentSecurityPolicy::reportAccumulatedHeaders`)).
> 
> I could certainly factor this out into a method on SecurityContext that takes
a
> parent frame (or the parent SecurityContext) and use it in both places.
> 
> It seemed natural to do it this way, using the JSON header itself, rather than
> re-serializing the complete policy. I'll give the other way some serious
> thought, though, to make sure it can handle all of the cases it needs to.

I'm slightly in favor of the current approach, actually (factor this out into a
helper and use both here and in FrameLoader).  Re-serializing the complete
policy would avoid the duplicate computation here, but it might also mean
replicating more data overall.  E.g., if you have a root frame with the FP
header and 50 different subframes with no header, with this approach you'd only
replicate the root frame's header, but with the other approach each of the
subframes might have a non-null final policy that would also need to be sent
around.

One other thing to think about is whether the enable/disable iframe attributes
would be easier to implement with the current approach or with replicating the
complete policy. (I'm guessing you will work on those in followup CLs?)

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:232: virtual void
didSetFeaturePolicyHeader(const WebString& headerValue) {}
nit: needs a comment.

[alexmos, 2016-11-09 01:27:49]

[also, +site-isolation-reviews]

[iclelland, 2016-11-09 18:02:28]

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

[commit-bot: I haz the power, 2016-11-09 18:03:03]

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

[iclelland, 2016-11-09 18:07:59]

Thanks, Alex!

> Looks reasonable to me as well.  Just a few comments, and it seems you need to
> look into the test failures?

Yes, I added the single layout test to see if I could get the tests to run at
all. Once I get the mechanics of that to work, I'm planning on adding quite a
few more. (And thanks for the pointers there)

Other comments are addressed inline.

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3128: return;
On 2016/11/09 01:16:55, alexmos wrote:
> On 2016/11/08 03:13:41, iclelland wrote:
> > On 2016/11/07 23:10:25, raymes wrote:
> > > Hmm - we will want this to be replicated to the browser even if site
> isolation
> > > is disabled. Do you know if that will still happen with this check in
place?
> > > Should we remove it? alexmos@ might know.
> > 
> > That's a good point - once we start using it for browser-side features /
> > permissions, then we will want to send this to the browser. I can remove it
> > here, or we can remove it later, once it's used for something other than
site
> > isolation.
> 
> I'd say just remove this now.  

Done, make sense. Thanks.

> 
> AreCrossProcessFramesPossible would be true if any kind of OOPIF mode is
active,
> and it's already true for lots of canary/dev/beta users in our field trial for
> --isolate-extensions.  The value of this on ToT had been true for a while; we
> had to flip it to false recently but anticipate turning it back on after
branch
> point.
> 
> I think it's ok to just always send the IPC, as down the road, if your
> browser-side code starts relying on the header and you forget to remove this,
> you might have a nasty surprise if ToT value of this is |true|, but our field
> trials make it |false| for some users.
> 
> On that note, if you plan to use this on the browser side, it'd be good to
have
> a browsertest to verify the feature_policy_header on FrameTreeNode (see the
CSP
> and other replication tests in SitePerProcessBrowserTest).  Probably fine if
you
> want to add it later though, given that you have end-to-end coverage with your
> layout test.

Added https://bugs.chromium.org/p/chromium/issues/detail?id=663731 so it doesn't
get forgotten.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/cross-origin-feature-disabled.php
(right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/cross-origin-feature-disabled.php:17:
<iframe
src="http://127.0.0.1:8000/feature-policy/resources/cross-origin-iframe-for-feature-enabled.html"></iframe>
On 2016/11/09 01:16:55, alexmos wrote:
> Did you intend for this second one to be a same-origin or cross-origin frame? 
> (i.e., should it be using localhost, like the first one?)

That is intentional, to ensure that the feature is still enabled in a
same-origin iframe. (The loaded html file filename should probably be changed)

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/cross-origin-iframe-for-feature-disabled.html
(right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/cross-origin-iframe-for-feature-disabled.html:10:
assert_false("write" in document);
On 2016/11/09 01:16:55, alexmos wrote:
> Looks like the test is failing without --site-per-process, and crashing with
it.
> 
> The crash seems to be in BlinkTestController::OnLayoutDumpResponse - if you
need
> any help with that, you can ping lukasza@, who worked on OOPIF support for
> layout test harness.

Thanks for the pointer -- I'll definitely do that if I can't get the tests
working soon.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:862: if
(m_webFrame->client()) {
On 2016/11/09 01:16:55, alexmos wrote:
> nit: { not needed

Done.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void
WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader(
On 2016/11/09 01:16:55, alexmos wrote:
> Just to double-check: will this work in the case where we have
> A-embed-B-embed-C, A has a feature policy header, B does *not* have the
header,
> and we're initializing the frame tree in process C?  
> 
> We'll first create the root frame's proxy, which will call this and create the
> right feature policy.  Next, we'll create the middle child's proxy, and still
> call this, but with the default empty header value (since we never called
> DidSetFeaturePolicyHeader).  Will this do the right thing with the empty
header
> value, i.e., inherit the parent's policy?  (Lastly, the bottom child's
> LocalFrame will initialize its policy using the middle child's policy.)
> 
> It might be worth adding another test to cover this case
(remote-remote-local).

Definitely, I'm planning on adding a number of other tests, to cover all of the
cases. That one should work -- a frame with no header should still call
DidSetFeaturePolicyHeader -- the FrameLoader always calls
client()->didSetFeaturePolicyHeader, even if the header is empty. (Maybe it
should be renamed, or dropped in that case)

If I fix that (so that the header is not replicated over IPC if not sent) then
the FrameReplicationState could still be initialized to a correct empty string,
and this method would still be called by RenderFrameProxy::SetReplicatedState,
and would create the correct policy, inheriting everything from the parent.

I'll add an explicit test for that case as well. (And its various permutations)

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:454: Vector<String>
messages;
On 2016/11/08 14:25:45, iclelland wrote:
> On 2016/11/08 12:12:23, Mike West wrote:
> > On 2016/11/08 at 03:13:41, iclelland wrote:
> > > On 2016/11/07 23:10:25, raymes wrote:
> > > > I think we can ignore messages here because appropriate errors will be
> > displayed
> > > > when the header is parsed in its own frame. Is that true? Should we add
a
> > > > comment to that effect?
> > > 
> > > Agreed; that's why we don't do anything with the messages var. It's passed
> by
> > reference to setHeaderPolicy, though, so I needed to pass *something*. I can
> > rename it to _, unless it makes more sense to provide a 1-arg version of
> > setHeaderPolicy just for this case.
> > 
> > 1. ContentSecurityPolicy makes itself responsible for sending messages to
the
> > console when it's bound to an execution context. Since it never binds to an
> > execution context in a remote frame, no messages are duplicated. Perhaps
that
> > kind of model could be useful here? Look at
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp...
> > if you're curious.
> 
> I like that approach, thanks!
> 
> > 
> > 2. Aesthetically, I think it would be nicer to have a messageless version of
> > `setHeaderPolicy`, but it's fine either way. If you keep it here, perhaps
you
> > could rename the variable to something like |unused_messages|?
> 
> I'll make a unary method, and call a private method with nullptr from it (if
> it's still necessary, given (1))

I've switched it to taking an optional Vector<String>*, and not logging if the
passed-in pointer is null. I don't think there's any benefit in passing a
reference at this point. The bind-to-execution-context pattern is interesting,
but I'm not sure its worth the additional complexity here. It looks like it is
better at solving the potentially-delayed-side-effect issue than just making the
logging optional.

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:232: virtual void
didSetFeaturePolicyHeader(const WebString& headerValue) {}
On 2016/11/09 01:16:55, alexmos wrote:
> nit: needs a comment.

Done.

[iclelland, 2016-11-09 18:13:52]

On 2016/11/09 18:07:59, iclelland wrote:
> Thanks, Alex!
> 
> > Looks reasonable to me as well.  Just a few comments, and it seems you need
to
> > look into the test failures?
> 
> Yes, I added the single layout test to see if I could get the tests to run at
> all. Once I get the mechanics of that to work, I'm planning on adding quite a
> few more. (And thanks for the pointers there)
> 

FYI, my testing plan is to:
1) Add a feature-policy-controlled feature to the internals interface
2) Add layout tests that test that feature, rather than document.write
3) Move the IDL-switched features behind a separate "experimental FPV2" flag
4) Land this patch, and see that the tests pass on the site-isolation bot as
well.

[commit-bot: I haz the power, 2016-11-09 19:51:51]

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

[commit-bot: I haz the power, 2016-11-09 19:51:52]

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, 2016-11-10 17:50:28]

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void
WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader(
On 2016/11/09 18:07:59, iclelland wrote:
> On 2016/11/09 01:16:55, alexmos wrote:
> > Just to double-check: will this work in the case where we have
> > A-embed-B-embed-C, A has a feature policy header, B does *not* have the
> header,
> > and we're initializing the frame tree in process C?  
> > 
> > We'll first create the root frame's proxy, which will call this and create
the
> > right feature policy.  Next, we'll create the middle child's proxy, and
still
> > call this, but with the default empty header value (since we never called
> > DidSetFeaturePolicyHeader).  Will this do the right thing with the empty
> header
> > value, i.e., inherit the parent's policy?  (Lastly, the bottom child's
> > LocalFrame will initialize its policy using the middle child's policy.)
> > 
> > It might be worth adding another test to cover this case
> (remote-remote-local).
> 
> Definitely, I'm planning on adding a number of other tests, to cover all of
the
> cases. That one should work -- a frame with no header should still call
> DidSetFeaturePolicyHeader -- the FrameLoader always calls
> client()->didSetFeaturePolicyHeader, even if the header is empty. (Maybe it
> should be renamed, or dropped in that case)
> 
> If I fix that (so that the header is not replicated over IPC if not sent) then
> the FrameReplicationState could still be initialized to a correct empty
string,
> and this method would still be called by RenderFrameProxy::SetReplicatedState,
> and would create the correct policy, inheriting everything from the parent.
> 
> I'll add an explicit test for that case as well. (And its various
permutations)

That sounds great, I missed that DidSetFeaturePolicyHeader was done even for the
empty header.

I see that you've changed it now (which is great, as it benefits the common
case). One issue for that though is that we need to deal properly with the same
frame navigating from a document with a non-empty header to a document with an
empty header.  In that case, we need to reset the header's value in
FrameTreeNode's replication state.  This should've worked when sending the empty
header IPC, but I'm not so sure it does now that we drop it.  We could probably
clear the replication state's feature_policy_header in
NavigatorImpl::DidNavigate, where we update the other replicated properties,
like origin or insecure_request_policy.  Probably another thing worth having a
test for eventually.

https://codereview.chromium.org/2483703002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2483703002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:33: //
errors in the input will cause an error message fo be appended to it.
s/fo/to/

[iclelland, 2016-11-16 19:49:04]

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

[commit-bot: I haz the power, 2016-11-16 19:49:52]

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

[iclelland, 2016-11-16 20:21:32]

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void
WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader(
On 2016/11/10 17:50:28, alexmos wrote:
> On 2016/11/09 18:07:59, iclelland wrote:
> > On 2016/11/09 01:16:55, alexmos wrote:
> > > Just to double-check: will this work in the case where we have
> > > A-embed-B-embed-C, A has a feature policy header, B does *not* have the
> > header,
> > > and we're initializing the frame tree in process C?  
> > > 
> > > We'll first create the root frame's proxy, which will call this and create
> the
> > > right feature policy.  Next, we'll create the middle child's proxy, and
> still
> > > call this, but with the default empty header value (since we never called
> > > DidSetFeaturePolicyHeader).  Will this do the right thing with the empty
> > header
> > > value, i.e., inherit the parent's policy?  (Lastly, the bottom child's
> > > LocalFrame will initialize its policy using the middle child's policy.)
> > > 
> > > It might be worth adding another test to cover this case
> > (remote-remote-local).
> > 
> > Definitely, I'm planning on adding a number of other tests, to cover all of
> the
> > cases. That one should work -- a frame with no header should still call
> > DidSetFeaturePolicyHeader -- the FrameLoader always calls
> > client()->didSetFeaturePolicyHeader, even if the header is empty. (Maybe it
> > should be renamed, or dropped in that case)
> > 
> > If I fix that (so that the header is not replicated over IPC if not sent)
then
> > the FrameReplicationState could still be initialized to a correct empty
> string,
> > and this method would still be called by
RenderFrameProxy::SetReplicatedState,
> > and would create the correct policy, inheriting everything from the parent.
> > 
> > I'll add an explicit test for that case as well. (And its various
> permutations)
> 
> That sounds great, I missed that DidSetFeaturePolicyHeader was done even for
the
> empty header.
> 
> I see that you've changed it now (which is great, as it benefits the common
> case). One issue for that though is that we need to deal properly with the
same
> frame navigating from a document with a non-empty header to a document with an
> empty header.  In that case, we need to reset the header's value in
> FrameTreeNode's replication state.  This should've worked when sending the
empty
> header IPC, but I'm not so sure it does now that we drop it.  We could
probably
> clear the replication state's feature_policy_header in
> NavigatorImpl::DidNavigate, where we update the other replicated properties,
> like origin or insecure_request_policy.  Probably another thing worth having a
> test for eventually.

Thanks for catching that -- I've fixed it now, and added tests for it. (For both
same and for cross-origin navigations)

https://codereview.chromium.org/2483703002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2483703002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:33: //
errors in the input will cause an error message fo be appended to it.
On 2016/11/10 17:50:28, alexmos wrote:
> s/fo/to/

Done.

[commit-bot: I haz the power, 2016-11-16 21:03:46]

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

[commit-bot: I haz the power, 2016-11-16 21:03:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[iclelland, 2016-11-16 21:07:37]

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

[commit-bot: I haz the power, 2016-11-16 21:08:49]

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, 2016-11-16 22:23:13]

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

[commit-bot: I haz the power, 2016-11-16 22:23:14]

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

[iclelland, 2016-11-17 04:26:09]

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

[commit-bot: I haz the power, 2016-11-17 04:26:47]

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, 2016-11-17 07:16:56]

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

[commit-bot: I haz the power, 2016-11-17 07:16:56]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-17 14:36:26]

mkwst@chromium.org changed required reviewers:
+ alexmos@chromium.org

[Mike West, 2016-11-17 14:36:27]

This LGTM, but I'm marking alexmos@ as required. Once he's happy with the
change, I'm happy with it too. :)

[iclelland, 2016-11-17 15:23:32]

On 2016/11/17 14:36:27, Mike West wrote:
> This LGTM, but I'm marking alexmos@ as required. Once he's happy with the
> change, I'm happy with it too. :)

SGTM, thanks!

[iclelland, 2016-11-17 20:25:59]

alexmos, can you take another look?

[alexmos, 2016-11-17 21:45:59]

Thanks, looks close.  One request about tests below and a few nits.

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

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:258: void
FrameTreeNode::ResetFeaturePolicy() {
nit: move this below SetFeaturePolicyHeader to match order in the header.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:586:
render_frame_host->frame_tree_node()->ResetFeaturePolicy();
nit: update comment to mention feature policy (so it doesn't seem like this is
part of resetting CSP)

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:561:
command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
Do you mind splitting this out into a new subclass of SitePerProcessBrowserTest?
 Similar to the way SitePerProcessHighDPIBrowserTest or
SitePerProcessEmbedderCSPEnforcementBrowserTest is set up?

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8567:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
These tests provide good coverage that the browser process has correct knowledge
about the feature policy, but they don't test whether we also replicate it
correctly to remote frames, which I think we also should have, if possible.  The
layout tests you used to have provided most of that coverage, so you could bring
those back, or you could add another test here with a cross-site iframe, similar
to what's done in CrossSiteIframeBlockedByParentCSPFromHeaders,
FrameOwnerPropertiesPropagationCSP,
PassiveMixedContentInIframeWithStrictBlocking, etc.  With a cross-site iframe,
we'd be checking that we propagate the feature policy correctly to the OOPIF's
remote parent in order for the frame itself to get the right policy.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8570: GURL
b_url(embedded_test_server()->GetURL("a.com", "/feature-policy2.html"));
nit: b_url implies that it's at b.com (this is the convention we've been using
in this file).  Can you rename it to something else?  Same for c_url in this
test.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8603:
TestFrameNavigationObserver frame_observer(root);
This doesn't seem necessary.  NavigateToURL will wait for for the navigation to
fully finish: it will return after DidStopLoading, which is after commit.  You
can use one or the other; I think just NavigateToURL should be sufficient.

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

https://codereview.chromium.org/2483703002/diff/90001/content/common/frame_me...
content/common/frame_messages.h:1005:
IPC_MESSAGE_ROUTED1(FrameHostMsg_DidSetFeaturePolicyHeader, std::string)
Please add a comment for what this is for, and mention it's only sent when the
header is present/non-empty.

[iclelland, 2016-11-18 16:10:06]

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

[commit-bot: I haz the power, 2016-11-18 16:10:46]

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

[iclelland, 2016-11-18 16:12:38]

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

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:258: void
FrameTreeNode::ResetFeaturePolicy() {
On 2016/11/17 21:45:58, alexmos wrote:
> nit: move this below SetFeaturePolicyHeader to match order in the header.

Done.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:586:
render_frame_host->frame_tree_node()->ResetFeaturePolicy();
On 2016/11/17 21:45:58, alexmos wrote:
> nit: update comment to mention feature policy (so it doesn't seem like this is
> part of resetting CSP)

Done.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:561:
command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
On 2016/11/17 21:45:59, alexmos wrote:
> Do you mind splitting this out into a new subclass of
SitePerProcessBrowserTest?
>  Similar to the way SitePerProcessHighDPIBrowserTest or
> SitePerProcessEmbedderCSPEnforcementBrowserTest is set up?

That's a good idea, I hadn't seen that -- thanks. Done.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8567:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
On 2016/11/17 21:45:59, alexmos wrote:
> These tests provide good coverage that the browser process has correct
knowledge
> about the feature policy, but they don't test whether we also replicate it
> correctly to remote frames, which I think we also should have, if possible. 
The
> layout tests you used to have provided most of that coverage, so you could
bring
> those back, or you could add another test here with a cross-site iframe,
similar
> to what's done in CrossSiteIframeBlockedByParentCSPFromHeaders,
> FrameOwnerPropertiesPropagationCSP,
> PassiveMixedContentInIframeWithStrictBlocking, etc.  With a cross-site iframe,
> we'd be checking that we propagate the feature policy correctly to the OOPIF's
> remote parent in order for the frame itself to get the right policy.

Definitely should have that. I'm bringing the layout tests back for specific
features, so those should test it, on the site-isolation bots. 

For the generic framework, I like your idea of adding a couple more browser
tests. Unfortunately, I don't currently have a generic way to get the renderer
to tell the browser about its feature policy details. (We haven't specced a JS
api for that). I could create an internals interface for it, or we could leave
the testing to layout tests for specific features (See
https://codereview.chromium.org/2492623002/, for instance)

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8570: GURL
b_url(embedded_test_server()->GetURL("a.com", "/feature-policy2.html"));
On 2016/11/17 21:45:59, alexmos wrote:
> nit: b_url implies that it's at b.com (this is the convention we've been using
> in this file).  Can you rename it to something else?  Same for c_url in this
> test.

Done.

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8603:
TestFrameNavigationObserver frame_observer(root);
On 2016/11/17 21:45:58, alexmos wrote:
> This doesn't seem necessary.  NavigateToURL will wait for for the navigation
to
> fully finish: it will return after DidStopLoading, which is after commit.  You
> can use one or the other; I think just NavigateToURL should be sufficient.

Removed... somehow during testing it looked like I was seeing a situation where
the second navigation wouldn't start because the first one was still pending,
but I must have been mistaken (or it's been resolved somehow) because it doesn't
look like I need it now. Thanks.

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

https://codereview.chromium.org/2483703002/diff/90001/content/common/frame_me...
content/common/frame_messages.h:1005:
IPC_MESSAGE_ROUTED1(FrameHostMsg_DidSetFeaturePolicyHeader, std::string)
On 2016/11/17 21:45:59, alexmos wrote:
> Please add a comment for what this is for, and mention it's only sent when the
> header is present/non-empty.

Done.

[commit-bot: I haz the power, 2016-11-18 16:49:45]

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

[commit-bot: I haz the power, 2016-11-18 16:49:47]

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, 2016-11-18 17:21:44]

Thanks, everything looks good, but looks like the trybots are red on a compile
failure?  And do you still need to re-add the layout tests?

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8567:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
On 2016/11/18 16:12:37, iclelland wrote:
> On 2016/11/17 21:45:59, alexmos wrote:
> > These tests provide good coverage that the browser process has correct
> knowledge
> > about the feature policy, but they don't test whether we also replicate it
> > correctly to remote frames, which I think we also should have, if possible. 
> The
> > layout tests you used to have provided most of that coverage, so you could
> bring
> > those back, or you could add another test here with a cross-site iframe,
> similar
> > to what's done in CrossSiteIframeBlockedByParentCSPFromHeaders,
> > FrameOwnerPropertiesPropagationCSP,
> > PassiveMixedContentInIframeWithStrictBlocking, etc.  With a cross-site
iframe,
> > we'd be checking that we propagate the feature policy correctly to the
OOPIF's
> > remote parent in order for the frame itself to get the right policy.
> 
> Definitely should have that. I'm bringing the layout tests back for specific
> features, so those should test it, on the site-isolation bots. 
> 
> For the generic framework, I like your idea of adding a couple more browser
> tests. Unfortunately, I don't currently have a generic way to get the renderer
> to tell the browser about its feature policy details. (We haven't specced a JS
> api for that). I could create an internals interface for it, or we could leave
> the testing to layout tests for specific features (See
> https://codereview.chromium.org/2492623002/, for instance)
>  

In that case I think it's fine to go with layout tests.  But it doesn't seem
like they made it to the latest patchset?

[iclelland, 2016-11-18 18:23:09]

On 2016/11/18 17:21:44, alexmos wrote:
> Thanks, everything looks good, but looks like the trybots are red on a compile
> failure?  And do you still need to re-add the layout tests?

Yes, the change to SecurityContext.h here broke a test that I added in an
intervening commit, so I'm updating that test to use the new
setFeaturePolicyFromHeader method instead.

> 
>
https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2483703002/diff/90001/content/browser/site_pe...
> content/browser/site_per_process_browsertest.cc:8567:
> IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
> On 2016/11/18 16:12:37, iclelland wrote:
> > On 2016/11/17 21:45:59, alexmos wrote:
> > > These tests provide good coverage that the browser process has correct
> > knowledge
> > > about the feature policy, but they don't test whether we also replicate it
> > > correctly to remote frames, which I think we also should have, if
possible. 
> > The
> > > layout tests you used to have provided most of that coverage, so you could
> > bring
> > > those back, or you could add another test here with a cross-site iframe,
> > similar
> > > to what's done in CrossSiteIframeBlockedByParentCSPFromHeaders,
> > > FrameOwnerPropertiesPropagationCSP,
> > > PassiveMixedContentInIframeWithStrictBlocking, etc.  With a cross-site
> iframe,
> > > we'd be checking that we propagate the feature policy correctly to the
> OOPIF's
> > > remote parent in order for the frame itself to get the right policy.
> > 
> > Definitely should have that. I'm bringing the layout tests back for specific
> > features, so those should test it, on the site-isolation bots. 
> > 
> > For the generic framework, I like your idea of adding a couple more browser
> > tests. Unfortunately, I don't currently have a generic way to get the
renderer
> > to tell the browser about its feature policy details. (We haven't specced a
JS
> > api for that). I could create an internals interface for it, or we could
leave
> > the testing to layout tests for specific features (See
> > https://codereview.chromium.org/2492623002/, for instance)
> >  
> 
> In that case I think it's fine to go with layout tests.  But it doesn't seem
> like they made it to the latest patchset?

No, the tests I removed will no longer pass, as they were testing the presence
of the document.write attribute, which isn't part of the MVP anymore. The linked
CL for vibrate will be the first actual behavioural change to go out as part of
feature-policy, and so those layout tests will stand in for it. Every feature
from that point on will also have similar layout tests to ensure correct
behavior in same- and cross-origin frames.

[iclelland, 2016-11-18 18:53:22]

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

[commit-bot: I haz the power, 2016-11-18 18:54:07]

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, 2016-11-18 20:25:00]

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

[commit-bot: I haz the power, 2016-11-18 20:25:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2016-11-18 21:14:45]

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

[commit-bot: I haz the power, 2016-11-18 21:15:32]

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

[alexmos, 2016-11-18 21:47:15]

> No, the tests I removed will no longer pass, as they were testing the presence
> of the document.write attribute, which isn't part of the MVP anymore. The
linked
> CL for vibrate will be the first actual behavioural change to go out as part
of
> feature-policy, and so those layout tests will stand in for it. Every feature
> from that point on will also have similar layout tests to ensure correct
> behavior in same- and cross-origin frames.

Ah, I see, thanks for clarifying.  In that case, LGTM given that the layout
tests
will be added in the subsequent CL.

It'd still be nice to add a subframe case to one of the browsertests here. I.e.,
navigate a cross-site subframe to one of your URLs with a feature policy and see
that at least the browser-side replication state is correct in the subframe
FrameTreeNode.  I'll leave it up to you -- the logic for replicating the header
doesn't care whether it's a main frame or subframe, so it's not that big of a
big deal, but it could prevent future regressions if we ever refactor this code.

[commit-bot: I haz the power, 2016-11-19 01:45:54]

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

[commit-bot: I haz the power, 2016-11-19 01:45:56]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-11-20 05:49:06]

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

[commit-bot: I haz the power, 2016-11-20 05:49: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, 2016-11-20 05:51:06]

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

[commit-bot: I haz the power, 2016-11-20 05:51:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dcheng, 2016-11-20 21:04:10]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-11-20 21:04:12]

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

https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:263:
replication_state_.feature_policy_header = "";
Nit: std::string::clear() is slightly cheaper.

Btw, how big can these headers get? Do we explicitly want to release as much
storage as possible here with something like STLClearObject() from stl_util.h?

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

https://codereview.chromium.org/2483703002/diff/170001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3148: routing_id_,
base::UTF16ToUTF8(base::StringPiece16(header_value))));
Nit: header_value.utf8() will be slightly more efficient, since the WebString
may already contain all 8-bit chars.

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:595: if (client() &&
!featurePolicyHeader.isEmpty())
Is this null check necessary? The frame should be attached here.

[raymes, 2016-11-21 02:17:34]

A few small thoughts after scanning through :)

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:105: Vector<String>*
messages) {
I think we should DCHECK(!m_featurePolicy) here?

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:588:
m_frame->securityContext()->setFeaturePolicyFromHeader(
nit: since only the parent's feature policy is really needed, should we just
pass that in instead of the whole parentSecurityContext?

[iclelland, 2016-11-21 04:10:07]

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

[commit-bot: I haz the power, 2016-11-21 04:10:16]

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

[iclelland, 2016-11-21 05:05:06]

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

[commit-bot: I haz the power, 2016-11-21 05:05:20]

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

[iclelland, 2016-11-21 05:05:33]

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

https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:263:
replication_state_.feature_policy_header = "";
On 2016/11/20 21:04:12, dcheng wrote:
> Nit: std::string::clear() is slightly cheaper.

Done.

> Btw, how big can these headers get? Do we explicitly want to release as much
> storage as possible here with something like STLClearObject() from stl_util.h?

There's no explicit limit in the spec; a site could provide a policy with a
large number of explicitly-authorized domains for each feature. We should
probably have a sanity limit of some small number of kilobytes.

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

https://codereview.chromium.org/2483703002/diff/170001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3148: routing_id_,
base::UTF16ToUTF8(base::StringPiece16(header_value))));
On 2016/11/20 21:04:12, dcheng wrote:
> Nit: header_value.utf8() will be slightly more efficient, since the WebString
> may already contain all 8-bit chars.

Done.

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:105: Vector<String>*
messages) {
On 2016/11/21 02:17:34, raymes wrote:
> I think we should DCHECK(!m_featurePolicy) here?

To ensure that it hasn't been set previously -- certainly can't hurt. Thanks,
done.

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:588:
m_frame->securityContext()->setFeaturePolicyFromHeader(
On 2016/11/21 02:17:34, raymes wrote:
> nit: since only the parent's feature policy is really needed, should we just
> pass that in instead of the whole parentSecurityContext?

Makes sense; I was thinking that methods on SecurityContext should really be
working at that level of abstraction (i.e., other SecurityContexts), but since
SC knows about FP, and provides methods to manipulate it, I don't see anything
wrong with using it as a parameter.

https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:595: if (client() &&
!featurePolicyHeader.isEmpty())
On 2016/11/20 21:04:12, dcheng wrote:
> Is this null check necessary? The frame should be attached here.

I wasn't 100% certain that that had to be the case, so it was a sanity check for
me to ensure that it was nonnull before dereferencing it. I can easily replace
with a DCHECK, or eliminate if it's completely unnecessary.

[commit-bot: I haz the power, 2016-11-21 06:53:57]

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

[commit-bot: I haz the power, 2016-11-21 06:53:58]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-11-22 18:45:34]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-11-22 18:45:35]

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

[commit-bot: I haz the power, 2016-11-22 18:45:56]

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, 2016-11-22 18:54:28]

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

[commit-bot: I haz the power, 2016-11-22 18:54:30]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[iclelland, 2016-11-22 19:02:11]

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

[iclelland, 2016-11-22 19:02:12]

+r pfeldman, Can you take a look as OWNER at
  content/browser/site_per_process_browsertest.cc
  content/common/*
  content/renderer/*

Thanks!

[pfeldman, 2016-11-22 19:47:51]

lgtm

[iclelland, 2016-11-22 19:48:19]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2016-11-22 19:48:57]

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, 2016-11-22 22:58:24]

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

[commit-bot: I haz the power, 2016-11-22 22:58:26]

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

[iclelland, 2016-11-22 23:36:42]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:37:11]

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, 2016-11-23 02:00:46]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1479857802670520,
"parent_rev": "82b65af56811dba76fdbc55db103dfdbaf3b5040", "commit_rev":
"128a12ce3a27b20db3df4122f536f75eeb154cce"}

[commit-bot: I haz the power, 2016-11-23 02:01:16]

Description was changed from

==========
Replicate feature policy headers to remote frames

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

to

==========
Replicate feature policy headers to remote frames

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

[commit-bot: I haz the power, 2016-11-23 02:01:18]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-11-23 02:02:56]

Description was changed from

==========
Replicate feature policy headers to remote frames

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

to

==========
Replicate feature policy headers to remote frames

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

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

[commit-bot: I haz the power, 2016-11-23 02:02:58]

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