Bug 205617 – Add makeRefCounted<>

• Status: NEW
• Component: Web Template Framework

[Antti Koivisto]

Add a standalone template function for constructing RefCounted objects, similar to makeUnique<>(). This allows clean construction of reference counted objects without clunky static create() functions.

[Antti Koivisto]

Created attachment 386469 [details]
patch

[Sam Weinig]

I'm not sure if this is a good idea or not.

For context, we historically used the create() pattern, rather than introduce a makeRefCounted(), based on the assumption that it never makes sense to directly call the constructor of a RefCounted object and not put it into a Ref/RefPtr, and therefore making the constructor private would avoid bugs since you couldn't misuse it.

This is in contrast to objects that live in std::unique_ptr<> or WTF::UniqueRef<>, which often make sense as stack allocated or as a member of another class/struct.

Perhaps there is something in-between we can do, though I can't think of a way to do it at the moment, where we keep RefCounted derived objects' constructors private, but allow makeRefCounted<>() to call it, perhaps via friend or some such.

[Sam Weinig]

Created attachment 386472 [details]
One potential way to keep constructors private/protected

Attaching one potential way to keep the constructors private/protected. Requires replacing create() functions with a WTF_MAKE_REF_COUNTED_CONSTRUCTABLE(NameOfClass) (ala WTF_MAKE_NONCOPYABLE, WTF_MAKE_ISO_ALLOCATED, etc).

[Sam Weinig]

I think there is a good argument to be made that the macro based solution I propose above is superior to the create() functions, as it is substantially less code to write per class. 

I think the argument could also be made that the usefulness of preventing RefCounted objects from being incorrectly constructed into something that isn't a Ref/RefPtr might be more hassle than it is worth. 

All things being equal though, I think I would side with the macro solution for now.

[Antti Koivisto]

I'm not convinced that the public constructor would be a problem in practice.

Accidentally stack-allocating refcounted objects and then passing them via RefPtrs seems bit far fetched, makeUnique could static_assert that the type is not RefCounted, and direct new/delete is effectively banned already.

That's why I'd prefer the simplest macro-free solution.

[Anders Carlsson]

