| Summary: | CSP: Include the sample in eval violation reports | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, katherine_cheney, keith_miller, mark.lam, mkwst, msaboff, pgriffis, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Carlos Garcia Campos
2021-12-16 07:22:04 PST
Created attachment 447353 [details]
Patch
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. 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. Created attachment 447593 [details]
Patch
Committed r287303 (245455@trunk): <https://commits.webkit.org/245455@trunk> |