Closed Bug 643297 Opened 9 years ago Closed 9 years ago
XBLProto Impl Method::Set Uncompiled Method leaks if called twice on the same object
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.
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
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.
You need to log in before you can comment on or make changes to this bug.