Bug 234390 – CSP: Include the sample in eval violation reports

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are not including the sample for eval reports.

[Carlos Garcia Campos]

Created attachment 447353 [details]
Patch

[Kate Cheney]

Comment on attachment 447353 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:144
> +        auto codeScope = DECLARE_THROW_SCOPE(vm);

What is the benefit of creating codeScope and using that instead of scope?

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:147
> +        globalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(globalObject, !code.isNull() ? jsNontrivialString(vm, code) : nullptr);

I think this can be WTFMove(code) to avoid a potential copy.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:226
> +    void reportViolation(const String& effectiveViolatedDirective, const ContentSecurityPolicyDirective& violatedDirective, const String& blockedURL, const String& consoleMessage, JSC::JSGlobalObject*, const StringView& sourceContent) const;

Despite the fact that this is done in multiple places in CSP code, I think passing a StringView as a const reference is actually worse for performance, because it's in the same "complexity category" as int and char* and it's cost-of-copy does not outweigh the benefits of pass-by-value.

[Carlos Garcia Campos]

Comment on attachment 447353 [details]
Patch

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

>> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:144
>> +        auto codeScope = DECLARE_THROW_SCOPE(vm);
> 
> What is the benefit of creating codeScope and using that instead of scope?

I assumed we don't want to throw an exception in case something fails in stringifyFunction, since we don't even know if the sample report will be used at this point, and scope is already used to throw the createEvalError. I don't know what happens in throwException() if the given scope already has an exception thrown.

>> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:147
>> +        globalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(globalObject, !code.isNull() ? jsNontrivialString(vm, code) : nullptr);
> 
> I think this can be WTFMove(code) to avoid a potential copy.

Indeed.

>> Source/WebCore/page/csp/ContentSecurityPolicy.h:226
>> +    void reportViolation(const String& effectiveViolatedDirective, const ContentSecurityPolicyDirective& violatedDirective, const String& blockedURL, const String& consoleMessage, JSC::JSGlobalObject*, const StringView& sourceContent) const;
> 
> Despite the fact that this is done in multiple places in CSP code, I think passing a StringView as a const reference is actually worse for performance, because it's in the same "complexity category" as int and char* and it's cost-of-copy does not outweigh the benefits of pass-by-value.

Ok.

[Carlos Garcia Campos]

Created attachment 447593 [details]
Patch

[Carlos Garcia Campos]

Committed r287303 (245455@trunk): <https://commits.webkit.org/245455@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/86758157>