When you make a RefCounted class, you really ought to have a FooBar::create method that adopts the initial ref for the object otherwise you're going to leak a lot. We should make it easier for developers to remember to make these methods.
One possibility is to ASSERT that at least one object of the type was destructed if at least one object was constructed. When you make this mistake, you tend to leak *all* instances of the derived class.
I wonder how many locations violate that pattern. An alternate way to catch problems is to have adoptRef set a flag in RefCounted<> in debug. If the first ref count happens before adoptRef is called, then assert.
Created attachment 33573 [details] Here's a start at implementing the assert functionality This patch has lots of issues still, so it isn't up for review, but I'm putting it here in case someone else want to play with it. Details: The addition of the adoptRef's in Nodes.cpp causes a crash on start. If you change this to call the setAdoptRefCalled, you'll hit another assert while running WebKit test. (And the method naming leaves much to be desired.)
We got bit by this twice today. I'm going to try to make this patch work.
Created attachment 41554 [details] work-in-progress
Created attachment 41573 [details] work-in-progress
I’ve made a lot of progress on this. I’ll have some good things to land soon.
I have a patch to do this without so much template magic on my other computer. Will attach it soon.
Turned on the assertion for TreeShared in <http://trac.webkit.org/changeset/62291>.
I think the only remaining work is to turn this on for CrossThreadRefCounted and ThreadSafeShared.
(In reply to comment #10) > I think the only remaining work is to turn this on for CrossThreadRefCounted and ThreadSafeShared. CrossThreadRefCounted is gone. So the only remaining work is to turn this on for ThreadSafeShared.
Which is now named ThreadSafeRefCounted.
I’m closing this. ThreadSafeRefCounted is hardly used at all so no need to wait on it.