Deleted Objects create null pointer segmentation faults in release mode making it close to impossible to cater for

[chucklepie]

Godot version:
3.2.3

OS/device including version:
Linux

Issue description:
I have an array of nodes. In this very simple test loop that iterates through them:

		for e in GlobalTileMap.active_room_enemies:
			if e == null:
				print("A deleted item.")
                        else:
				print("Item Object " + str(e))                          #1
				print("Item  Name " + str(e.name))               #2

During a debug mode release everything works fine and it finds one null node and skips it as per the code. But during release mode with the same data it finds the null and skips as expected but then I get a segmentation fault (detailed below) due to a null pointer and after hours of looking it is because at comment 1 it is returning this for one other node that must have been freed at some previous time:

[Deleted Object]

Then when it gets to comment 2 it crashes the program, terminates with the segmentation fault and quits the game. I can only presume release mode ran a bit faster so the 'deleted' object didn't have time to turn to null. However when I force this during testing in debug mode I simply get the error in the console, 'attempt to call free on a previously freed instance' as you would expect.

I found a workaround using is_instance_valid(e), but clearly you cannot wrap this method around every single call in case your object is 'deleted' by chance by some other part of your code via a signal, an event, or whatever.

Surely GDScript should know it's a deleted object and stop this from happening?

But if not, what is the point in having two variations of essentially null objects and one of them masquerades as a normal non-null object until you try and access it then you get a segmentation fault and your code terminates badly.

Is there anything that can or is being done to stop this?

btw, the fault is pretty generic I'm afraid:
0x00000002067ce200 in ?? ()
(gdb) backtrace
#1 0x0000000000a157f2 in ClassDB::get_property(Object*, StringName const&, Variant&) ()
#19 0x0000000000a2a36e in Object::get(StringName const&, bool*) const ()
#29 0x00000000008e03ea in Variant::evaluate(Variant::Operator const&, Variant const&, Variant const&, Variant&, bool&) ()
#33 0x000000000089c0c4 in Variant::iter_next(Variant&, bool&) const ()
#43 0x00000000008d6520 in Variant::get_named(StringName const&, bool*) const ()

Surely one of those methods should be testing if a node is a 'deleted object'?

Minimal reproduction project:
I really don't know where to start with this, I'm just dropping the issue in case somebody can look at this fault without a recreatable piece of code because segmentation faults only at runtime caused by 'deleted objects' is a pretty bad way to have an interpreted script language I think...

Thanks.

[KoBeWi]

I found a workaround using is_instance_valid(e), but clearly you cannot wrap this method around every single call in case your object is 'deleted' by chance by some other part of your code via a signal, an event, or whatever.

This is what is_instance_valid() is for. It also returns false when you call it with null object, so you can just replace all your null checks with is_instance_valid() and it will work.

[chucklepie]

Yes, but when my object is null and I try to access it it is fine, when my object is 'deleted' and I do it during 'debug' it is fine. Only when I do it in release mode and when it is 'deleted' do I get a stack fault.

debug null = ok
debug deleted = ok
release null = ok
release deleted = segmentation fault

That isn't consistent is it, and does GDScript not perform any kind of check because it must be when null objects don't fault.

Maybe a nice article in the documentation stating never use null might do or is there a reason why only deleted objects fail at runtime?....

[KoBeWi]

Eh so I checked this code:

func _ready():
	var a = Button.new()
	a.free()
	if a:
		a.text = "test"

And it properly throws error. So, like, freed nodes evaluate to true, but when you compare them to null that's also true. I mean:

func _ready():
var a = Button.new()
a.free()
if a == null:
a.text = "test"

No error here.
(forget this)

So this is actually a bug.~~

EDIT:
Wait nevermind. I used wrong condition/Godot version, this is actually fine.

[akien-mga]

debug null = ok
debug deleted = ok
release null = ok
release deleted = segmentation fault

This is fixed in 3.2.4, now you should get an error in debug mode (and a crash in release, that's expected - the error in debug needs to be fixed in your code).

[KoBeWi]

Ok closing. This bug exists only in 3.2.3.

[chucklepie]

I never get the error at debug, maybe it's a timing issue?

But from your previous statement you're saying it's a bug depending whether you call 'is a' or 'a==null'?

[KoBeWi]

No disregard my previous statement, I checked in wrong version. In 3.2.3 freed objects evaluate to null. This wasn't a case in older versions (or was unreliable), but it's fixed in 3.2.4.

[chucklepie]

ok, but given this can still causes issues with a bad null check and the last thing anyone wants in GDScript is an internal null pointer error, how much traction would a suggestion to turn 'is_instance_valid' into an operator?

Python has no null operator because introducing null to a language is something every language creator regrets...

Writing 'if object == null' types easily and fluently, but 'if is_instance_valid(object)' requires more typing, brackets, etc and is pretty tedious.

How about 'if object == None' where 'deleted' translates to is_instance_valid?

[KoBeWi]

Writing 'if object == null' types easily and fluently, but 'if is_instance_valid(object)' requires more typing, brackets, etc and is pretty tedious.
How about 'if object == None' where 'deleted' translates to is_instance_valid?

You can open a proposal about it: https://github.com/godotengine/godot-proposals

[chucklepie]

@KoBeWi Have done, thanks.

But may I ask a question to save me loads of work. In code when you retrieve a node via a call, e.g. get_node()/find_node(), will it return a 'deleted' node? i.e. can I safely perform a null check in the same statement block after this without having to wrap the api method call?

[KoBeWi]

get_node() will never return deleted node. When a node is freed, it's removed from scene tree before being invalidated, so you can't access it if you don't have reference.

[chucklepie]

Thanks, that removes a lot of work for me then, the bulk of my null checks are in evil singletons ;)