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

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	Details Formatted Diff Diff
work-in-progress (10.66 KB, patch) 2009-10-21 00:47 PDT, Adam Barth	no flags	Details Formatted Diff Diff
work-in-progress (15.36 KB, patch) 2009-10-21 10:13 PDT, Adam Barth	no flags	Details Formatted Diff Diff
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.