Bug 17108 - CSS parser should NOT return raw CSSValue* from creation functions
Summary: CSS parser should NOT return raw CSSValue* from creation functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-30 18:13 PST by Eric Seidel (no email)
Modified: 2008-02-01 00:02 PST (History)
1 user (show)

See Also:


Attachments
Beat CSSParser with the RefPtr stick (37.24 KB, patch)
2008-01-31 00:01 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Beat CSSParser with the RefPtr stick (41.25 KB, patch)
2008-01-31 00:38 PST, Eric Seidel (no email)
darin: review+
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) 2008-01-30 18:13:46 PST
CSS parser should NOT return raw CSSValue* from creation functions

Instead it should use PassRefPtr<CSSValue> for single returns, and use RefPtr<CSSValue>& for multiple return values.

I expect we have leaks in this code and just don't know it yet.
Comment 1 Eric Seidel (no email) 2008-01-31 00:01:31 PST
Created attachment 18812 [details]
Beat CSSParser with the RefPtr stick

 WebCore/ChangeLog            |   29 +++++
 WebCore/css/CSSParser.cpp    |  287 +++++++++++++++++++----------------------
 WebCore/css/CSSParser.h      |   38 +++---
 WebCore/css/SVGCSSParser.cpp |   17 +--
 4 files changed, 188 insertions(+), 183 deletions(-)
Comment 2 Eric Seidel (no email) 2008-01-31 00:37:22 PST
Comment on attachment 18812 [details]
Beat CSSParser with the RefPtr stick

Actually this one had one bug.  I've fixed it.  Uploading a new patch.
Comment 3 Eric Seidel (no email) 2008-01-31 00:38:07 PST
Created attachment 18813 [details]
Beat CSSParser with the RefPtr stick

 WebCore/ChangeLog                          |   29 +++
 WebCore/css/CSSMutableStyleDeclaration.cpp |    5 +-
 WebCore/css/CSSParser.cpp                  |  313 +++++++++++++---------------
 WebCore/css/CSSParser.h                    |   38 ++--
 WebCore/css/SVGCSSParser.cpp               |   17 +-
 5 files changed, 205 insertions(+), 197 deletions(-)
Comment 4 Darin Adler 2008-01-31 10:19:00 PST
Comment on attachment 18813 [details]
Beat CSSParser with the RefPtr stick

 1097                     values->append(parsedValue.get()); // FIXME: why woule one ever append the same value twice?

Typo "woule".

I also think that this comment should be more clear on the point. Something along the lines of, "If we know this could never be done with the same value twice, we could change this to release() and get rid of a little bit of reference count churn."

And I think the comment should go on its own line. The way to do that is to change the code to early-break if parsedValue is 0 instead of having an else.

Same below for values->length(). It should be an early exit that calls return false.

 1138         PassRefPtr<Pair> pair = new Pair(parsedValue1.release(), parsedValue2.release());
 1139         RefPtr<CSSPrimitiveValue> val = new CSSPrimitiveValue(pair);

How about just doing this all on one line without the local variable? Or doing it with a RefPtr and a call to release(). It seems a little ugly to have a local variable of type PassRefPtr.

 1585     Vector<RefPtr<CSSValue>, numProperties> values(numProperties);

Why not

     RefPtr<CSSValue> values[numProperties];

here? I don't think a Vector is better than an array here.

I think CSSParser::parseSVGValue would look better with early exits too.

I'll say r=me, although I'm not so happy with the Vector.
Comment 5 Eric Seidel (no email) 2008-02-01 00:02:30 PST
r29910