Variant's reference count is inconsistent

[Geequlim]

Godot version

4.0 alpha11

System information

Linux x64

Issue description

• The reference count is not increased when construct a Variant with a RefCounted pointer but decreased by the destructor.
• The refernece count is increased when assign a pointer of RefCounted to a Variant

I'm confused about the implementation of the constructor Variant::Variant(const Object *p_object). Can someone help me explain the intent of this code ?

godot/core/variant/variant.cpp

Lines 2692 to 2699 in b438859

	if (p_object->is_ref_counted()) {
	RefCounted *ref_counted = const_cast<RefCounted *>(static_cast<const RefCounted *>(p_object));
	if (!ref_counted->init_ref()) {
	_get_obj().obj = nullptr;
	_get_obj().id = ObjectID();
	return;
	}
	}

Steps to reproduce

The numbers annotated in the sample code below are the reference counts I expect

RefCounted ref;  // 1
ref.reference(); // 2
ref.reference(); // 3
ref.reference(); // 4
print_line("RefCounted ref count: " + itos(ref.reference_get_count()));
{
	Variant v(&ref); // 5
	print_line("RefCounted ref count: " + itos(ref.reference_get_count()));
} // 4
print_line("RefCounted ref count: " + itos(ref.reference_get_count()));
{
	Variant v;
	v = &ref; // 5
	print_line("RefCounted ref count: " + itos(ref.reference_get_count()));
} // 4
print_line("RefCounted ref count: " + itos(ref.reference_get_count()));

The results are not consistent with what I expected

RefCounted ref count: 4
RefCounted ref count: 4
RefCounted ref count: 3
RefCounted ref count: 4
RefCounted ref count: 3

Minimal reproduction project

No response

[neikeq]

This is not a bug. The first time RefCounted is referenced, it must be with init_ref.

[Geequlim]

This is not a bug. The first time RefCounted is referenced, it must be with init_ref.

@neikeq What is the usage of init_ref ?
As the code snippt above shown the Variant change down the reference count.
Do you think this is correct?

[neikeq]

The initial refcount of a RefCounted object is 1. The reason it's 1 and not 0 is because reference() does a conditional increment that fails if refcount is already 0. I think this is to avoid data races (accidentally killing and reviving an object from two different threads).
However, because of this, the first time you reference the object, the refcount becomes 2 instead of 1. This is wrong because there's only one reference. init_ref() handles this case by checking if it's the first time an object is being referenced, decrementing refcount by one in that case.

Why not make reference() private and make init_ref() the only public API to avoid these kinds of mistakes? I don't know, maybe for performance (avoids an extra check). Keep in mind, you are not supposed to call these manually, any way. Only special scenarios need to do that, like script language bindings.

[neikeq]

We could add a debug-only check in init_ref() to print an informative error when it's not used properly. That should help to avoid confusion in the future.

[Geequlim]

Keep in mind, you are not supposed to call these manually, any way. Only special scenarios need to do that, like script language bindings.

I'm making JavaScript language binding when pass a RefCounted back as an Variant the referece count is decrease in this situation.

[neikeq]

If you want to be safe, just call init_ref() always, never use reference(). If you want to avoid an extra check, you can look for places where it's ok to use reference().
You have to be very careful about manually handling the refcount. C# has had many leaks in the past because of this.