Clarify/fix {Shadow,BorderImage}ParseContext's memory management
authoraroben <aroben@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Jul 2007 05:48:00 +0000 (05:48 +0000)
committeraroben <aroben@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Jul 2007 05:48:00 +0000 (05:48 +0000)
Prefast emitted warnings that drew my attention to
{Shadow,BorderImage}ParseContext::failed().  It turned out that these
methods were actually correct, but rather confusing. "failed" really
meant "abort and clean up" rather than "did you fail?", which was
unclear. However, once I figured that out, the "and clean up" part was
still a bit confusing, because all failed() did was to set a flag that
would later cause the ParseContext's members to be deleted in the
destructor. To clear this up, I've gotten rid of the failed() method
altogether. It always returned false, so I've replaced all calls to
it with the value false.

I also noticed that the lifetime management of the ParseContexts'
members was in all cases confusing, and in some cases wrong. The
m_border{Top,Right,Bottom,Left} members of BorderImageParseContext
were leaked whenever a border-image property was successfully parsed.
I fixed that by holding these members in OwnPtrs. The
CSSPrimitiveValue members of {Shadow,BorderImage}ParseContext, which
inherit from Shared, were being explicitly deleted, which is not a
safe way to manage the lifetime of objects that inherit from Shared.
To fix this, I put those members inside RefPtrs. These two changes
allowed me to remove the destructors entirely.

Reviewed by Darin.

All regression tests pass.

* css/cssparser.cpp:
(WebCore::ShadowParseContext::commitValue): Use .release() to avoid
ref-count churn.
(WebCore::ShadowParseContext::commitLength): Use a RefPtr for the new value to
avoid a leak.
(WebCore::CSSParser::parseShadow): Use 'false' instead of
'context.failed()', and use .release() to avoid ref-count churn.
(WebCore::BorderImageParseContext::commitWidth): Updated to use
OwnPtr.
(WebCore::CSSParser::parseBorderImage): Use 'false' instead of
'context.failed'.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@23925 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/css/cssparser.cpp

index ea10dd81abc2d38a83f2f547a8fb2e29ead4186d..72ab74ed956afb34245973587f534da8b86faf93 100644 (file)
@@ -1,3 +1,45 @@
+2007-07-01  Adam Roben  <aroben@apple.com>
+
+        Clarify/fix {Shadow,BorderImage}ParseContext's memory management
+
+        Prefast emitted warnings that drew my attention to
+        {Shadow,BorderImage}ParseContext::failed().  It turned out that these
+        methods were actually correct, but rather confusing. "failed" really
+        meant "abort and clean up" rather than "did you fail?", which was
+        unclear. However, once I figured that out, the "and clean up" part was
+        still a bit confusing, because all failed() did was to set a flag that
+        would later cause the ParseContext's members to be deleted in the
+        destructor. To clear this up, I've gotten rid of the failed() method
+        altogether. It always returned false, so I've replaced all calls to
+        it with the value false.
+
+        I also noticed that the lifetime management of the ParseContexts'
+        members was in all cases confusing, and in some cases wrong. The
+        m_border{Top,Right,Bottom,Left} members of BorderImageParseContext
+        were leaked whenever a border-image property was successfully parsed.
+        I fixed that by holding these members in OwnPtrs. The
+        CSSPrimitiveValue members of {Shadow,BorderImage}ParseContext, which
+        inherit from Shared, were being explicitly deleted, which is not a
+        safe way to manage the lifetime of objects that inherit from Shared.
+        To fix this, I put those members inside RefPtrs. These two changes
+        allowed me to remove the destructors entirely.
+
+        Reviewed by Darin.
+
+        All regression tests pass.
+
+        * css/cssparser.cpp:
+        (WebCore::ShadowParseContext::commitValue): Use .release() to avoid
+        ref-count churn.
+        (WebCore::ShadowParseContext::commitLength): Use a RefPtr for the new
+        value to avoid a leak.
+        (WebCore::CSSParser::parseShadow): Use 'false' instead of
+        'context.failed()', and use .release() to avoid ref-count churn.
+        (WebCore::BorderImageParseContext::commitWidth): Updated to use
+        OwnPtr.
+        (WebCore::CSSParser::parseBorderImage): Use 'false' instead of
+        'context.failed'.
+
 2007-07-01  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by John Sullivan.
