CSP: group policies in didAddContentSecurityPolicy.

CSP: group policies in didAddContentSecurityPolicy.

This CL makes chrome sends one IPC in
ContentSecurityPolicy::reportAccumulatedHeaders instead
of one IPC per CSP headers.

In addition, in didAddContentSecurityPolicy, the CSP header
is no more sent since it is already included in the parsed
policy.

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

Review-Url: https://codereview.chromium.org/2764993002
Cr-Commit-Position: refs/heads/master@{#460060}
Committed: https://chromium.googlesource.com/chromium/src/+/662aa65677aabd2ee107e76a6928574cd167bc75

[arthursonzogni, 2017-03-21 16:18:18]

Description was changed from

==========
CSP send policies in didAddContentSecurityPolicy one by one.

A CSP header can contain several policies as stated in RFC2616, section
4.2. It specifies that headers appearing multiple times can be combined
with a comma.

This CL makes FrameHostMsg_DidAddContentSecurityPolicy sends the
policies one by one, in contrast to grouped.

BUG=none
==========

to

==========
CSP send policies in didAddContentSecurityPolicy one by one.

A CSP header can contain several policies as stated in RFC2616, section
4.2. It specifies that headers appearing multiple times can be combined
with a comma.

This CL makes FrameHostMsg_DidAddContentSecurityPolicy sends the
policies one by one, in contrast to grouped.

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

[arthursonzogni, 2017-03-21 16:18:22]

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

[commit-bot: I haz the power, 2017-03-21 16:18:53]

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

[arthursonzogni, 2017-03-21 17:14:39]

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

[commit-bot: I haz the power, 2017-03-21 17:15:03]

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

[commit-bot: I haz the power, 2017-03-21 18:52:17]

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

[commit-bot: I haz the power, 2017-03-21 18:52:18]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-22 09:18:28]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org, mkwst@chromium.org

[arthursonzogni, 2017-03-22 09:18:28]

Hi Mike and Camille,

Please take a look. Thanks!

[arthursonzogni, 2017-03-22 09:18:47]

Patchset #1 (id:1) has been deleted

[Mike West, 2017-03-22 09:45:53]

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:330:
policy->exposeForNavigationalChecks());
It seems like doing the opposite might be more performant. That is, we're
awaiting a vector, so why not build a vector of the page's 15 policies and send
them up, rather than doing 15 IPC hops?

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:353: //
policies to be added.
I don't think this is necessary; we say something along the same lines when
parsing the policies in `addPolicyFromHeaderValue`. Here, I think we can simply
take it for granted that more than one policy might exist.

[arthursonzogni, 2017-03-22 10:27:09]

Thanks! In the next patch, I will merge all the IPCs into one.

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:330:
policy->exposeForNavigationalChecks());
On 2017/03/22 09:45:53, Mike West wrote:
> It seems like doing the opposite might be more performant. That is, we're
> awaiting a vector, so why not build a vector of the page's 15 policies and
send
> them up, rather than doing 15 IPC hops?

I agree, I did this CL in order to improve the simplicity of the code. It is a
sort of follow up of the comments I had in:
https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_...

If the performance is an issue, maybe we could go deeper in the opposite side as
you said.

several IPC(one header, several policies) [Current]
several IPC(one header, one policy)       [My CL]
one IPC(several header, several policies) [The suggestion]

In the next CL, I will try this.

https://codereview.chromium.org/2764993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:353: //
policies to be added.
On 2017/03/22 09:45:53, Mike West wrote:
> I don't think this is necessary; we say something along the same lines when
> parsing the policies in `addPolicyFromHeaderValue`. Here, I think we can
simply
> take it for granted that more than one policy might exist.

Okay, I will remove this comment.

[Mike West, 2017-03-22 11:58:05]

I'd like to see us reduce the number of IPCs. Will landing this CL get you
closer to that goal? It seems like you'd just be undoing the removal of the
`std::vector` parameter in the next patch...

[arthursonzogni, 2017-03-22 14:51:38]

Patchset #2 (id:40001) has been deleted

[arthursonzogni, 2017-03-22 15:02:20]

Description was changed from

==========
CSP send policies in didAddContentSecurityPolicy one by one.

