Closed Bug 1376651 Opened 7 years ago Closed 7 years ago

mozilla::dom::CSPAllowsInlineScript shows malloc/free churn on Facebook

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact low
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: kmckinley)

References

Details

(Keywords: perf, top100, Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

Bug 1376651 - Pass the nsIScriptElement instead of allocating a string every time
59 bytes, text/x-review-board-request
ckerschb
: review+
Details 
The performance profile in bug 1375346 comment 2 shows heap allocation
in mozilla::dom::CSPAllowsInlineScript is slow.

I'm *guessing* it's from copying the script text in a temporary
variable here:
http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/script/ScriptLoader.cpp#1115-1116

Can we avoid doing that somehow?

It seems nsCSPContext::GetAllowsInline doesn't even use it some cases:
http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/security/nsCSPContext.cpp#508,528,540
For example if either of these two conditions are true:
http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/security/nsCSPContext.cpp#526-527
Keywords: top100
Whiteboard: [qf] → [qf:p3]
Here's a link to the profile showing this function being hit (2 samples): https://perfht.ml/2u1EamL
I see two possible solutions:
a) We add something like GetScriptSnippet() on the scriptEelement, because after all we call GetScriptText() and then only care about the first 40 characters of the script.
b) Alternatively we could pass the scriptElement pointer around and only query the script text if needed.

How important is it to get that fixed?
[qf:p3] means this isn't blocker-level, but it's a definite performance "want".  We're trying to avoid n

Both of the strategies you mentioned sound nice to me! (And they don't sound too complicated to implement, which makes this more attractive as a bug-we-can-and-should-fix.)  Depending on how common it is that we need the script snippet, maybe it'd be worth doing both?
(In reply to Daniel Holbert [:dholbert] from comment #3)
>  We're trying to avoid n
...needless allocation churn, where we can.
(In reply to Daniel Holbert [:dholbert] from comment #4)
> (In reply to Daniel Holbert [:dholbert] from comment #3)
> >  We're trying to avoid n
> ...needless allocation churn, where we can.

Makes sense. Daniel, do we have any timeline when we want that landed? I'll try to find some resources to get that done, seems to align with our overall goals.
Flags: needinfo?(dholbert)
Early August would be great, if possible (to land in the Nightly 57 timeframe).
Flags: needinfo?(dholbert)
Wennie, this really needs to be fixed for 57, can we assign it to someone? Maybe Kate?
Flags: needinfo?(wleung)
Priority: P3 → P2
Whiteboard: [qf:p3] → [qf:p3][domsecurity-backlog1]
Hi Kate, please take a look at this.
Flags: needinfo?(wleung) → needinfo?(kmckinley)
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
Flags: needinfo?(kmckinley)
Comment on attachment 8892628 [details]
Bug 1376651 - Pass the nsIScriptElement instead of allocating a string every time

https://reviewboard.mozilla.org/r/163614/#review169278

Kate, this seems like the right approach to me. Please answer my questions and flag me for review again - thanks! Btw, do all CSP tests still pass? In particular test/csp_report.html?

::: dom/interfaces/security/nsIContentSecurityPolicy.idl:154
(Diff revision 1)
>     *     (block the rules if false).
>     */
>    boolean getAllowsInline(in nsContentPolicyType aContentPolicyType,
>                            in AString aNonce,
>                            in boolean aParserCreated,
> -                          in AString aContent,
> +                          in nsISupports aElement,

nit: maybe we should call that argument aElementOrContent (which would also clear up information why it's an nsISupports)

::: dom/security/nsCSPContext.cpp:532
(Diff revision 1)
>    for (uint32_t i = 0; i < mPolicies.Length(); i++) {
>      bool allowed =
>        mPolicies[i]->allows(aContentType, CSP_UNSAFE_INLINE, EmptyString(), aParserCreated) ||
> -      mPolicies[i]->allows(aContentType, CSP_NONCE, aNonce, aParserCreated) ||
> -      mPolicies[i]->allows(aContentType, CSP_HASH, aContent, aParserCreated);
> +      mPolicies[i]->allows(aContentType, CSP_NONCE, aNonce, aParserCreated);
> +
> +    if (allowed) {

this definitely lacks a comment explaining why we are doing 2 allows calls up here and 1 further down.

::: dom/security/nsCSPContext.cpp:536
(Diff revision 1)
> -      mPolicies[i]->allows(aContentType, CSP_HASH, aContent, aParserCreated);
> +
> +    if (allowed) {
> +      continue;
> +    }
> +
> +    if (content.Length() > 0) {

maybe I am just missing something, but can you explain why we need |if(content.Length() > 0)? If so, please add a comment.

It seems like in the very first iteration the length will always be 0, right? Assuming there will be only one policy, then content will not ever have a length > 0. Unless I am missing something.

::: layout/style/nsStyleUtil.cpp:857
(Diff revision 1)
>  
> +  nsCOMPtr<nsISupportsString> styleText(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
> +  if (styleText) {
> +    styleText->SetData(aStyleText);
> +  }
> +  //nsCOMPtr<nsISupports> sampleISupports(do_QueryInterface(styleText));

please remove that line if not needed.
Attachment #8892628 - Flags: review?(ckerschb)
Attachment #8892628 - Flags: review?(ckerschb)
Comment on attachment 8892628 [details]
Bug 1376651 - Pass the nsIScriptElement instead of allocating a string every time

https://reviewboard.mozilla.org/r/163614/#review169930

::: dom/security/nsCSPContext.cpp:539
(Diff revision 4)
> +    // unecessary strings
> +    if (allowed) {
> +      continue;
> +    }
> +
> +    if (content.Length() == 0) {

Kate, sorry for the push back here again, but I still don't fully understand why we need to check content.length() here? Please add a comment to explain why it is needed and what happens here - thanks. Other than that, the patch seems good to go
Attachment #8892628 - Flags: review?(ckerschb)
Comment on attachment 8892628 [details]
Bug 1376651 - Pass the nsIScriptElement instead of allocating a string every time

https://reviewboard.mozilla.org/r/163614/#review170260

looks good to me. Please just fix my nit and this is ready to go. r=me

::: dom/interfaces/security/nsIContentSecurityPolicy.idl:143
(Diff revision 5)
>     * Whether this policy allows inline script or style.
>     * @param aContentPolicyType Either TYPE_SCRIPT or TYPE_STYLESHEET
>     * @param aNonce The nonce string to check against the policy
>     * @param aParserCreated If the script element was created by the HTML Parser
> -   * @param aContent  The content of the inline resource to hash
> +   * @param aElement The script element of the inline resource to hash or the
> +   *        content of the psuedo-script to compare to hash

please rename to aElementOrContent here as well, like you did in the *.cpp files.
Attachment #8892628 - Flags: review?(ckerschb) → review+
Pushed by kmckinley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f39e4fb4ff1
Pass the nsIScriptElement instead of allocating a string every time r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/2f39e4fb4ff1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
https://hg.mozilla.org/projects/date/rev/2f39e4fb4ff15c395d6e3af74843070e17933e89
Bug 1376651 - Pass the nsIScriptElement instead of allocating a string every time r=ckerschb
Performance Impact: --- → P3
Whiteboard: [qf:p3][domsecurity-backlog1] → [domsecurity-backlog1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: