GNOME Bugzilla – Bug 347446
fix leaks
Last modified: 2006-08-30 18:50:29 UTC
drip, drip... Patch coming.
Created attachment 68890 [details] [review] patch First pass. Lots of fixes. We were leaking just about every entry-view, never disposing podcast-source, not unreffing objects and disconnecting signals in properties... OK to commit?
Damn that patch is unreadable with all the whitespace changes... OK if I just go ahead remove the whitespace crap once and for all? Then I'll resync this patch.
I haven't combed through it checking everything, but it works well here.
Created attachment 68920 [details] [review] resynced patch I committed the whitespace parts of the change separately. Here is the same patch without the whitespace changes.
Created attachment 68948 [details] [review] patch Second pass. Not tested a lot. Seems to fix all the leaked references so that the db is finalized!
Created attachment 69032 [details] [review] patch + few more fixes Looks good to me - this has a few additional leak fixes I noticed.
Created attachment 69047 [details] [review] updated patch This is an update that fixes an uninitialized variable from the last patch and a reference counting error on action_groups that has been there all along.
Created attachment 69071 [details] [review] patch Lots more... This gets us a lot closer to being able to finalize RhythmDBEntry's. There are a handful of leaked refs now rather than hundreds or thousands.
Created attachment 69180 [details] [review] resync with cvs Same patch. Resynced with CVS. Looks like part of the last patch was fixed by the patch from bug #347985. Does this look OK?
Seems okay from my testing.
Actually, maybe not - there seems to be an extraneous unref/missing ref somewhere. When the playing song finishes, it's refcount seems to ends up one lower then it was, which will eventually remove all the refs to it and explode.
Created attachment 69363 [details] [review] updated patch Good catch. This should fix it.
Looks okay to me
Created attachment 69510 [details] [review] patch This focuses on ref leaks of query-models. Also prevents dispose from running twice on a few sources.
Created attachment 69546 [details] [review] updated patch Fixes a few typos and the like. This seems to work from my quick testing. One of the causes of these leaks is probably that our API is very inconsistent as to whether it returns a new or borrowed reference to things, which also causes issues for the bindings. It might be worth going through everything and making it always return a copy/referenced object unless it's documented (API docs. yay) to do otherwise.
Cool. Thanks for catching that typo. I've committed this. Yeah, we should probably go over the whole public API and make sure things make sense. That would probably be a good time to try to document it. :) So, the only thing left as far as leakage goes is the final few refs on entries.
Marking patch committed, since it was.
We'll consider this fixed.