[Webkit-unassigned] [Bug 234390] CSP: Include the sample in eval violation reports

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=234390

--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 447353
  --> https://bugs.webkit.org/attachment.cgi?id=447353
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211220/4c05ddcf/attachment.htm>