index de2d832e7530c80b98ba7c46b452f7754cdf143b..d16020694dc351dad76db9a8b2f893be14450305 100644 (file)
@@ -2609,20 +2609,8 @@ struct ShadowParseContext {
      allowBreak(true)
     {}
 
-    ~ShadowParseContext() {
-        if (!allowBreak) {
-            delete values;
-            delete x;
-            delete y;
-            delete blur;
-            delete color;
-        }
-    }
-
     bool allowLength() { return allowX || allowY || allowBlur; }
 
-    bool failed() { return allowBreak = false; }
-    
     void commitValue() {
         // Handle the ,, case gracefully by doing nothing.
         if (x || y || blur || color) {
@@ -2630,7 +2618,7 @@ struct ShadowParseContext {
                 values = new CSSValueList();
             
             // Construct the current shadow value and add it to the list.
-            values->append(new ShadowValue(x, y, blur, color));
+            values->append(new ShadowValue(x.release(), y.release(), blur.release(), color.release()));
         }
         
         // Now reset for the next shadow value.
@@ -2640,18 +2628,18 @@ struct ShadowParseContext {
     }
 
     void commitLength(Value* v) {
-        CSSPrimitiveValue* val = new CSSPrimitiveValue(v->fValue,
-                                                               (CSSPrimitiveValue::UnitTypes)v->unit);
+        RefPtr<CSSPrimitiveValue> val = new CSSPrimitiveValue(v->fValue, (CSSPrimitiveValue::UnitTypes)v->unit);
+
         if (allowX) {
-            x = val;
+            x = val.release();
             allowX = false; allowY = true; allowColor = false; allowBreak = false;
         }
         else if (allowY) {
-            y = val;
+            y = val.release();
             allowY = false; allowBlur = true; allowColor = true; allowBreak = true;
         }
         else if (allowBlur) {
-            blur = val;
+            blur = val.release();
             allowBlur = false;
         }
     }
@@ -2665,11 +2653,11 @@ struct ShadowParseContext {
             allowBlur = false;
     }
     
-    CSSValueList* values;
-    CSSPrimitiveValue* x;
-    CSSPrimitiveValue* y;
-    CSSPrimitiveValue* blur;
-    CSSPrimitiveValue* color;
+    RefPtr<CSSValueList> values;
+    RefPtr<CSSPrimitiveValue> x;
+    RefPtr<CSSPrimitiveValue> y;
+    RefPtr<CSSPrimitiveValue> blur;
+    RefPtr<CSSPrimitiveValue> color;
 
     bool allowX;
     bool allowY;
@@ -2688,7 +2676,7 @@ bool CSSParser::parseShadow(int propId, bool important)
             if (val->iValue != ',' || !context.allowBreak)
                 // Other operators aren't legal or we aren't done with the current shadow
                 // value.  Treat as invalid.
-                return context.failed();
+                return false;
             
             // The value is good.  Commit it.
             context.commitValue();
@@ -2697,7 +2685,7 @@ bool CSSParser::parseShadow(int propId, bool important)
         else if (validUnit(val, FLength, true)) {
             // We required a length and didn't get one. Invalid.
             if (!context.allowLength())
-                return context.failed();
+                return false;
 
             // A length is allowed here.  Construct the value and add it.
             context.commitLength(val);
@@ -2709,7 +2697,7 @@ bool CSSParser::parseShadow(int propId, bool important)
                             (val->id >= CSS_VAL__WEBKIT_FOCUS_RING_COLOR && val->id <= CSS_VAL__WEBKIT_TEXT && !strict));
             if (isColor) {
                 if (!context.allowColor)
-                    return context.failed();
+                    return false;
                 parsedColor = new CSSPrimitiveValue(val->id);
             }
 
@@ -2718,8 +2706,8 @@ bool CSSParser::parseShadow(int propId, bool important)
                 parsedColor = parseColor(val);
 
             if (!parsedColor || !context.allowColor)
-                return context.failed(); // This value is not a color or length and is invalid or
-                                         // it is a color, but a color isn't allowed at this point.
+                return false; // This value is not a color or length and is invalid or
+                              // it is a color, but a color isn't allowed at this point.
             
             context.commitColor(parsedColor);
         }
@@ -2730,13 +2718,13 @@ bool CSSParser::parseShadow(int propId, bool important)
     if (context.allowBreak) {
         context.commitValue();
         if (context.values->length()) {
-            addProperty(propId, context.values, important);
+            addProperty(propId, context.values.release(), important);
             valueList->next();
             return true;
         }
     }
     
-    return context.failed();
+    return false;
 }
 
 struct BorderImageParseContext
@@ -2747,15 +2735,6 @@ struct BorderImageParseContext
      m_borderLeft(0), m_horizontalRule(0), m_verticalRule(0)
     {}
     
-    ~BorderImageParseContext() {
-        if (!m_allowBreak) {
-            delete m_image;
-            delete m_top; delete m_right; delete m_bottom; delete m_left;
-            delete m_borderTop; delete m_borderRight; delete m_borderBottom; delete m_borderLeft;
-        }
-    }
-
-    bool failed() { return m_allowBreak = false; }
     bool allowBreak() const { return m_allowBreak; }
     bool allowNumber() const { return m_allowNumber; }
     bool allowSlash() const { return m_allowSlash; }
@@ -2783,14 +2762,14 @@ struct BorderImageParseContext
     void commitSlash() { m_allowBreak = m_allowSlash = m_allowNumber = false; m_allowWidth = true; }
     void commitWidth(Value* val) {
         if (!m_borderTop)
-            m_borderTop = val;
+            m_borderTop.set(val);
         else if (!m_borderRight)
-            m_borderRight = val;
+            m_borderRight.set(val);
         else if (!m_borderBottom)
-            m_borderBottom = val;
+            m_borderBottom.set(val);
         else {
             ASSERT(!m_borderLeft);
-            m_borderLeft = val;
+            m_borderLeft.set(val);
         }
 
         m_allowBreak = m_allowRule = true;
@@ -2856,17 +2835,17 @@ struct BorderImageParseContext
     bool m_allowWidth;
     bool m_allowRule;
     
-    CSSImageValue* m_image;
+    RefPtr<CSSImageValue> m_image;
     
-    CSSPrimitiveValue* m_top;
-    CSSPrimitiveValue* m_right;
-    CSSPrimitiveValue* m_bottom;
-    CSSPrimitiveValue* m_left;
+    RefPtr<CSSPrimitiveValue> m_top;
+    RefPtr<CSSPrimitiveValue> m_right;
+    RefPtr<CSSPrimitiveValue> m_bottom;
+    RefPtr<CSSPrimitiveValue> m_left;
     
-    Value* m_borderTop;
-    Value* m_borderRight;
-    Value* m_borderBottom;
-    Value* m_borderLeft;
+    OwnPtr<Value> m_borderTop;
+    OwnPtr<Value> m_borderRight;
+    OwnPtr<Value> m_borderBottom;
+    OwnPtr<Value> m_borderLeft;
     
     int m_horizontalRule;
     int m_verticalRule;
@@ -2878,11 +2857,11 @@ bool CSSParser::parseBorderImage(int propId, bool important)
     BorderImageParseContext context;
     Value* val = valueList->current();
     if (val->unit != CSSPrimitiveValue::CSS_URI)
-        return context.failed();
+        return false;
         
     String uri = parseURL(domString(val->string));
     if (uri.isEmpty())
-        return context.failed();
+        return false;
     
     context.commitImage(new CSSImageValue(String(KURL(styleElement->baseURL().deprecatedString(), uri.deprecatedString()).url()),
                                                              styleElement));
@@ -2899,7 +2878,7 @@ bool CSSParser::parseBorderImage(int propId, bool important)
             context.commitRule(val->id);
         } else {
             // Something invalid was encountered.
-            return context.failed();
+            return false;
         }
     }
     
@@ -2909,7 +2888,7 @@ bool CSSParser::parseBorderImage(int propId, bool important)
         return true;
     }
     
-    return context.failed();
+    return false;
 }
 
 bool CSSParser::parseCounter(int propId, int defaultValue, bool important)