|Summary:||MASTER: Assert when you forget to adoptRef to make it harder to leak by accident|
|Product:||WebKit||Reporter:||Adam Barth <abarth>|
|Component:||Tools / Tests||Assignee:||Darin Adler <darin>|
|Severity:||Normal||CC:||abarth, darin, levin, michelangelo, mjs, oliver|
|Version:||528+ (Nightly build)|
|Bug Depends on:||28068, 40760, 41422, 41823|
Description Adam Barth 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.
Comment 1 Adam Barth 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.
Comment 2 David Levin 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.
Comment 3 David Levin 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.)
Comment 4 Adam Barth 2009-10-20 21:54:17 PDT
We got bit by this twice today. I'm going to try to make this patch work.
Comment 7 Darin Adler 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.
Comment 8 Darin Adler 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.
Comment 9 Darin Adler 2010-07-01 13:53:16 PDT
Turned on the assertion for TreeShared in <http://trac.webkit.org/changeset/62291>.
Comment 10 Darin Adler 2010-07-14 16:08:32 PDT
I think the only remaining work is to turn this on for CrossThreadRefCounted and ThreadSafeShared.
Comment 11 Darin Adler 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.
Comment 12 Darin Adler 2011-11-01 14:14:10 PDT
Which is now named ThreadSafeRefCounted.
Comment 13 Darin Adler 2011-11-02 16:18:53 PDT
I’m closing this. ThreadSafeRefCounted is hardly used at all so no need to wait on it.