Bug 55156 - PatternAlternative leaked in YarrPatternConstructor::atomParenthesesEnd()
Summary: PatternAlternative leaked in YarrPatternConstructor::atomParenthesesEnd()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-24 09:23 PST by Michael Saboff
Modified: 2011-02-24 15:59 PST (History)
3 users (show)

See Also:

Attachments
Patch to delete unneeded PatternAlternative (1.46 KB, patch)
2011-02-24 11:06 PST, Michael Saboff 		oliver: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-02-24 09:23:58 PST 
The removal of empty disjunction alternatives from the alternatives vector doesn't delete the removed alternative thus leaking it.
Comment 1 Michael Saboff 2011-02-24 11:06:30 PST 
Created attachment 83682 [details]
Patch to delete unneeded PatternAlternative
Comment 2 Oliver Hunt 2011-02-24 11:19:23 PST 
Comment on attachment 83682 [details]
Patch to delete unneeded PatternAlternative

r=me
Comment 3 Michael Saboff 2011-02-24 11:25:56 PST 
Committed r79594: <http://trac.webkit.org/changeset/79594>
Comment 4 Darin Adler 2011-02-24 11:58:14 PST 
Comment on attachment 83682 [details]
Patch to delete unneeded PatternAlternative

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

> Source/JavaScriptCore/yarr/YarrPattern.cpp:500
> +                PatternAlternative* altToRemove = parenthesesDisjunction->m_alternatives[i];
>                  parenthesesDisjunction->m_alternatives.remove(i);
> +                delete altToRemove;

A couple post-patch-lading thoughts:

I would prefer to do this with OwnPtr instead of an explicit delete. Maybe m_alternatives could actually use OwnPtr.

I would also have called it alternativeToRemove instead of altToRemove.
Comment 5 WebKit Review Bot 2011-02-24 15:59:25 PST 
http://trac.webkit.org/changeset/79594 might have broken GTK Linux 32-bit Release