Web Inspector: [Styles] [CRASH] Handle rule addition and inline style editing failure due to Content-Security-Policy in the page

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 129699 [details]
Patch

[Pavel Feldman]

Comment on attachment 129699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129699&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:905
> +    // BUG80024: We may have failed to add an actual StyleSheet into the document due to CSP getting in our way.

CSP should not block native code from adding rule. Or you should build a way to override CSP.

[Alexander Pavlov (apavlov)]

Created attachment 129707 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129707 [details]
Patch

Attachment 129707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11770571

New failing tests:
http/tests/security/contentSecurityPolicy/inline-style-blocked.html

[Pavel Feldman]

Comment on attachment 129707 [details]
Patch

r- for regressing CSP

[Alexander Pavlov (apavlov)]

Created attachment 129893 [details]
[PATCH] Fixed CSP violation reporting for inline styles

[Pavel Feldman]

Comment on attachment 129893 [details]
[PATCH] Fixed CSP violation reporting for inline styles

View in context: https://bugs.webkit.org/attachment.cgi?id=129893&action=review

> Source/WebCore/dom/Document.cpp:2579
> +void Document::setAllowInlineStyle(bool value)

You should override content security policy in ContentSecurityPolicy.cpp.

> Source/WebCore/dom/StyledElement.cpp:-101
> -        else if (document()->contentSecurityPolicy()->allowInlineStyle())

Please do not change call sites.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:900
> +        bool inlineStyleEnabled = document->allowInlineStyle(false);

Should be:
ContentSecurityPolicy* policy = document->contentSecurityPolicy();
policy->setOverrideAllowInlineStyle(true);
targetNode->...
policy->setOverrideAllowInlineStyle(false);

> Source/WebCore/page/ContentSecurityPolicy.cpp:525
> +    CSPDirective* styleSrcDirective = operativeDirective(m_styleSrc.get());

drop this

> LayoutTests/inspector/styles/add-new-rule-inline-style-csp.html:13
> +    InspectorTest.selectNodeAndWaitForStyles("inspected", step1);

These are not 3 steps, these are 3 tests.

[Alexander Pavlov (apavlov)]

Created attachment 129903 [details]
[PATCH] Comments addressed

[Adam Barth]

Comment on attachment 129903 [details]
[PATCH] Comments addressed

The changes to ContentSecurityPolicy.cpp/h look fine.

[Pavel Feldman]

Comment on attachment 129903 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=129903&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:745
> +        *errorString = "Failed to find or create inspector stylesheet, probably due to Content-Security-Policy";

This is no longer the case.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1324
> +    ContentSecurityPolicy* policy = m_element->ownerDocument() ? m_element->ownerDocument()->contentSecurityPolicy() : 0;

ownerDocument never returns NULL for an element. Also, I would recommend using raii idiom for this case.

> Source/WebCore/page/ContentSecurityPolicy.h:73
> +    void setOverrideAllowInlineStyle(bool);

Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?

[Adam Barth]

> Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?

setOverrideAllowInlineStyle ?

[Alexander Pavlov (apavlov)]

(In reply to comment #11)
> > Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?
> 
> setOverrideAllowInlineStyle ?

Will revert to this name when landing.

[Alexander Pavlov (apavlov)]

Committed r109730: <http://trac.webkit.org/changeset/109730>