A CSP header can contain several policies as stated in RFC2616, section
4.2. It specifies that headers appearing multiple times can be combined
with a comma.

This CL makes FrameHostMsg_DidAddContentSecurityPolicy sends the
policies one by one, in contrast to grouped.

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

to

==========
CSP: group policies in didAddContentSecurityPolicy.

This CL makes chrome sends one IPC in
ContentSecurityPolicy::reportAccumulatedHeaders instead
of one IPC per CSP headers.

In addition, in didAddContentSecurityPolicy, the CSP header
is no more sent since it is already included in the parsed
policy.

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

[arthursonzogni, 2017-03-22 15:05:42]

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

[commit-bot: I haz the power, 2017-03-22 15:06:13]

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

[arthursonzogni, 2017-03-22 15:24:36]

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

[commit-bot: I haz the power, 2017-03-22 15:24:59]

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

[arthursonzogni, 2017-03-22 15:27:15]

On 2017/03/22 11:58:05, Mike West wrote:
> I'd like to see us reduce the number of IPCs. Will landing this CL get you
> closer to that goal? It seems like you'd just be undoing the removal of the
> `std::vector` parameter in the next patch...

I wanted to do something a little bit different of undoing my 1st patch.

Please let me know what do you think of the latest patch. The number of IPC
is reduced as it will use only one IPC in
ContentSecurityPolicy::reportAccumulatedHeaders instead of one IPC per header.

In addition, it removes the |header| attribute in
FrameHostMsg_DidAddContentSecurityPolicy, because it is also present in the
parsed policy.

If we really want to reduce the number of IPC sent, we should also group the
headers in RenderFrameProxy::OnAddContentSecurityPolicy
This patch only reduce the number of IPC sent from the renderer
to the browser, but not on the other side.
Are you okay if I do it too?

[commit-bot: I haz the power, 2017-03-22 16:10:33]

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

[commit-bot: I haz the power, 2017-03-22 16:10:34]

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

[Mike West, 2017-03-23 08:43:07]

On 2017/03/22 at 15:27:15, arthursonzogni wrote:
> On 2017/03/22 11:58:05, Mike West wrote:
> > I'd like to see us reduce the number of IPCs. Will landing this CL get you
> > closer to that goal? It seems like you'd just be undoing the removal of the
> > `std::vector` parameter in the next patch...
> 
> I wanted to do something a little bit different of undoing my 1st patch.
> 
> Please let me know what do you think of the latest patch. The number of IPC
> is reduced as it will use only one IPC in
> ContentSecurityPolicy::reportAccumulatedHeaders instead of one IPC per header.

LGTM, with the exception of the `ContentSecurityPolicyPolicy` name, which is
terrible. :) It probably shouldn't change in this patch, but it needs to change.

> If we really want to reduce the number of IPC sent, we should also group the
> headers in RenderFrameProxy::OnAddContentSecurityPolicy
> This patch only reduce the number of IPC sent from the renderer
> to the browser, but not on the other side.
> Are you okay if I do it too?

Yup. :)

https://codereview.chromium.org/2764993002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFrameClientImpl.h (right):

https://codereview.chromium.org/2764993002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFrameClientImpl.h:191: const
blink::WebVector<WebContentSecurityPolicyPolicy>&) override;
Nit: Did we not change the name of this class in a previous patch?
`ContentSecurityPolicyPolicy` is a terrible name.

[arthursonzogni, 2017-03-23 09:00:40]

> > If we really want to reduce the number of IPC sent, we should also group the
> > headers in RenderFrameProxy::OnAddContentSecurityPolicy
> > This patch only reduce the number of IPC sent from the renderer
> > to the browser, but not on the other side.
> > Are you okay if I do it too?
> 
> Yup. :)
Okay, I will prepare another patch.

> 
>
https://codereview.chromium.org/2764993002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/LocalFrameClientImpl.h (right):
> 
>
https://codereview.chromium.org/2764993002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/LocalFrameClientImpl.h:191: const
> blink::WebVector<WebContentSecurityPolicyPolicy>&) override;
> Nit: Did we not change the name of this class in a previous patch?
> `ContentSecurityPolicyPolicy` is a terrible name.

