<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" /><style type="text/css"><!--
#msg dl { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fc0 solid; padding: 6px; }
#msg ul, pre { overflow: auto; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<title>[23925] trunk/WebCore</title>
</head>
<body>
<div id="msg">
<dl>
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/23925">23925</a></dd>
<dt>Author</dt> <dd>aroben</dd>
<dt>Date</dt> <dd>2007-07-01 22:48:00 -0700 (Sun, 01 Jul 2007)</dd>
</dl>
<h3>Log Message</h3>
<pre>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'.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkWebCoreChangeLog">trunk/WebCore/ChangeLog</a></li>
<li><a href="#trunkWebCorecsscssparsercpp">trunk/WebCore/css/cssparser.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/WebCore/ChangeLog (23924 => 23925)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/WebCore/ChangeLog 2007-07-02 05:18:19 UTC (rev 23924)
+++ trunk/WebCore/ChangeLog 2007-07-02 05:48:00 UTC (rev 23925)
</span><span class="lines">@@ -1,3 +1,45 @@
</span><ins>+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'.
+
</ins><span class="cx"> 2007-07-01 Anders Carlsson <andersca@apple.com>
</span><span class="cx">
</span><span class="cx"> Reviewed by John Sullivan.
</span></span></pre></div>
<a id="trunkWebCorecsscssparsercpp"></a>
<div class="modfile"><h4>Modified: trunk/WebCore/css/cssparser.cpp (23924 => 23925)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/WebCore/css/cssparser.cpp 2007-07-02 05:18:19 UTC (rev 23924)
+++ trunk/WebCore/css/cssparser.cpp 2007-07-02 05:48:00 UTC (rev 23925)
</span><span class="lines">@@ -2609,20 +2609,8 @@
</span><span class="cx"> allowBreak(true)
</span><span class="cx"> {}
</span><span class="cx">
</span><del>- ~ShadowParseContext() {
- if (!allowBreak) {
- delete values;
- delete x;
- delete y;
- delete blur;
- delete color;
- }
- }
-
</del><span class="cx"> bool allowLength() { return allowX || allowY || allowBlur; }
</span><span class="cx">
</span><del>- bool failed() { return allowBreak = false; }
-
</del><span class="cx"> void commitValue() {
</span><span class="cx"> // Handle the ,, case gracefully by doing nothing.
</span><span class="cx"> if (x || y || blur || color) {
</span><span class="lines">@@ -2630,7 +2618,7 @@
</span><span class="cx"> values = new CSSValueList();
</span><span class="cx">
</span><span class="cx"> // Construct the current shadow value and add it to the list.
</span><del>- values->append(new ShadowValue(x, y, blur, color));
</del><ins>+ values->append(new ShadowValue(x.release(), y.release(), blur.release(), color.release()));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Now reset for the next shadow value.
</span><span class="lines">@@ -2640,18 +2628,18 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void commitLength(Value* v) {
</span><del>- CSSPrimitiveValue* val = new CSSPrimitiveValue(v->fValue,
- (CSSPrimitiveValue::UnitTypes)v->unit);
</del><ins>+ RefPtr<CSSPrimitiveValue> val = new CSSPrimitiveValue(v->fValue, (CSSPrimitiveValue::UnitTypes)v->unit);
+
</ins><span class="cx"> if (allowX) {
</span><del>- x = val;
</del><ins>+ x = val.release();
</ins><span class="cx"> allowX = false; allowY = true; allowColor = false; allowBreak = false;
</span><span class="cx"> }
</span><span class="cx"> else if (allowY) {
</span><del>- y = val;
</del><ins>+ y = val.release();
</ins><span class="cx"> allowY = false; allowBlur = true; allowColor = true; allowBreak = true;
</span><span class="cx"> }
</span><span class="cx"> else if (allowBlur) {
</span><del>- blur = val;
</del><ins>+ blur = val.release();
</ins><span class="cx"> allowBlur = false;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -2665,11 +2653,11 @@
</span><span class="cx"> allowBlur = false;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- CSSValueList* values;
- CSSPrimitiveValue* x;
- CSSPrimitiveValue* y;
- CSSPrimitiveValue* blur;
- CSSPrimitiveValue* color;
</del><ins>+ RefPtr<CSSValueList> values;
+ RefPtr<CSSPrimitiveValue> x;
+ RefPtr<CSSPrimitiveValue> y;
+ RefPtr<CSSPrimitiveValue> blur;
+ RefPtr<CSSPrimitiveValue> color;
</ins><span class="cx">
</span><span class="cx"> bool allowX;
</span><span class="cx"> bool allowY;
</span><span class="lines">@@ -2688,7 +2676,7 @@
</span><span class="cx"> if (val->iValue != ',' || !context.allowBreak)
</span><span class="cx"> // Other operators aren't legal or we aren't done with the current shadow
</span><span class="cx"> // value. Treat as invalid.
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> // The value is good. Commit it.
</span><span class="cx"> context.commitValue();
</span><span class="lines">@@ -2697,7 +2685,7 @@
</span><span class="cx"> else if (validUnit(val, FLength, true)) {
</span><span class="cx"> // We required a length and didn't get one. Invalid.
</span><span class="cx"> if (!context.allowLength())
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> // A length is allowed here. Construct the value and add it.
</span><span class="cx"> context.commitLength(val);
</span><span class="lines">@@ -2709,7 +2697,7 @@
</span><span class="cx"> (val->id >= CSS_VAL__WEBKIT_FOCUS_RING_COLOR && val->id <= CSS_VAL__WEBKIT_TEXT && !strict));
</span><span class="cx"> if (isColor) {
</span><span class="cx"> if (!context.allowColor)
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx"> parsedColor = new CSSPrimitiveValue(val->id);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -2718,8 +2706,8 @@
</span><span class="cx"> parsedColor = parseColor(val);
</span><span class="cx">
</span><span class="cx"> if (!parsedColor || !context.allowColor)
</span><del>- 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.
</del><ins>+ 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.
</ins><span class="cx">
</span><span class="cx"> context.commitColor(parsedColor);
</span><span class="cx"> }
</span><span class="lines">@@ -2730,13 +2718,13 @@
</span><span class="cx"> if (context.allowBreak) {
</span><span class="cx"> context.commitValue();
</span><span class="cx"> if (context.values->length()) {
</span><del>- addProperty(propId, context.values, important);
</del><ins>+ addProperty(propId, context.values.release(), important);
</ins><span class="cx"> valueList->next();
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> struct BorderImageParseContext
</span><span class="lines">@@ -2747,15 +2735,6 @@
</span><span class="cx"> m_borderLeft(0), m_horizontalRule(0), m_verticalRule(0)
</span><span class="cx"> {}
</span><span class="cx">
</span><del>- ~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; }
</del><span class="cx"> bool allowBreak() const { return m_allowBreak; }
</span><span class="cx"> bool allowNumber() const { return m_allowNumber; }
</span><span class="cx"> bool allowSlash() const { return m_allowSlash; }
</span><span class="lines">@@ -2783,14 +2762,14 @@
</span><span class="cx"> void commitSlash() { m_allowBreak = m_allowSlash = m_allowNumber = false; m_allowWidth = true; }
</span><span class="cx"> void commitWidth(Value* val) {
</span><span class="cx"> if (!m_borderTop)
</span><del>- m_borderTop = val;
</del><ins>+ m_borderTop.set(val);
</ins><span class="cx"> else if (!m_borderRight)
</span><del>- m_borderRight = val;
</del><ins>+ m_borderRight.set(val);
</ins><span class="cx"> else if (!m_borderBottom)
</span><del>- m_borderBottom = val;
</del><ins>+ m_borderBottom.set(val);
</ins><span class="cx"> else {
</span><span class="cx"> ASSERT(!m_borderLeft);
</span><del>- m_borderLeft = val;
</del><ins>+ m_borderLeft.set(val);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> m_allowBreak = m_allowRule = true;
</span><span class="lines">@@ -2856,17 +2835,17 @@
</span><span class="cx"> bool m_allowWidth;
</span><span class="cx"> bool m_allowRule;
</span><span class="cx">
</span><del>- CSSImageValue* m_image;
</del><ins>+ RefPtr<CSSImageValue> m_image;
</ins><span class="cx">
</span><del>- CSSPrimitiveValue* m_top;
- CSSPrimitiveValue* m_right;
- CSSPrimitiveValue* m_bottom;
- CSSPrimitiveValue* m_left;
</del><ins>+ RefPtr<CSSPrimitiveValue> m_top;
+ RefPtr<CSSPrimitiveValue> m_right;
+ RefPtr<CSSPrimitiveValue> m_bottom;
+ RefPtr<CSSPrimitiveValue> m_left;
</ins><span class="cx">
</span><del>- Value* m_borderTop;
- Value* m_borderRight;
- Value* m_borderBottom;
- Value* m_borderLeft;
</del><ins>+ OwnPtr<Value> m_borderTop;
+ OwnPtr<Value> m_borderRight;
+ OwnPtr<Value> m_borderBottom;
+ OwnPtr<Value> m_borderLeft;
</ins><span class="cx">
</span><span class="cx"> int m_horizontalRule;
</span><span class="cx"> int m_verticalRule;
</span><span class="lines">@@ -2878,11 +2857,11 @@
</span><span class="cx"> BorderImageParseContext context;
</span><span class="cx"> Value* val = valueList->current();
</span><span class="cx"> if (val->unit != CSSPrimitiveValue::CSS_URI)
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> String uri = parseURL(domString(val->string));
</span><span class="cx"> if (uri.isEmpty())
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> context.commitImage(new CSSImageValue(String(KURL(styleElement->baseURL().deprecatedString(), uri.deprecatedString()).url()),
</span><span class="cx"> styleElement));
</span><span class="lines">@@ -2899,7 +2878,7 @@
</span><span class="cx"> context.commitRule(val->id);
</span><span class="cx"> } else {
</span><span class="cx"> // Something invalid was encountered.
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -2909,7 +2888,7 @@
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- return context.failed();
</del><ins>+ return false;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool CSSParser::parseCounter(int propId, int defaultValue, bool important)
</span></span></pre>
</div>
</div>
</body>
</html>