Bug 27672

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>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, levin, michelangelo, mjs, oliver

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

28068, 40760, 41422, 41823

Bug Blocks:

Attachments:

Description	Flags
Here's a start at implementing the assert functionality	none
work-in-progress	none
work-in-progress	none

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 5 Adam Barth 2009-10-21 00:47:10 PDT

Created attachment 41554 [details]
work-in-progress

Comment 6 Adam Barth 2009-10-21 10:13:52 PDT

Created attachment 41573 [details]
work-in-progress

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.