Bug 41422 – Add assertion, off by default, for when you forget to do adoptRef

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Add assertion, off by default, for when you forget to do adoptRef

[Darin Adler]

Created attachment 60133 [details]
Patch

[Eric Seidel (no email)]

Attachment 60133 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3370085

[WebKit Review Bot]

Attachment 60133 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3283986

[WebKit Review Bot]

Attachment 60133 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3375060

[Darin Adler]

Created attachment 60151 [details]
Patch

[Darin Adler]

New patch fixes the release build.

[Adam Barth]

Should we do style cleanup in a separate patch?

[Darin Adler]

(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.

[Adam Barth]

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?

[Eric Seidel (no email)]

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?

[Darin Adler]

(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.

[Darin Adler]

(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?

[Eric Seidel (no email)]

(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.

[Darin Adler]

Committed r62213: <http://trac.webkit.org/changeset/62213>