Yes of course. I reassure you, you said it was a terrible name(and it is).
We started a conversation  we did not complete and that we probably
forgot.
```
> > Nit: ContentSecurityPolicyPolicy sounds super-weird. How about dropping the
last
> > `Policy`?
> Inside blink, a ContentSecurityPolicy doesn't represent a policy, but a set of
> policies (+ a ton of other things). So maybe dropping the Policy could cause
> confusion. The real CSPPolicy class in blink is the CSPDirectiveList.
> 
> I would like to abbreviate ContentSecurityPolicy into CSP. But I think it is
> against blink coding style.
> 
> So, yes maybe dropping the Policy if we don't have a better idea.
```
I have already started yesterday a patch that remove the last Policy. I will
send it soon.

[arthursonzogni, 2017-03-23 16:57:21]

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

[commit-bot: I haz the power, 2017-03-23 16:57: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, 2017-03-23 17:23:52]

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

[commit-bot: I haz the power, 2017-03-23 17:23:53]

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

[arthursonzogni, 2017-03-24 09:11:13]

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

[commit-bot: I haz the power, 2017-03-24 09:11:24]

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

[commit-bot: I haz the power, 2017-03-24 09:19:30]

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

[commit-bot: I haz the power, 2017-03-24 09:19:31]

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

[arthursonzogni, 2017-03-24 10:13:56]

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

[commit-bot: I haz the power, 2017-03-24 10:14:10]

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

[commit-bot: I haz the power, 2017-03-24 11:50:21]

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

[commit-bot: I haz the power, 2017-03-24 11:50:22]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-24 13:29:26]

Patchset #5 (id:120001) has been deleted

[arthursonzogni, 2017-03-24 13:33:36]

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

[arthursonzogni, 2017-03-24 13:33:37]

Hi Alex,