(In reply to Antti Koivisto from comment #5)
> I'm not convinced that the public constructor would be a problem in practice.
> 

I agree.

> Accidentally stack-allocating refcounted objects and then passing them via
> RefPtrs seems bit far fetched, makeUnique could static_assert that the type
> is not RefCounted, and direct new/delete is effectively banned already.

It seems like stack-allocating RefCounted would already ASSERT on destruction due to the m_adoptionIsRequired checks in place.

> 
> That's why I'd prefer the simplest macro-free solution.

Again, I agree.

That said, shouldn't this function be called makeRefPtr since it actually returns a RefPtr, just like makeUnique returns an std::unique_ptr ;)

[Alex Christensen]

Another approach is something like what we've done in classes like CustomHeaderFields.  We templatize the create function to just call whatever constructor it can.  This templatized create can be moved to RefCounted and ThreadSafeRefCounted, but then we would need to befriend that class (like we do in many places with our "friend class NeverDestroyed" instances) or have a public constructor.

Either way, I think that it would be a bit nicer to continue having T::create() rather than makeRefCounted<T>() because the code reads nicer.  It's an implementation detail that it's ref counted and we don't need to remind ourselves at every call site.

[Antti Koivisto]

(In reply to Anders Carlsson from comment #6)
> It seems like stack-allocating RefCounted would already ASSERT on
> destruction due to the m_adoptionIsRequired checks in place.

Good point!

> That said, shouldn't this function be called makeRefPtr since it actually
> returns a RefPtr, just like makeUnique returns an std::unique_ptr ;)

It is not called makeUniquePtr isn't it?

makeUnique makes a "unique" object and makeRefCounted makes a RefCounted object. Seems logical to me.

[Antti Koivisto]

(In reply to Alex Christensen from comment #7)
> Another approach is something like what we've done in classes like
> CustomHeaderFields.  We templatize the create function to just call whatever
> constructor it can.  This templatized create can be moved to RefCounted and
> ThreadSafeRefCounted, but then we would need to befriend that class (like we
> do in many places with our "friend class NeverDestroyed" instances) or have
> a public constructor.

What would this achieve?

> Either way, I think that it would be a bit nicer to continue having
> T::create() rather than makeRefCounted<T>() because the code reads nicer. 
> It's an implementation detail that it's ref counted and we don't need to
> remind ourselves at every call site.

Duplicating constructors as static functions seems opposite of nice to me. 

Understanding memory management of an object is important and making it more explicit seems like a good thing (though since we only use create() pattern with refcounted objects it isn't much of a change).

[Alex Christensen]

(In reply to Antti Koivisto from comment #9)
> (In reply to Alex Christensen from comment #7)
> > Another approach is something like what we've done in classes like
> > CustomHeaderFields.  We templatize the create function to just call whatever
> > constructor it can.  This templatized create can be moved to RefCounted and
> > ThreadSafeRefCounted, but then we would need to befriend that class (like we
> > do in many places with our "friend class NeverDestroyed" instances) or have
> > a public constructor.
> 
> What would this achieve?

This would make it so we get a static create function that takes whatever the parameters of the constructor are without having to write one each time.

I think the intent of having private constructors and static create functions was to prevent accidentally making RefCounted types on the stack.  If we had public constructors then we could have something like this:

RefCountedType a { 1, 2 };

Whether we go with makeRefCounted or stick with create, I think we should keep private constructors and befriend something if necessary.

[Antti Koivisto]

> RefCountedType a { 1, 2 };

This will be caught by an assert in RefCounted destructor (m_deletionHasBegun). 

I doubt this is a common enough mistake to be worth adding complexity everywhere to prevent at compile time.

[Geoffrey Garen]

(In reply to Antti Koivisto from comment #8)
> (In reply to Anders Carlsson from comment #6)
> > It seems like stack-allocating RefCounted would already ASSERT on
> > destruction due to the m_adoptionIsRequired checks in place.
> 
> Good point!

In addition to the ASSERT, a tool can tell us "you called new / constructor on a RefCounted object" -- and we are working to enforce other RefCounted rules that way.

I wonder if makeRefCounted can eventually take care of declaring and creating the refcount too, removing the need to inherit from RefCounted. In theory, it might be just fine to stack-allocate, unique-allocate, or refptr-allocate an object, depending on how you use it. (One option would be for makeRefCounted to allocate a RefCountWrapper<T> type, rather than T, where RefCountWrapper<T> inherited from RefCounted<T> and T.)

[Anders Carlsson]

(In reply to Geoffrey Garen from comment #12)
> (In reply to Antti Koivisto from comment #8)
> > (In reply to Anders Carlsson from comment #6)
> > > It seems like stack-allocating RefCounted would already ASSERT on
> > > destruction due to the m_adoptionIsRequired checks in place.
> > 
> > Good point!
> 
> In addition to the ASSERT, a tool can tell us "you called new / constructor
> on a RefCounted object" -- and we are working to enforce other RefCounted
> rules that way.
> 
> I wonder if makeRefCounted can eventually take care of declaring and
> creating the refcount too, removing the need to inherit from RefCounted. In
> theory, it might be just fine to stack-allocate, unique-allocate, or
> refptr-allocate an object, depending on how you use it. (One option would be
> for makeRefCounted to allocate a RefCountWrapper<T> type, rather than T,
> where RefCountWrapper<T> inherited from RefCounted<T> and T.)

That's interesting. It's similar to how std::shared_ptr works if you use std::make_shared.

One thing it would break is "protecting yourself" - RefPtr<Foo> protect(this);.

std::shared_ptr handles this by requiring you to derive from std::enable_shared_from_this.

[Alex Christensen]

That would be great for most of our use of RefCounted, but we also have ThreadSafeRefCounted and custom ref/deref implementations that do interesting things sometimes.  It wouldn't work for those, but those are the minority.

[youenn fablet]

Comment on attachment 386469 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386469&action=review

> Source/WTF/ChangeLog:13
> +        (WTF::makeRefCounted):

Usually we have the create method that is public and the constructor which is private.
This seems like a good patern for RefCounted object as such object should never be created on the stack or we should never forget the adoptRef call.
That would basically means switching from a create method to making makeRefCounted friend.

The alternative would be to have a templated create(Args&&... args) method,  which would be a one liner as well.