Bug 155623 – Cleanup: Remove the need to pass reporting status to ContentSecurityPolicy functions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

ScriptController::initScript() is the only function that passes ContentSecurityPolicy::ReportingStatus::SuppressReport() following the removal of the SecurityPolicy script interface in <http://trac.webkit.org/changeset/197142> (bug #154694). We can refactor the logic in ScriptController::initScript() such that it delegates to the ContentSecurityPolicy object to enable/disable JavaScript eval()/operator eval in the newly initialized world.

[Daniel Bates]

Created attachment 274367 [details]
Patch

[Andy Estes]

Comment on attachment 274367 [details]
Patch

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

r=me

Do you anticipate adding more uses of SuppressReport in later patches, or is ContentSecurityPolicyDirectiveList::allowEval() the only function that will ever be passed ReportingStatus::SuppressReport? I'm wondering if you considered further simplifying this by removing the reportingStatus argument and associated branch from the other allow functions in ContentSecurityPolicyDirectiveList.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:44
> +class JSDOMWindowShell;

Should be listed after DOMStringList.

[Alex Christensen]

Comment on attachment 274367 [details]
Patch

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

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:97
> +    ASSERT(windowShell.window());
> +    ASSERT(windowShell.window()->scriptExecutionContext());
> +    ASSERT(windowShell.window()->scriptExecutionContext()->contentSecurityPolicy() == this);
> +    JSDOMWindow* window = windowShell.window();

It would have less redundancy if you put the assertions after the pointer declaration and used window in your assertions.

[Daniel Bates]

(In reply to comment #2)
> Do you anticipate adding more uses of SuppressReport in later patches

No.

> , or is ContentSecurityPolicyDirectiveList::allowEval() the only function that will
> ever be passed ReportingStatus::SuppressReport? 

It is the only function.

> I'm wondering if you considered further simplifying this by removing the
> reportingStatus argument and associated branch from the other allow functions in
> ContentSecurityPolicyDirectiveList.
> 

Yes, I plan to completely remove the ReportingStatus enum and it use throughout ContentSecurityPolicyDirectiveList. This patch is the first step.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:44
> > +class JSDOMWindowShell;
> 
> Should be listed after DOMStringList.

Will fix before landing.

[Daniel Bates]

(In reply to comment #3)
> Comment on attachment 274367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274367&action=review
> 
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:97
> > +    ASSERT(windowShell.window());
> > +    ASSERT(windowShell.window()->scriptExecutionContext());
> > +    ASSERT(windowShell.window()->scriptExecutionContext()->contentSecurityPolicy() == this);
> > +    JSDOMWindow* window = windowShell.window();
> 
> It would have less redundancy if you put the assertions after the pointer
> declaration and used window in your assertions.

Will fix before landing.

[Daniel Bates]

Committed r198379: <http://trac.webkit.org/changeset/198379>