643297 - nsXBLProtoImplMethod::SetUncompiledMethod leaks if called twice on the same object

Status: RESOLVED INVALID

(Core :: XBL, defect)

Description

[(no longer active)]

nsXBLProtoImplMethod::SetUncompiledMethod doesn't free the previously allocated uncompiled method if called twice in a row.

Found this leak using the OS X leaks tool.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the way this gets hit is if there's a syntax error in the method, right?  I don't see any other way this can happen....

Comment 3

[(no longer active)]

(In reply to comment #2)
> So the way this gets hit is if there's a syntax error in the method, right?  I
> don't see any other way this can happen....

Yes, I think you're right.  In fact, with this patch, I get a crash in this test case <http://mxr.mozilla.org/mozilla-central/source/content/xbl/crashtests/232095-1.xul>.  In this test case, the getSuccessfulControls method is invalid, which causes the CompileFunction call in nsXBLProtoImplMethod::CompileMember to fail.  In that case, we have already deleted uncompiledMethod by the time we get to the SetUncompiledMethod call, which causes us to double-delete the object with this patch.  After investigating more thoroughly, I think that delete takes care of the potential leak here.

Nevertheless, we are getting the leak report using the leaks tool here.  This made me think about why, and I think I know why now.  We don't actually store a pointer to the allocated nsXBLUncompiledMethod anywhere.  We add 1 to the pointer value, to serve as a distinguishing bit in the union to know which member is valid.  This confuses the leaks tool, thinking that we've lost the last pointer to the allocated memory, therefore flagging a false positive leak.  This is a useful thing to watch for when looking at the output of leaks, and it implicitly makes this bug INVALID.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  Marked pointers make "leaks" unhappy.  I wish there were a way to teach it about them (other than telling it to ignore leaks of the corresponding objects, of course).

Which sucks even more given how many tagged pointers we use.  :(

Comment 5

[(no longer active)]

(In reply to comment #4)
> Ah, yeah.  Marked pointers make "leaks" unhappy.  I wish there were a way to
> teach it about them (other than telling it to ignore leaks of the corresponding
> objects, of course).
> 
> Which sucks even more given how many tagged pointers we use.  :(

Yeah, "man leaks" doesn't seem to offer any way to protect against any such dumb false positives (not even by tagging some objects/functions to be ignored).  I guess grep -v is our best friend there.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

You can use -exclude to exclude things, no?

Comment 7

[(no longer active)]

(In reply to comment #6)
> You can use -exclude to exclude things, no?

Hmm, originally I couldn't get it to work, but when I tried again I could.  Maybe I had a typo or something the first time...

Comment 8

[timeless]

you could add some #ifdef TRACKING fields which store untagged pointers.

it won't result in true fidelity, but it would work.