Closed Bug 643297 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: XBL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: memory-leak)

Attachments

(1 obsolete file)

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.
Blocks: mlk-fx5+
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #520534 - Flags: review?(jonas)
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....
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Attachment #520534 - Attachment is obsolete: true
Attachment #520534 - Flags: review?(jonas)
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.  :(
(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.  :(
You can use -exclude to exclude things, no?
(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...
you could add some #ifdef TRACKING fields which store untagged pointers.

it won't result in true fidelity, but it would work.
No longer blocks: mlk-fx5+
You need to log in before you can comment on or make changes to this bug.