working in my tree; will attach patch around Jan 7/8, 2002.

Patch also fixes bug 116033, bug 94080, bug 98052

Comment on attachment 64003 [details] [diff] [review] Patch to fix this and a whole bunch of other bugs >+ return val->QueryInterface(NS_GET_IID(nsIDOMCSSPrimitiveValue), >+ (void **)&aValue); Use CallQueryInterface here. >+ val->SetString(NS_LITERAL_STRING("iherit")); "i_n_herit" ;) >+ if (topVal) >+ delete topVal; >+ if (rightVal) >+ delete rightVal; >+ if (bottomVal) >+ delete bottomVal; >+ if (leftVal) >+ delete leftVal; >+ rv = NS_ERROR_OUT_OF_MEMORY; >+ } >+ } >+ } >+ >+ if (NS_FAILED(rv)) { >+ delete val; >+ return rv; >+ } As discussed, don't leak topVal etc, move it down to where you delete val. >+NS_IMETHODIMP >+nsDOMCSSRect::GetTop(nsIDOMCSSPrimitiveValue** aTop) >+{ >+ NS_ENSURE_TRUE(mTop, NS_ERROR_NOT_INITIALIZED); I think you should NS_ENSURE_ARG_POINTER(aTop) in the getters in nsDOMCSSRect. >- case CSS_RECT : > case CSS_RGBCOLOR : >+ aCssText.Truncate(); > return NS_ERROR_DOM_INVALID_ACCESS_ERR; just setting result = NS_ERROR_DOM_INVALID_ACCESS_ERR would work here too. > } > >- aCssText.Assign(tmpStr); >+ if (NS_FAILED(result)) { >+ aCssText.Truncate(); >+ } else { >+ aCssText.Assign(tmpStr); >+ } With these changes (NS_ENSURE_ARG_POINTER is optional), r=peterv.

Comment on attachment 64003 [details] [diff] [review] Patch to fix this and a whole bunch of other bugs What peterv said, and I'd vote for not adding NS_ENSURE_ARG_POINTER()'s, whoever calls these methods w/o passing in valid out parameters needs to suffer. >+ val->SetString(style.get()); Is it time to make SetString() take a nsACString& in stead of char*? >- if(str_weight.Length()>0) { >+ if(! str_weight.IsEmpty()) { Ew, eek, space after negation operator, horror! :-) >+ if (topVal) >+ delete topVal; >+ if (rightVal) >+ delete rightVal; >+ if (bottomVal) >+ delete bottomVal; >+ if (leftVal) >+ delete leftVal; |delete| is null safe, no need for all those if checks. > nsROCSSPrimitiveValue::GetCssText(nsAWritableString& aCssText) > { > nsAutoString tmpStr; >- >- aCssText.Truncate(); Not sure I agree with this change, it's basically a nop, and it's good for maintainability, so I'd say leave the Truncate() call there. >+ case CSS_RECT : >+ { >+ nsCOMPtr<nsIDOMCSSPrimitiveValue> sideCSSValue; >+ nsAutoString sideValue; >+ tmpStr = NS_LITERAL_STRING("rect("); >+ // get the top >+ result = mRect->GetTop(getter_AddRefs(sideCSSValue)); >+ if (NS_FAILED(result)) >+ break; >+ result = sideCSSValue->GetCssText(sideValue); >+ if (NS_FAILED(result)) >+ break; >+ tmpStr.Append(sideValue + NS_LITERAL_STRING(", ")); You have 3 occurances of the literal string ", ", use NS_NAMED_LITERAL_STRING() and use the variable in stead of duplicating the literal. > nsROCSSPrimitiveValue::GetStringValue(nsAWritableString& aReturn) > { >+ if (mType != CSS_STRING) { Truncate() aReturn here. >+ return NS_ERROR_DOM_INVALID_ACCESS_ERR; >+ } ... > nsROCSSPrimitiveValue::GetRectValue(nsIDOMRect** aReturn) > { >- return NS_ERROR_DOM_NOT_SUPPORTED_ERR; >+ if (mType != CSS_RECT || !mRect) { Null out *aReturn here, returning from methods with out parameters w/o initializing them is not cool, even if you return an error. >+ return NS_ERROR_DOM_INVALID_ACCESS_ERR; >+ } >+ return CallQueryInterface(mRect, aReturn); > } With that, sr=jst

fixed.