1070064 - figure out what to do about RefPtr.h

Status: RESOLVED WORKSFORME

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

RefPtr.h contains two major pieces of functionality:

- RefCounted<T>, for mixing in to provide refcounting functionality;
- RefPtr<T>, a smart pointer for reference-counted classes.

We've jumped through several hoops getting refcount logging to work for RefCounted<T>, but even so, ISTR that RefCounted<T> usage is actively discouraged in new code...Ehsan, do you recall the particular reasons?

Similarly, though less strongly, we encourage people to use nsRefPtr<T> instead of RefPtr<T>.

There are good reasons why one might use RefPtr instead of nsRefPtr; the graphics team, for instance, would like to build Moz2D standalone, yet still use reference counting types.  RefPtr means that they can build code unmodified for standalone builds and Gecko builds.

I was informed of a plan to build WebRTC standalone (bug 1045967), which also benefits from a RefPtr type that's not dependent on XPCOM bits and pieces.  They seem to want to use their own refcounting thing, not sure if that's good or bad yet.

I know we had talked about doing something with this header before (hence the CC to Bas and Vlad), but I cannot remember all the details!  But I am more motivated than before now that we have two clients wanting to do things with non-Gecko builds and RefPtr.h...it almost certainly means there will be more.

Maybe this is just a dup of 1028264, though?

Comment 1

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> RefPtr.h contains two major pieces of functionality:
> 
> - RefCounted<T>, for mixing in to provide refcounting functionality;
> - RefPtr<T>, a smart pointer for reference-counted classes.
> 
> We've jumped through several hoops getting refcount logging to work for
> RefCounted<T>, but even so, ISTR that RefCounted<T> usage is actively
> discouraged in new code...Ehsan, do you recall the particular reasons?

Currently refcount logging only works if all of the code is compiled into libxul, which has lead us to do horrendous hacks such as building moz2d inside libxul as opposed to gkmedias and hope that none of the other gfx code that uses it through headers and whatnot gets build in gkmedias.

Also, proper usage of RefCounted now requires a macro, so the only theoretical merit for using it (avoid the usage of a macro) no longer holds.  In practice though, it's needed in code that cannot link against xpcom (such as mozalloc/mozmemory) because I have never been able to figure out what it takes to put the code responsible for refcount logging in mozglue.

> Similarly, though less strongly, we encourage people to use nsRefPtr<T>
> instead of RefPtr<T>.

Yes.  RefPtr<T> has literally zero advantages over nsRefPtr.

> There are good reasons why one might use RefPtr instead of nsRefPtr; the
> graphics team, for instance, would like to build Moz2D standalone, yet still
> use reference counting types.  RefPtr means that they can build code
> unmodified for standalone builds and Gecko builds.

If that is a goal, we can make nsRefPtr usable standalone (if it's not already.)

> I was informed of a plan to build WebRTC standalone (bug 1045967), which
> also benefits from a RefPtr type that's not dependent on XPCOM bits and
> pieces.

They should use nsRefPtr, see above.

> They seem to want to use their own refcounting thing, not sure if
> that's good or bad yet.

As our experience with introducing the MFBT way of refcounting has shown, that is a terrible idea.  We should definitely not replicate that failure once again.

> I know we had talked about doing something with this header before (hence
> the CC to Bas and Vlad), but I cannot remember all the details!  But I am
> more motivated than before now that we have two clients wanting to do things
> with non-Gecko builds and RefPtr.h...it almost certainly means there will be
> more.
> 
> Maybe this is just a dup of 1028264, though?

I think it is, unless you can think of another reason why RefPtr needs to live.  (Note that we cannot currently ensure that RefCounted is unused because of the issue above, but the RefPtr class can definitely die.)

Comment 2

[Nathan Froyd [:froydnj]]

We renamed nsRefPtr to RefPtr, so we now need to keep around RefPtr.h or lots of things will break.  We've also done a good job of moving specific things out of RefPtr.h into their own headers.

Comment 3

[Andrew McCreight [:mccr8]]

There's also bug 1071102 about removing uses of RefPtr.h, though it seems pretty tricky to do well.