Replicate a parsed feature policy representation so it doesn't need to be parsed in the browser pro…

Replicate a parsed feature policy representation so it doesn't need to be parsed in the browser process

Currently we just replicated the feature policy header as a raw string. However
we need to reconstruct the header in the browser process and we can't do that
from a string. So instead we propagate a parsed representation of it in the
replication state.

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

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

[raymes, 2016-11-22 05:10:34]

Description was changed from

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process
==========

to

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[raymes, 2016-11-22 05:16:33]

Hey Ian,

There's a bunch of rough edges which I will clean up, but can you let me know if
the high level approach looks ok to you? 

This isn't ideal but the challenge is that we need to return a parsed
representation in classes which can be passed in/out of the webkit public API.
We also need to convert the blink-classes to/from classes that can be serialized
for Chrome IPC in order to put them into the FrameReplicationState. This makes
things a little nasty but I don't think there is a way around it for now.

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:13: struct
WebFeaturePolicy {
When we make the FeaturePolicy class publicly available, we can merge it with
this one.

[raymes, 2016-11-22 05:17:12]

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

[raymes, 2016-11-22 05:17:13]

Hey Ian,

There's a bunch of rough edges which I will clean up, but can you let me know if
the high level approach looks ok to you? 

This isn't ideal but the challenge is that we need to return a parsed
representation in classes which can be passed in/out of the webkit public API.
We also need to convert the blink-classes to/from classes that can be serialized
for Chrome IPC in order to put them into the FrameReplicationState. This makes
things a little nasty but I don't think there is a way around it for now.

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:13: struct
WebFeaturePolicy {
On 2016/11/22 05:16:33, raymes wrote:
> When we make the FeaturePolicy class publicly available, we can merge it with
> this one.

[iclelland, 2016-11-22 21:01:51]

On 2016/11/22 05:17:13, raymes wrote:
> Hey Ian,
> 
> There's a bunch of rough edges which I will clean up, but can you let me know
if
> the high level approach looks ok to you? 

I think I do like the high-level approach, it makes sense to serialize the
policy into whitelists to avoid any JSON parsing of untrusted data. The
alternative is to serialize the entire policy, I think -- parsed headers,
inherited policy, and everything, but this is simpler.

Is the plan, then, to make the implementation of blink::(Web)FeaturePolicy
available as part of the Blink API, so that the full policy can be reconstructed
in the browser and stored on the RenderFrameHost?

> 
> This isn't ideal but the challenge is that we need to return a parsed
> representation in classes which can be passed in/out of the webkit public API.
> We also need to convert the blink-classes to/from classes that can be
serialized
> for Chrome IPC in order to put them into the FrameReplicationState. This makes
> things a little nasty but I don't think there is a way around it for now.

Probably not with Chrome IPC; Is there any work being done to mojofy frame
replication? If we can use that framework, then typemapping should make the code
a lot cleaner.

> 
>
https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):
> 
>
https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebFeaturePolicy.h:13: struct
> WebFeaturePolicy {
> On 2016/11/22 05:16:33, raymes wrote:
> > When we make the FeaturePolicy class publicly available, we can merge it
with
> > this one.

[iclelland, 2016-11-22 21:02:05]

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:13: struct
WebFeaturePolicy {
On 2016/11/22 05:17:12, raymes wrote:
> On 2016/11/22 05:16:33, raymes wrote:
> > When we make the FeaturePolicy class publicly available, we can merge it
with
> > this one.
> 

I was working on that, which probably overlapped a bit with some of this CL -- I
think that we'll end up with a real WebFeaturePolicy class declared here, and
defined in Source/platform/exported.

[raymes, 2016-11-23 04:54:26]

Description was changed from

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process

Currently we just replicated the feature policy header as a raw string. However 
we need to reconstruct the header in the browser process and we can't do that 
from a string. So instead we propagate a parsed representation of it in the 
replication state.

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

[raymes, 2016-11-23 04:55:05]

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

[raymes, 2016-11-23 04:55:06]

iclelland - could you PTAL?

+alexmos too

[raymes, 2016-11-23 04:55:14]

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

[commit-bot: I haz the power, 2016-11-23 04:55:28]

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-23 06:01:47]

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

[commit-bot: I haz the power, 2016-11-23 06:01:48]

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

[raymes, 2016-11-23 08:21:05]

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

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

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-23 08:24:03]

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

[commit-bot: I haz the power, 2016-11-23 08:24:04]

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

[iclelland, 2016-11-23 20:20:28]

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:13: struct
WebFeaturePolicy {
On 2016/11/22 21:02:05, iclelland wrote:
> On 2016/11/22 05:17:12, raymes wrote:
> > On 2016/11/22 05:16:33, raymes wrote:
> > > When we make the FeaturePolicy class publicly available, we can merge it
> with
> > > this one.
> > 
> 
> I was working on that, which probably overlapped a bit with some of this CL --
I
> think that we'll end up with a real WebFeaturePolicy class declared here, and
> defined in Source/platform/exported.

But we probably can't use that in the browser process, so it'll have to have
impl in content/common instead.

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

https://codereview.chromium.org/2520223002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:660: if (first.feature_name !=
second.feature_name ||
Can this be written as

std::tie(first.feature_name, first.matches_all_origins, first.origins) ==
std::tie(second.feature_name, second.matches_all_origins, second.origins)

?

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

https://codereview.chromium.org/2520223002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.h:493: const
blink::WebVector<blink::WebFeaturePolicy::ParsedWhitelist>&
This type is used so much, what do you think about declaring it with a using
decl in WebFeaturePolicy.h, something like

using WebParsedFeaturePolicyHeader =
WebVector<WebFeaturePolicy::ParsedWhitelist>

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:136:
WebVector<WebFeaturePolicy::ParsedWhitelist> FeaturePolicy::parseFeaturePolicy(
As a new public interface, it would be good to have explicit coverage of this in
the unit tests. At least we could verify that the strings are pulled out
correctly, and that the right number of items are seen.

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:142: if
(origin->isUnique())
This is a new check -- as far as I can tell, a unique origin would mean that
'self' is not resolvable to any specific origin. Is our only response in that
situation to reject the policy completely?

(And if we have to, then we should have a couple of tests for it -- a unit test
here, and maybe a browsertest where one frame has an opaque origin)

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:185: if
(originUrl.isValid())
This might as well be
if (KURL(KURL(), targetString).isValid())

but I'm wondering if there's a way to store the parsed origins here, instead of
the raw strings

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:203: for
(const WebFeaturePolicy::ParsedWhitelist& parsedWhitelist : policy) {
This method seems like it's mostly a conversion function between
WebFeaturePolicy::ParsedWhiteList and FeaturePolicy::Whitelist. Would it look
cleaner to express it as such? Something like

//static
std::unique_ptr<Whitelist> Whitelist::from(const
WebFeaturePolicy::ParsedWhitelist&)

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFeaturePolicy.h:18: WebVector<WebString>
origins;
Can we use WebVector<WebSecurityOrigin> for this?

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebRemoteFrame.h (right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebRemoteFrame.h:60: const = 0;
That's a really unfortunate line wrap :(

[alexmos, 2016-11-29 01:44:47]

The replication bits look good. I was waiting for Ian to be happy with the rest
of the CL before doing a full review, but let me know if I should do it sooner.

[raymes, 2016-11-29 08:20:34]

Thanks Ian and Alex.

Alex, feel free to wait for Ian. Cheers!

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

https://codereview.chromium.org/2520223002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:660: if (first.feature_name !=
second.feature_name ||
On 2016/11/23 20:20:28, iclelland wrote:
> Can this be written as
> 
> std::tie(first.feature_name, first.matches_all_origins, first.origins) ==
> std::tie(second.feature_name, second.matches_all_origins, second.origins)
> 
> ?

Done.

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

https://codereview.chromium.org/2520223002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.h:493: const
blink::WebVector<blink::WebFeaturePolicy::ParsedWhitelist>&
On 2016/11/23 20:20:28, iclelland wrote:
> This type is used so much, what do you think about declaring it with a using
> decl in WebFeaturePolicy.h, something like
> 
> using WebParsedFeaturePolicyHeader =
> WebVector<WebFeaturePolicy::ParsedWhitelist>

Done - I was contemplating that :P. I called it WebParsedFeaturePolicy since it
might not necessarily be a header (the same structure could represent an
attribute).

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:185: if
(originUrl.isValid())
On 2016/11/23 20:20:28, iclelland wrote:
> This might as well be
> if (KURL(KURL(), targetString).isValid())
> 
> but I'm wondering if there's a way to store the parsed origins here, instead
of
> the raw strings

Good point - we can probably use a WebSecurityOrigin and url::Origin? I've
changed it to use those.

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:203: for
(const WebFeaturePolicy::ParsedWhitelist& parsedWhitelist : policy) {
On 2016/11/23 20:20:28, iclelland wrote:
> This method seems like it's mostly a conversion function between
> WebFeaturePolicy::ParsedWhiteList and FeaturePolicy::Whitelist. Would it look
> cleaner to express it as such? Something like
> 
> //static
> std::unique_ptr<Whitelist> Whitelist::from(const
> WebFeaturePolicy::ParsedWhitelist&)

I haven't made this change yet (and my connection to corp is being really slow)
but it may be worth looking at the other changes to make sure they're ok :)

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFeaturePolicy.h:18: WebVector<WebString>
origins;
On 2016/11/23 20:20:28, iclelland wrote:
> Can we use WebVector<WebSecurityOrigin> for this?

Done.

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebRemoteFrame.h (right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebRemoteFrame.h:60: const = 0;
On 2016/11/23 20:20:28, iclelland wrote:
> That's a really unfortunate line wrap :(

Done.

[raymes, 2016-11-30 06:12:47]

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

[commit-bot: I haz the power, 2016-11-30 06:13:40]

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

[raymes, 2016-11-30 06:25:06]

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

[raymes, 2016-11-30 06:25:17]

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:136:
WebVector<WebFeaturePolicy::ParsedWhitelist> FeaturePolicy::parseFeaturePolicy(
On 2016/11/23 20:20:28, iclelland wrote:
> As a new public interface, it would be good to have explicit coverage of this
in
> the unit tests. At least we could verify that the strings are pulled out
> correctly, and that the right number of items are seen.

Done - I added a test.

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:142: if
(origin->isUnique())
Yeah it is a new check. You're right - let me remove it for now because this CL
is already quite large. We can consider it more in followups.

https://codereview.chromium.org/2520223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:203: for
(const WebFeaturePolicy::ParsedWhitelist& parsedWhitelist : policy) {
On 2016/11/29 08:20:34, raymes wrote:
> On 2016/11/23 20:20:28, iclelland wrote:
> > This method seems like it's mostly a conversion function between
> > WebFeaturePolicy::ParsedWhiteList and FeaturePolicy::Whitelist. Would it
look
> > cleaner to express it as such? Something like
> > 
> > //static
> > std::unique_ptr<Whitelist> Whitelist::from(const
> > WebFeaturePolicy::ParsedWhitelist&)
> 
> I haven't made this change yet (and my connection to corp is being really
slow)
> but it may be worth looking at the other changes to make sure they're ok :)

Done now.

[commit-bot: I haz the power, 2016-11-30 06:25:38]

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-30 10:35:08]

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

[commit-bot: I haz the power, 2016-11-30 10:35:09]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-11-30 22:13:10]

This lgtm, thanks!

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: if
(!origin->isUnique())
This isn't strictly necessary for the policy objects in the renderer -- 
SecurityOrigin::isSameSchemeHostPort will return false for any unique urls (even
comparing to themselves), so there wouldn't be any harm in adding them to the
list. Is this to avoid sending them to the browser, or is there an issue with
serialization?

(Curious, mostly; I don't have a problem adding the test here)

[Marijn Kruisselbrink, 2016-11-30 22:21:00]

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: if
(!origin->isUnique())
On 2016/11/30 at 22:13:10, iclelland wrote:
> SecurityOrigin::isSameSchemeHostPort will return false for any unique urls
(even comparing to themselves)

This is not true. isSameSchemeHostPort() starts with if (this == other) return
true;

[raymes, 2016-11-30 23:18:22]

raymes@chromium.org changed reviewers:
+ meacer@chromium.org, pfeldman@chromium.org

[raymes, 2016-11-30 23:18:23]

alexmos: could you ptal? Thanks!

Also +pfeldman for content/ and WebKit/

+meacer for frame_messages.h

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: if
(!origin->isUnique())
On 2016/11/30 22:13:10, iclelland wrote:
> This isn't strictly necessary for the policy objects in the renderer -- 
> SecurityOrigin::isSameSchemeHostPort will return false for any unique urls
(even
> comparing to themselves), so there wouldn't be any harm in adding them to the
> list. Is this to avoid sending them to the browser, or is there an issue with
> serialization?
> 
> (Curious, mostly; I don't have a problem adding the test here)

Good point - I guess it just felt weird to add a unique origin here because this
is meant to be a parsed representation of the FP json, which only really allows
scheme/host/port origins to be specified.

[alexmos, 2016-12-01 02:04:19]

Looks good, just a few minor things below.

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1772: const
std::vector<FeaturePolicyParsedWhitelist>& parsed_header) {
nit: I'm curious if we could make the type a bit more readable.  Is "Parsed"
needed in the whitelist class name, or would it be simpler to name it
FeaturePolicyWhitelist, or even just FeatureWhitelist to emphasize that it's a
per-feature whitelist and not per-policy?

Alternatively, maybe you could repeat what you did with WebParsedFeaturePolicy
and define the whole parsed header type as ParsedFeaturePolicy?

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

https://codereview.chromium.org/2520223002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:652: bool operator==(const
FeaturePolicyParsedWhitelist& first,
Any value in putting this next to the struct definition in
frame_replication_state, or do you envision this being useful only in this test?

https://codereview.chromium.org/2520223002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8709:
EXPECT_EQ(CreateExpectedFeaturePolicyHeader("vibrate", false,
nit: the tests might be a bit more readable if instead of
CreateExpectedFeaturePolicyHeader you had two helpers: one that takes the
feature name + origins vector, and another for the matches_all_origins case that
takes just the feature name.

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_m...
content/common/frame_messages.h:1021: // delivered with the document being
loaded into the frame.
Might be worth explaining what the arg is now (a list of origin whitelists for
each feature).

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
content/common/frame_replication_state.h:25: // part of the
FrameReplicationState below.
nit: Maybe also mention that this is a whitelist for one feature, and the full
feature policy is a list of these.

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
content/common/frame_replication_state.h:102:
std::vector<FeaturePolicyParsedWhitelist> feature_policy_header;
Do we need to keep calling this feature_policy_header and not just
feature_policy?  Presumably yes, since a frame's feature policy might also
depend on <iframe> enable/disable attributes?

[raymes, 2016-12-01 03:03:09]

Thanks Alex!

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1772: const
std::vector<FeaturePolicyParsedWhitelist>& parsed_header) {
In blink I was trying to distinguish it from the FeaturePolicy::Whitelist class.
We don't have anything like that in content/ yet but we may have something as we
try to share logic between browser/renderer. So I think it's safer to keep the
FaturePolicyParsedWhitelist name. I can definitely do the typdef you suggest
though.

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

https://codereview.chromium.org/2520223002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:652: bool operator==(const
FeaturePolicyParsedWhitelist& first,
On 2016/12/01 02:04:19, alexmos wrote:
> Any value in putting this next to the struct definition in
> frame_replication_state, or do you envision this being useful only in this
test?

Right now it's only used from the test and I can't see a use outside from it
atm, so I was thinking to leave it here. We could always move it out if needed.

https://codereview.chromium.org/2520223002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8709:
EXPECT_EQ(CreateExpectedFeaturePolicyHeader("vibrate", false,
On 2016/12/01 02:04:19, alexmos wrote:
> nit: the tests might be a bit more readable if instead of
> CreateExpectedFeaturePolicyHeader you had two helpers: one that takes the
> feature name + origins vector, and another for the matches_all_origins case
that
> takes just the feature name.

Done.

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_m...
content/common/frame_messages.h:1021: // delivered with the document being
loaded into the frame.
On 2016/12/01 02:04:19, alexmos wrote:
> Might be worth explaining what the arg is now (a list of origin whitelists for
> each feature).

Done.

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
content/common/frame_replication_state.h:25: // part of the
FrameReplicationState below.
On 2016/12/01 02:04:19, alexmos wrote:
> nit: Maybe also mention that this is a whitelist for one feature, and the full
> feature policy is a list of these.

Done.

https://codereview.chromium.org/2520223002/diff/200001/content/common/frame_r...
content/common/frame_replication_state.h:102:
std::vector<FeaturePolicyParsedWhitelist> feature_policy_header;
On 2016/12/01 02:04:19, alexmos wrote:
> Do we need to keep calling this feature_policy_header and not just
> feature_policy?  Presumably yes, since a frame's feature policy might also
> depend on <iframe> enable/disable attributes?

Exactly - this is to distinguish it from the attribute that we plan to add.

[iclelland, 2016-12-01 03:31:25]

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2520223002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: if
(!origin->isUnique())
On 2016/11/30 22:21:00, Marijn Kruisselbrink wrote:
> On 2016/11/30 at 22:13:10, iclelland wrote:
> > SecurityOrigin::isSameSchemeHostPort will return false for any unique urls
> (even comparing to themselves)
> 
> This is not true. isSameSchemeHostPort() starts with if (this == other) return
> true;

You're absolutely right -- when I went to check to see how isUnique was handled,
I glossed right over the first lines of that method. Sorry for the confusion.

[alexmos, 2016-12-01 22:27:54]

Thanks!  LGTM

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2520223002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1772: const
std::vector<FeaturePolicyParsedWhitelist>& parsed_header) {
On 2016/12/01 03:03:09, raymes wrote:
> In blink I was trying to distinguish it from the FeaturePolicy::Whitelist
class.
> We don't have anything like that in content/ yet but we may have something as
we
> try to share logic between browser/renderer. So I think it's safer to keep the
> FaturePolicyParsedWhitelist name. I can definitely do the typdef you suggest
> though.

Acknowledged.

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

https://codereview.chromium.org/2520223002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:652: bool operator==(const
FeaturePolicyParsedWhitelist& first,
On 2016/12/01 03:03:09, raymes wrote:
> On 2016/12/01 02:04:19, alexmos wrote:
> > Any value in putting this next to the struct definition in
> > frame_replication_state, or do you envision this being useful only in this
> test?
> 
> Right now it's only used from the test and I can't see a use outside from it
> atm, so I was thinking to leave it here. We could always move it out if
needed.

Acknowledged.

[raymes, 2016-12-05 02:09:16]

ping pfeldman, meacer, PTAL :)

[raymes, 2016-12-05 02:09:24]

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

[commit-bot: I haz the power, 2016-12-05 02:09:40]

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-12-05 03:57:30]

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

[commit-bot: I haz the power, 2016-12-05 03:57:31]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-12-05 18:53:50]

lgtm

[meacer, 2016-12-05 19:30:49]

lgtm

[raymes, 2016-12-05 23:35:11]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-12-05 23:35:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2520223002/#ps240001
(title: "Replicate a parsed feature policy representation so it doesn't need to
be parsed in the browser pro…")

[commit-bot: I haz the power, 2016-12-05 23:35:47]

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-12-05 23:41:37]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480980911884870,
"parent_rev": "5473a61473ef6a88af4d8d6fc2700791b7d90752", "commit_rev":
"7144a563438fe667b4bc2cb264cb01f3fe52e7b1"}

[commit-bot: I haz the power, 2016-12-05 23:42:02]

Description was changed from

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process

Currently we just replicated the feature policy header as a raw string. However 
we need to reconstruct the header in the browser process and we can't do that 
from a string. So instead we propagate a parsed representation of it in the 
replication state.

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

to

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process

Currently we just replicated the feature policy header as a raw string. However 
we need to reconstruct the header in the browser process and we can't do that 
from a string. So instead we propagate a parsed representation of it in the 
replication state.

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

[commit-bot: I haz the power, 2016-12-05 23:42:04]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-05 23:44:20]

Description was changed from

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process

Currently we just replicated the feature policy header as a raw string. However 
we need to reconstruct the header in the browser process and we can't do that 
from a string. So instead we propagate a parsed representation of it in the 
replication state.

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

to

==========
Replicate a parsed feature policy representation so it doesn't need to be parsed
in the browser process

Currently we just replicated the feature policy header as a raw string. However
we need to reconstruct the header in the browser process and we can't do that
from a string. So instead we propagate a parsed representation of it in the
replication state.

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

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

[commit-bot: I haz the power, 2016-12-05 23:44:22]

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