Please take a look. Thanks!
Can you check the content/* part?

It is a small refactoring of the ContentSecurityPolicy class and how the IPC are
sent. Instead of sending multiple IPC (one per header), it sends the whole set
at once.

[alexmos, 2017-03-25 01:46:27]

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

https://codereview.chromium.org/2764993002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1935:
headers.push_back(policy.header);
Sanity check: your old comment on RFHI::OnDidAddContentSecurityPolicy mentioned
that it was possible for one header to correspond to multiple policies (the
RFC2616 Sec 4.2 case).  Could this now result in duplicate headers being added
if multiple policies corresponded to the same comma-separated header line?  Or
does policy.header correspond to just one header in the case of comma-separated
headers?

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:354: const
std::vector<ContentSecurityPolicyHeader>& headers) {
Is anything stopping us now from sending over the real policies to the proxies,
instead of reparsing policies from the headers?  That would be a nice cleanup to
do in a followup.

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right):

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrameClient.h:280: // Called when a
new Content Security Policy is added to the frame's document.
nit: update this comment to say a set of new policies

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:353:
document()->frame()->client()->didAddContentSecurityPolicies(policies);
Presumably, |policies| should only contain the newly added policies, not all
policies?  I'm asking because |policies| is allocated with the size of all
policies, but only the new ones are initialized.  E.g., if we had 3 policies and
we're adding a new one here, it seems |policies| will have four entries with
only the fourth one initialized.  Does that need to be fixed?

[arthursonzogni, 2017-03-27 09:48:45]

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

[commit-bot: I haz the power, 2017-03-27 09:48:54]

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

[commit-bot: I haz the power, 2017-03-27 11:29:12]

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

[commit-bot: I haz the power, 2017-03-27 11:29:13]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-27 12:03:52]

Thanks!
Some answers below:

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

https://codereview.chromium.org/2764993002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1935:
headers.push_back(policy.header);
On 2017/03/25 01:46:27, alexmos wrote:
> Sanity check: your old comment on RFHI::OnDidAddContentSecurityPolicy
mentioned
> that it was possible for one header to correspond to multiple policies (the
> RFC2616 Sec 4.2 case).  Could this now result in duplicate headers being added
> if multiple policies corresponded to the same comma-separated header line?  Or
> does policy.header correspond to just one header in the case of
comma-separated
> headers?

The headers that contains multiple policies are split in
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp.
The ContentSecurityPolicy object contains exactly one header.

Before this patch, multiple IPC were sent, one per header and one header could
contain multiples policies. After this patch, one IPC is sent, it contains
multiples policies and a policy contains exactly header.

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:354: const
std::vector<ContentSecurityPolicyHeader>& headers) {
On 2017/03/25 01:46:27, alexmos wrote:
> Is anything stopping us now from sending over the real policies to the
proxies,
> instead of reparsing policies from the headers?  That would be a nice cleanup
to
> do in a followup. 

We need this when PlzNavigate is disabled and OOPIF enabled. The browser-side
enforcement of the CSP works only with PlzNavigate.

I think that as soon as PlzNavigate is shipped, we will be able to remove the
replication of the CSP among the remote frame. That would be a nice cleanup.
Alternatively, it would be possible to enable the browser-side enforcement of
the CSP when PlzNavigate is disabled. FYI, I am currently blocked on making this
patch:
https://codereview.chromium.org/2698623006/
working without PlzNavigate.

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right):

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrameClient.h:280: // Called when a
new Content Security Policy is added to the frame's document.
On 2017/03/25 01:46:27, alexmos wrote:
> nit: update this comment to say a set of new policies 

Done.

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2764993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:353:
document()->frame()->client()->didAddContentSecurityPolicies(policies);
On 2017/03/25 01:46:27, alexmos wrote:
> Presumably, |policies| should only contain the newly added policies, not all
> policies?  I'm asking because |policies| is allocated with the size of all
> policies, but only the new ones are initialized.  E.g., if we had 3 policies
and
> we're adding a new one here, it seems |policies| will have four entries with
> only the fourth one initialized.  Does that need to be fixed?

Yes, you are absolutely right. I made this mistake in one of the latest patch
when I switched from a std::vector to the WebVector. Thanks!

[alexmos, 2017-03-27 18:27:22]

LGTM, thanks!

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

https://codereview.chromium.org/2764993002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1935:
headers.push_back(policy.header);
On 2017/03/27 12:03:52, arthursonzogni wrote:
> On 2017/03/25 01:46:27, alexmos wrote:
> > Sanity check: your old comment on RFHI::OnDidAddContentSecurityPolicy
> mentioned
> > that it was possible for one header to correspond to multiple policies (the
> > RFC2616 Sec 4.2 case).  Could this now result in duplicate headers being
added
> > if multiple policies corresponded to the same comma-separated header line? 
Or
> > does policy.header correspond to just one header in the case of
> comma-separated
> > headers?
> 
> The headers that contains multiple policies are split in
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp.
> The ContentSecurityPolicy object contains exactly one header.
> 
> Before this patch, multiple IPC were sent, one per header and one header could
> contain multiples policies. After this patch, one IPC is sent, it contains
> multiples policies and a policy contains exactly header.

Acknowledged.

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:354: const
std::vector<ContentSecurityPolicyHeader>& headers) {
On 2017/03/27 12:03:52, arthursonzogni wrote:
> On 2017/03/25 01:46:27, alexmos wrote:
> > Is anything stopping us now from sending over the real policies to the
> proxies,
> > instead of reparsing policies from the headers?  That would be a nice
cleanup
> to
> > do in a followup. 
> 
> We need this when PlzNavigate is disabled and OOPIF enabled. The browser-side
> enforcement of the CSP works only with PlzNavigate.
> 
> I think that as soon as PlzNavigate is shipped, we will be able to remove the
> replication of the CSP among the remote frame. That would be a nice cleanup.
> Alternatively, it would be possible to enable the browser-side enforcement of
> the CSP when PlzNavigate is disabled. FYI, I am currently blocked on making
this
> patch:
> https://codereview.chromium.org/2698623006/
> working without PlzNavigate.
> 
> 

I agree we still need this for OOPIF without PlzNavigate, I was just thinking
whether FrameMsg_AddContentSecurityPolicies could pass not
ContentSecurityPolicyHeaders, but rather a vector of
content::ContentSecurityPolicy.  With the former, we end up reparsing the same
policy multiple times in each OOPIF renderer, and with the latter we'd avoid
that.  Back when we did the CSP header replication, we didn't have
content::ContentSecurityPolicy, but now we do.  But that'll probably involve
modifying FrameReplicationState as well, and you're probably right that it's
just easier to wait for PlzNavigate to ship and remove this altogether.

[arthursonzogni, 2017-03-28 08:15:59]

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

[commit-bot: I haz the power, 2017-03-28 08:16:22]

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

[commit-bot: I haz the power, 2017-03-28 09:56:50]

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

[commit-bot: I haz the power, 2017-03-28 09:56:51]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-28 10:57:34]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-03-28 10:57:34]

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/2764993002/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-28 10:57:44]

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

[commit-bot: I haz the power, 2017-03-28 11:09:54]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490698654449830,
"parent_rev": "281080243642ac41b1004f9d8c6363db6081bbfa", "commit_rev":
"662aa65677aabd2ee107e76a6928574cd167bc75"}

[commit-bot: I haz the power, 2017-03-28 11:11:19]

Description was changed from

==========
CSP: group policies in didAddContentSecurityPolicy.

This CL makes chrome sends one IPC in
ContentSecurityPolicy::reportAccumulatedHeaders instead
of one IPC per CSP headers.

In addition, in didAddContentSecurityPolicy, the CSP header
is no more sent since it is already included in the parsed
policy.

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

to

==========
CSP: group policies in didAddContentSecurityPolicy.

This CL makes chrome sends one IPC in
ContentSecurityPolicy::reportAccumulatedHeaders instead
of one IPC per CSP headers.

In addition, in didAddContentSecurityPolicy, the CSP header
is no more sent since it is already included in the parsed
policy.

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

Review-Url: https://codereview.chromium.org/2764993002
Cr-Commit-Position: refs/heads/master@{#460060}
Committed:
https://chromium.googlesource.com/chromium/src/+/662aa65677aabd2ee107e76a6928...
==========

[commit-bot: I haz the power, 2017-03-28 11:11:21]

Committed patchset #8 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/662aa65677aabd2ee107e76a6928...

[arthursonzogni, 2017-03-28 11:12:21]

Thanks for the reviews!

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/2764993002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:354: const
std::vector<ContentSecurityPolicyHeader>& headers) {
On 2017/03/27 18:27:22, alexmos wrote:
> On 2017/03/27 12:03:52, arthursonzogni wrote:
> > On 2017/03/25 01:46:27, alexmos wrote:
> > > Is anything stopping us now from sending over the real policies to the
> > proxies,
> > > instead of reparsing policies from the headers?  That would be a nice
> cleanup
> > to
> > > do in a followup. 
> > 
> > We need this when PlzNavigate is disabled and OOPIF enabled. The
browser-side
> > enforcement of the CSP works only with PlzNavigate.
> > 
> > I think that as soon as PlzNavigate is shipped, we will be able to remove
the
> > replication of the CSP among the remote frame. That would be a nice cleanup.
> > Alternatively, it would be possible to enable the browser-side enforcement
of
> > the CSP when PlzNavigate is disabled. FYI, I am currently blocked on making
> this
> > patch:
> > https://codereview.chromium.org/2698623006/
> > working without PlzNavigate.
> > 
> > 
> 
> I agree we still need this for OOPIF without PlzNavigate, I was just thinking
> whether FrameMsg_AddContentSecurityPolicies could pass not
> ContentSecurityPolicyHeaders, but rather a vector of
> content::ContentSecurityPolicy.  With the former, we end up reparsing the same
> policy multiple times in each OOPIF renderer, and with the latter we'd avoid
> that.  Back when we did the CSP header replication, we didn't have
> content::ContentSecurityPolicy, but now we do.  But that'll probably involve
> modifying FrameReplicationState as well, and you're probably right that it's
> just easier to wait for PlzNavigate to ship and remove this altogether.

Yes, I think it would be possible to convert back the
content::ContentSecurityPolicy to the blink::CSPDirectiveList if we wanted to,
such that it isn't need to parse the header again.
I don't know. Let's say we will do nothing for the moment and wait for the CSP
replication not to be needed anymore and removed.