241272 - Various nsAttrValue methods don't reset properly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

From my last mail to sicking (the part he wrote is the quoted part):

---------------------------------------------------
Jonas Sicking wrote:

> Oh, actually, i have a theory. ParseSpecialIntValue doesn't Reset() the old
> value but rather just overwrites it. Can't figure out exactly why this would
> crash rather then leak, but in any case it's a bug.

Yeah, I don't see why it would crash either.  But yes, it could leak. Same issue
with ParseEnumValue(), ParseIntWithBounds(). 
---------------------------------------------------

We should fix those three methods, then see whether talkback still shows the
1.7b crash it's showing now in nsHTMLTableElement::ParseAttribute...

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should fix this on branch too....

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

the changes to nsAttrValue::ParseColor doesn't actually fix leaks. It's just to
make that function behave more consistently with the other parse functions
which resets the attrvalue in case the parsing failed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147351 [details] [diff] [review]
patch to fix

r+sr=bzbarsky

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 147351 [details] [diff] [review]
patch to fix

this patch is very simple so it should be safe. I'm not sure that we're
actually leaking memory as it is, because in most cases we're passing
attrvalues that are already reset. However I'm not 100% sure that's always the
case, so it would definitly be good to have this checked in.

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 147351 [details] [diff] [review]
patch to fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in on both branch and trunk