CSP: Move policy parsing out of Document.

CSP: Move policy parsing out of Document.

This patch finishes the process of allowing policy parsing outside of
an ExecutionContext, and adjusts Document::initContentSecurityPolicy
to accept a ContentSecurityPolicy object rather than a set of headers.
If such an object is provided, the document will take ownership of it
and use it going forward. If no such object is provided, an empty
policy will be created and stored.

A future patch will move parsing out of FrameLoader::didBeginDocument
into DocumentLoader::responseReceived so that we can begin doing some
checks before a document is created.

BUG=411889
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181811

[Mike West, 2014-09-10 08:31:55]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, sigbjornf@opera.com

[Mike West, 2014-09-10 08:33:33]

Jochen, would you mind taking a look at this?

Sigbjorn, FYI: this is decouples CSP parsing from an execution context, which
might make some of the redirect checking possible sooner rather than later.
Still probably isn't worth poking at until after the repositories merge, but
we're getting closer!

[Mike West, 2014-09-10 11:51:40]

mkwst@chromium.org changed reviewers:
+ philipj@opera.com

[sof, 2014-09-11 07:44:04]

Sorry about the delay. The re-casting looks fine, but got one clarifying
question regarding Document::initSecurityContext().

https://codereview.chromium.org/559503002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4819: // Otherwise, the CSP object will be
initialized in 'initContentSecurityPolicy'.
Where do you ensure that it is being called if not an HTML import? The previous
version made it locally clear that the policy was always set; now I'm not sure.

https://codereview.chromium.org/559503002/diff/40001/Source/core/frame/csp/Co...
File Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.h:189: explicit
ContentSecurityPolicy();
nit: drop explicit.

https://codereview.chromium.org/559503002/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:108:
setContentSecurityPolicy(ContentSecurityPolicy::create());
(Nothing wrong with this, but I like the style added in
FrameLoader::didBeginDocument(), which separately builds up the CSP object and
then assigns it in the end.)

[Mike West, 2014-09-11 08:29:13]

Thanks for taking a look.

https://codereview.chromium.org/559503002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4819: // Otherwise, the CSP object will be
initialized in 'initContentSecurityPolicy'.
On 2014/09/11 07:44:04, sof wrote:
> Where do you ensure that it is being called if not an HTML import? The
previous
> version made it locally clear that the policy was always set; now I'm not
sure.

Hrm. What would you like to see? `initContentSecurityPolicy` is called from a
few places (see the rest of this patch); it's not clear where I could ASSERT
anything meaningful.

I could certainly create an empty policy object here, and throw it away when
`initContentSecurityPolicy` is called. That's fairly low overhead, but seems a
bit wasteful.

https://codereview.chromium.org/559503002/diff/40001/Source/core/frame/csp/Co...
File Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.h:189: explicit
ContentSecurityPolicy();
On 2014/09/11 07:44:04, sof wrote:
> nit: drop explicit.

The bane of my existence. :)

https://codereview.chromium.org/559503002/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/559503002/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:108:
setContentSecurityPolicy(ContentSecurityPolicy::create());
On 2014/09/11 07:44:04, sof wrote:
> (Nothing wrong with this, but I like the style added in
> FrameLoader::didBeginDocument(), which separately builds up the CSP object and
> then assigns it in the end.)

I'll rework it, no worries.

[Mike West, 2014-09-11 08:38:03]

Took another pass. I don't like calling `initContentSecurityPolicy` there when
I'm pretty sure it's going to be overwritten later, but I agree with you that
ensuing that a policy object is always available is a reasonable goal.

WDYT?

[sof, 2014-09-11 08:45:59]

On 2014/09/11 08:38:03, Mike West wrote:
> Took another pass. I don't like calling `initContentSecurityPolicy` there when
> I'm pretty sure it's going to be overwritten later, but I agree with you that
> ensuing that a policy object is always available is a reasonable goal.
> 
> WDYT?

lgtm.

If it is clear that initSecurityContext() callers will inevitably always fall
into a initContentSecurityPolicy() call, then I agree this is an extra call not
worth having. That was the clarification I was looking for - if you think that
holds, then just remove.

[Mike West, 2014-09-11 08:56:03]

On 2014/09/11 08:45:59, sof wrote:
> On 2014/09/11 08:38:03, Mike West wrote:
> > Took another pass. I don't like calling `initContentSecurityPolicy` there
when
> > I'm pretty sure it's going to be overwritten later, but I agree with you
that
> > ensuing that a policy object is always available is a reasonable goal.
> > 
> > WDYT?
> 
> lgtm.
> 
> If it is clear that initSecurityContext() callers will inevitably always fall
> into a initContentSecurityPolicy() call, then I agree this is an extra call
not
> worth having. That was the clarification I was looking for - if you think that
> holds, then just remove.

*shrug* I'll leave it in for the moment. Perhaps we can find a cleaner way to do
it in the future, but this is minimal overhead, and provides 100% certainty. So
that's good. :)

[Mike West, 2014-09-11 08:56:07]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-09-11 08:57:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559503002/60001

[commit-bot: I haz the power, 2014-09-11 10:42:35]

Committed patchset #4 (id:60001) as 181811