Bug 111249 - Remove two unnecessary mallocs from the main-thread-parser code path
Summary: Remove two unnecessary mallocs from the main-thread-parser code path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 111264 106127
  Show dependency treegraph
 
Reported: 2013-03-02 02:34 PST by Eric Seidel (no email)
Modified: 2013-03-03 01:04 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.31 KB, patch)
2013-03-02 02:52 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2013-03-02 02:34:01 PST
Remove two unnecessary mallocs from the main-thread-parser code path
Comment 1 Eric Seidel (no email) 2013-03-02 02:52:06 PST
Created attachment 191104 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-03-02 02:53:44 PST
It actually ended up being 3 unnecessary mallocs, one of which affects both code paths.  I also killed nameString() since it was just getting us in trouble.
Comment 3 Adam Barth 2013-03-02 10:02:28 PST
Comment on attachment 191104 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditor.cpp:116
> +// If other files need this, we should move this to HTMLParserIdioms.h

We should move it to HTMLParserIdioms.h anyway since that's where the the other threadSafeMatch function is.
Comment 4 Adam Barth 2013-03-02 10:04:11 PST
You might also be interested in the mallocs in XSSAuditor::eraseDangerousAttributesIfInjected:

        bool valueContainsJavaScriptURL = !isInlineEventHandler && protocolIsJavaScript(stripLeadingAndTrailingHTMLSpaces(String(attribute.value)));

We shouldn't need to call malloc at all on that line, but we can end up calling it twice.
Comment 5 Eric Seidel (no email) 2013-03-02 12:52:22 PST
(In reply to comment #3)
> (From update of attachment 191104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191104&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:116
> > +// If other files need this, we should move this to HTMLParserIdioms.h
> 
> We should move it to HTMLParserIdioms.h anyway since that's where the the other threadSafeMatch function is.

The only problem with that is that then it would add QualifiedName.h to that header (because it's a template).  I think we should hold off for now.
Comment 6 Eric Seidel (no email) 2013-03-02 12:52:39 PST
(In reply to comment #4)
> You might also be interested in the mallocs in XSSAuditor::eraseDangerousAttributesIfInjected:
> 
>         bool valueContainsJavaScriptURL = !isInlineEventHandler && protocolIsJavaScript(stripLeadingAndTrailingHTMLSpaces(String(attribute.value)));
> 
> We shouldn't need to call malloc at all on that line, but we can end up calling it twice.

Yeah, I'll take a look at the XSS auditor in more detail soon.
Comment 7 Eric Seidel (no email) 2013-03-02 12:53:22 PST
I'm happy to move the function if you feel strongly (I originally put it there, but moved it here to avoid the #include "QualifiedName.h")
Comment 8 WebKit Review Bot 2013-03-02 13:08:57 PST
Comment on attachment 191104 [details]
Patch

Clearing flags on attachment: 191104

Committed r144544: <http://trac.webkit.org/changeset/144544>
Comment 9 WebKit Review Bot 2013-03-02 13:09:01 PST
All reviewed patches have been landed.  Closing bug.