Bug 17108 – CSS parser should NOT return raw CSSValue* from creation functions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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(-)

[Darin Adler]

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.

[Eric Seidel (no email)]

r29910