CSP: Check <param> element values against the document's CSP before loading.

CSP: Check <param> element values against the document's CSP before loading.

We ought to take account of the 'param' element parsing behavior that happens in
'HTMLObjectElement'. This patch moves the pluginIsLoadable check to make that
happen.

To avoid 'setTimeout' in the test, and to align with the spec[1], this patch also
starts dispatching an 'error' event on load failure for 'object' elements.

[1]: #4.6 ("If the load failed...") of http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element

BUG=320796
R=tsepez@chromium.org,abarth@chromium.org

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

[Mike West, 2013-11-19 14:45:48]

Tom, mind taking a look?

-mike

[abarth-chromium, 2013-11-19 17:22:07]

LGTM assuming the test isn't flaky.

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked-expected.txt
(right):

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked-expected.txt:13:
This test passes if there is are four console messages saying the plugins were
blocked.
Do these messages arrive in a predictable order or is this test going to be
flaky?

[Tom Sepez, 2013-11-19 17:54:22]

LGTM with comments.

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html
(right):

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html:15:
var names = ['src', 'movie', 'code', 'url'];
nit: maybe a link to the section of the spec that calls out these 4 names.

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html:27:

Can we also use an onload handler to keep track of failures?  If we break this
down the road, having a text diff vs a timeout when the expected number of
handlers don't fire ...

https://codereview.chromium.org/76303002/diff/30001/Source/core/html/HTMLObje...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/76303002/diff/30001/Source/core/html/HTMLObje...
Source/core/html/HTMLObjectElement.cpp:315: if (!success) {
nit: not sure having the local |success| buys us anything over if
(!beforeLoadAllowed || !...)

[Mike West, 2013-11-19 18:35:05]

Thank you both. I'll fix this up tomorrow and CQ it.

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked-expected.txt
(right):

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked-expected.txt:13:
This test passes if there is are four console messages saying the plugins were
blocked.
On 2013/11/19 17:22:08, abarth wrote:
> Do these messages arrive in a predictable order or is this test going to be
> flaky?

I'm fairly sure they're predictable (e.g. they don't deal with network requests,
so they happen along with parsing).

That said, you're right that we should not rely on the events always dispatching
in the same order; I'll split this into multiple tests

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
File
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html
(right):

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html:15:
var names = ['src', 'movie', 'code', 'url'];
On 2013/11/19 17:54:22, Tom Sepez wrote:
> nit: maybe a link to the section of the spec that calls out these 4 names.

This is non-standard behavior, based (I think) on something IE did/does.

There's a comment (like, literally an HTML comment) in the spec that notes "we
may have to define magic fallback to <param> if it turns out to be needed in
testing", and quotes Hyatt from IRC.

https://codereview.chromium.org/76303002/diff/30001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/object-src-param-blocked.html:27:

On 2013/11/19 17:54:22, Tom Sepez wrote:
> Can we also use an onload handler to keep track of failures?  If we break this
> down the road, having a text diff vs a timeout when the expected number of
> handlers don't fire ...

Sure. It's probably worth rewriting all the object-src tests in that pattern.

Of course, it would require implementing the 'load' event, which I don't think
exists at the moment either. I'd like to defer that to the next CL if at all
possible. :)

https://codereview.chromium.org/76303002/diff/30001/Source/core/html/HTMLObje...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/76303002/diff/30001/Source/core/html/HTMLObje...
Source/core/html/HTMLObjectElement.cpp:315: if (!success) {
On 2013/11/19 17:54:22, Tom Sepez wrote:
> nit: not sure having the local |success| buys us anything over if
> (!beforeLoadAllowed || !...)

Yeah, not anymore. I'll fold those in.

[commit-bot: I haz the power, 2013-11-20 09:50:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/76303002/130001

[commit-bot: I haz the power, 2013-11-20 11:14:02]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-20 12:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/76303002/280001

[commit-bot: I haz the power, 2013-11-20 14:00:27]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-11-20 14:19:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/76303002/280001

[commit-bot: I haz the power, 2013-11-20 16:48:05]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-13 08:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/76303002/470001

[commit-bot: I haz the power, 2014-01-13 09:07:59]

Change committed as 164952