Memory leak when using weakref()

[allenwp]

Tested versions

• Reproducible in 4.2.1 Stable and master 29b3d9e

System information

Godot v4.2.1.stable - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 980 Ti (NVIDIA; 31.0.15.3699) - Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz (8 Threads)

Issue description

The following code leaks memory:

extends Node

class MyObj:
	var obj: MyObj

func _process(_delta: float) -> void:
	var obj_1 = MyObj.new()
	var obj_2 = MyObj.new()
	# obj_1.obj = obj_2 # does not leak
	obj_1.obj = weakref(obj_2) # leaks

If you swap the commented line with the line that leaks, it will no longer leak.

This also happens with other types:

extends Node

class MyResource extends Resource:
	var my_resource : MyResource

func _process(_delta: float) -> void:
	var res_1 : MyResource = MyResource.new()
	var res_2 : MyResource = MyResource.new()
	#res_1.my_resource = res_2 # does not leak
	res_1.my_resource = weakref(res_2) # leaks

I expect that weakref() should not have this sort of impact in this scenario. Please correct me if I am mistaken!

With weakref():

Without weakref():

Steps to reproduce

1. Paste either of the above code snippets on a node in a scene and run the game.
2. Watch the Static Memory Monitor.

Minimal reproduction project (MRP)

weakref-leak-bug.zip

[DDAN-17]

I'm guessing this is how it's supposed to work, as it's not supposed to un-allocate the memory. It also says this in the documentation, that until the game needs the memory, it won't delete the objects.

[timothyqiu]

"Objects" in "Monitors" panel shows the number of objects growing indefinitely.

Changing var obj: MyObj inside MyObj to var obj: WeakRef fixes the problem.

Assigning the weak reference to a plain MyObj directly is an error:

var x: MyObj = weakref(obj_2)
# ERROR: Trying to assign value of type 'WeakRef' to a variable of type ''.

I guess something wrong happened during the implicit conversion.

[dalexeev]

Use NOTIFICATION_PREDELETE to check if the object is freed:

class MyObj:
    var id: int
    var obj: MyObj

    func _init(p_id: int) -> void:
        id = p_id

    func _notification(what: int) -> void:
        if what == NOTIFICATION_PREDELETE:
            prints(id, "freed")

func _run() -> void:
    var obj_1 = MyObj.new(1)
    var obj_2 = MyObj.new(2)
    #obj_1.obj = obj_2 # Does not leak.
    obj_1.obj = weakref(obj_2) # Does not leak.

class MyObj:
    var id: int
    var obj: WeakRef # <-- !!!

    func _init(p_id: int) -> void:
        id = p_id

    func _notification(what: int) -> void:
        if what == NOTIFICATION_PREDELETE:
            prints(id, "freed")

func _run() -> void:
    var obj_1 = MyObj.new(1)
    var obj_2 = MyObj.new(2)
    obj_1.obj = obj_2 # `obj_2` leaks.
    #obj_1.obj = weakref(obj_2) # Does not leak.

The implicit conversion between WeakRef and Object/RefCounted is quite unexpected. But I think this is not a GDScript issue, the line is marked as unsafe (though we don't have the UNSAFE_ASSIGNMENT warning) the variables are untyped. Looks like this is a core issue that happens at runtime.

[AThousandShips]

There's no casting at least not valid casting occurring, try just print(obj_1.obj) after assigning it, it'll print <null>, there's no warning as weakref is just Variant and can be cast to any object type as null

The leak in the obj -> WeakRef though is strange, that does sound like it's something in the GDScript side though

Edit: It happens regardless of what's contained in obj, so it seems to be related to some leak on the stack somehow, where the local variable is lost in the process and the underlying variant isn't freed, you can do:

obj_1.obj = obj_2
obj_1.obj = weakref(obj_2)

And it still happens

Edit 2:
This seems to be related to how property access is handled, as:

var w : WeakRef = obj_2

Fails with an error at runtime, so something is wrong with how obj_1.obj is processed on assignment, possibly causing some leak in the associated argument data

[AThousandShips]

Will try dig into the casting or conversion of refcounted, seems to be some glitch with the count increment:

print(obj_2.get_reference_count()) # Prints 1
obj_1.obj = obj_2
print(obj_2.get_reference_count()) # Prints 2!

[dalexeev]

This seems to be related to how property access is handled, as:

var w : WeakRef = obj_2

Fails with an error at runtime, so something is wrong with how obj_1.obj is processed on assignment

If the base type is not known at compile time, then the property is assigned as follows:

Variant::set_named() -> Object::set() -> GDScriptInstance::set()

And Object.set() does not report errors, according to the docs:

Assigns value to the given property. If the property does not exist or the given value's type doesn't match, nothing happens.

[AThousandShips]

The code that's causing the leak is this:

godot/modules/gdscript/gdscript.cpp

Lines 1605 to 1612 in 29b3d9e

	if (member->data_type.has_type && !member->data_type.is_type(value)) {
	const Variant *args = &p_value;
	Callable::CallError err;
	Variant::construct(member->data_type.builtin_type, value, &args, 1, err);
	if (err.error != Callable::CallError::CALL_OK || !member->data_type.is_type(value)) {
	return false;
	}
	}

Something about how it handles conversions here means the value leaks, even though at the end of the conversion value is null

Can't quite find what in the code is broken exactly, probably something in:

godot/core/variant/variant_construct.cpp

Lines 331 to 348 in 29b3d9e

	void VariantInternal::object_assign(Variant *v, const Object *o) {
	if (o) {
	if (o->is_ref_counted()) {
	RefCounted *ref_counted = const_cast<RefCounted *>(static_cast<const RefCounted *>(o));
	if (!ref_counted->init_ref()) {
	v->_get_obj().obj = nullptr;
	v->_get_obj().id = ObjectID();
	return;
	}
	}
	v->_get_obj().obj = const_cast<Object *>(o);
	v->_get_obj().id = o->get_instance_id();
	} else {
	v->_get_obj().obj = nullptr;
	v->_get_obj().id = ObjectID();
	}
	}

Not handling something correctly

[AThousandShips]

It seems that it's due to how the construction of an object doesn't correctly assign type, so it's a Variant with type NIL but the object is still assigned

Will write up a fix, new changes makes it error on runtime, and doesn't leak