Bug 41422 - Add assertion, off by default, for when you forget to do adoptRef
Summary: Add assertion, off by default, for when you forget to do adoptRef
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks: 27672
  Show dependency treegraph
 
Reported: 2010-06-30 10:56 PDT by Darin Adler
Modified: 2010-06-30 16:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.36 KB, patch)
2010-06-30 11:01 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (18.43 KB, patch)
2010-06-30 14:12 PDT, Darin Adler
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2010-06-30 10:56:02 PDT
Add assertion, off by default, for when you forget to do adoptRef
Comment 1 Darin Adler 2010-06-30 11:01:25 PDT
Created attachment 60133 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-06-30 11:13:33 PDT
Attachment 60133 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3370085
Comment 3 WebKit Review Bot 2010-06-30 12:27:44 PDT
Attachment 60133 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3283986
Comment 4 WebKit Review Bot 2010-06-30 13:15:57 PDT
Attachment 60133 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3375060
Comment 5 Darin Adler 2010-06-30 14:12:42 PDT
Created attachment 60151 [details]
Patch
Comment 6 Darin Adler 2010-06-30 14:13:00 PDT
New patch fixes the release build.
Comment 7 Adam Barth 2010-06-30 14:14:23 PDT
Should we do style cleanup in a separate patch?
Comment 8 Darin Adler 2010-06-30 14:15:28 PDT
(In reply to comment #7)
> Should we do style cleanup in a separate patch?

The style changes here are small and should be easy to look at, and this is a patch that limits itself to a small number of source files, but if I have an enthusiastic reviewer who wants a style-only patch for the same files, that’s OK with me too.
Comment 9 Adam Barth 2010-06-30 14:22:57 PDT
Comment on attachment 60151 [details]
Patch

Looks good.  Not sure why I went off to template land before for this patch.

JavaScriptCore/wtf/PassRefPtr.h:32
 +      inline void adopted(const void*)
One line?

JavaScriptCore/wtf/RefCounted.h:70
 +          ASSERT(m_deletionHasBegun);
This means we'll need to delete via deref(), right?
Comment 10 Eric Seidel (no email) 2010-06-30 14:32:08 PDT
Comment on attachment 60151 [details]
Patch

I'm curious if there is a benefit to making adopted() a free function instead of a member function?
Comment 11 Darin Adler 2010-06-30 15:07:36 PDT
(In reply to comment #9)
> JavaScriptCore/wtf/PassRefPtr.h:32
>  +      inline void adopted(const void*)
> One line?

Sure, that would be OK.

> JavaScriptCore/wtf/RefCounted.h:70
>  +          ASSERT(m_deletionHasBegun);
> This means we'll need to delete via deref(), right?

Yes.

I think this goes hand in hand with requiring adoptRef.

It found some good things when I turned it on. I had these assertions on for a while and was fixing all the things they found, but then I decided it was better to do a simple patch that makes it so other could work on it in parallel if they like.
Comment 12 Darin Adler 2010-06-30 15:07:54 PDT
(In reply to comment #10)
> I'm curious if there is a benefit to making adopted() a free function instead of a member function?

I don't think it could be a member function. A member function of what class?
Comment 13 Eric Seidel (no email) 2010-06-30 15:11:02 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > I'm curious if there is a benefit to making adopted() a free function instead of a member function?
> 
> I don't think it could be a member function. A member function of what class?

I see.  RefCounted is not the only way to make something play nice in the RefPtr system.  Makes sense now.
Comment 14 Darin Adler 2010-06-30 16:09:25 PDT
Committed r62213: <http://trac.webkit.org/changeset/62213>