RESOLVED FIXED 27672
MASTER: Assert when you forget to adoptRef to make it harder to leak by accident 
https://bugs.webkit.org/show_bug.cgi?id=27672
Summary MASTER: Assert when you forget to adoptRef to make it harder to leak by accident
Adam Barth
Reported 2009-07-24 17:37:55 PDT
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.
Attachments
Here's a start at implementing the assert functionality (9.40 KB, patch)
2009-07-27 15:14 PDT, David Levin 		no flags
DetailsFormatted DiffDiff
work-in-progress (10.66 KB, patch)
2009-10-21 00:47 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
work-in-progress (15.36 KB, patch)
2009-10-21 10:13 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2009-07-24 17:39:26 PDT
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.
David Levin
Comment 2 2009-07-24 18:03:22 PDT
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.
David Levin
Comment 3 2009-07-27 15:14:04 PDT
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.)
Adam Barth
Comment 4 2009-10-20 21:54:17 PDT
We got bit by this twice today. I'm going to try to make this patch work.
Adam Barth
Comment 5 2009-10-21 00:47:10 PDT
Created attachment 41554 [details] work-in-progress
Adam Barth
Comment 6 2009-10-21 10:13:52 PDT
Created attachment 41573 [details] work-in-progress
Darin Adler
Comment 7 2010-06-14 16:54:33 PDT
I’ve made a lot of progress on this. I’ll have some good things to land soon.
Darin Adler
Comment 8 2010-06-28 13:36:33 PDT
I have a patch to do this without so much template magic on my other computer. Will attach it soon.
Darin Adler
Comment 9 2010-07-01 13:53:16 PDT
Turned on the assertion for TreeShared in <http://trac.webkit.org/changeset/62291>.
Darin Adler
Comment 10 2010-07-14 16:08:32 PDT
I think the only remaining work is to turn this on for CrossThreadRefCounted and ThreadSafeShared.
Darin Adler
Comment 11 2011-11-01 14:13:50 PDT
(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.
Darin Adler
Comment 12 2011-11-01 14:14:10 PDT
Which is now named ThreadSafeRefCounted.
Darin Adler
Comment 13 2011-11-02 16:18:53 PDT
I’m closing this. ThreadSafeRefCounted is hardly used at all so no need to wait on it.
Note You need to log in before you can comment on or make changes to this bug.