godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
86.4k stars 19.24k forks source link

Calling a class method on NOTIFICATION_PREDELETE will crash the editor #80834

Open Lemonymous opened 10 months ago

Lemonymous commented 10 months ago

Godot version

v4.2.dev.custom_build [eca6f0eb5]

System information

Windows 10, Forward+, NVIDIA GeForce GTX 1050 Ti (31.0.15.3179)

Issue description

Calling a class method from _notification on NOTIFICATION_PREDELETE will crash the Godot editor. It seems self is <Object#null> at that time. Right before the Godot editor closes, it will say something about "attempting to call _on_predelete on a null instance" or something similar.

This makes sense, since self is null.

However, it seems like a bug that the object is a null instance on predelete, when you would want to clean up resources that the object was responsible for.

Steps to reproduce

Create a node that creates an object that will be deleted at the end of the function:

extends Node

func _ready():
    var test_ref_counted = TestRefCounted.new()

Create a RefCounted object which runs some code on predelete:

  1. The following will correctly print "predelete"
    
    class_name TestRefCounted
    extends RefCounted

func _notification(what): if what == NOTIFICATION_PREDELETE: print("predelete")

2. The following will print <Object#null>
```gdscript
class_name TestRefCounted
extends RefCounted

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        print(self)
  1. The following will crash the Godot editor, because you attempt to call a function on a null object.
    
    class_name TestRefCounted
    extends RefCounted

func _on_predelete(): pass

func _notification(what): if what == NOTIFICATION_PREDELETE: _on_predelete()



### Minimal reproduction project

This MRP has the code in point 3 in "Steps to reproduce", and will crash the Godot editor if ran.
[MRP-predelete.zip](https://github.com/godotengine/godot/files/12389286/MRP-predelete.zip)
Rindbee commented 10 months ago

Related to #6784.

bitsawer commented 8 months ago

Interestingly, it seems like you can still access script member variables during NOTIFICATION_PREDELETE even if self is null, maybe they are still available in some local script scope etc.? Someone could test this with ASAN enabled just in case:

class_name TestRefCounted
extends RefCounted

var member_variable = "you can print this one"

func _on_predelete():
    pass

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
                print(self) #Prints null
        print(member_variable) #Ok
        member_variable = "something else" #Ok
        print(member_variable) #Ok
        _on_predelete() #Error here
lostminds commented 3 months ago

I ran into this issue today on 4.3dev4 as I was converting some custom Object classes that I had manually freed using free() to RefCounted to get automatic reference counting and freeing, and I wanted to see that the automatic freeing was happening as expected.

The interesting thing is that when the objects were just Object subclasses and I called free() they would receive the NOTIFICATION_PREDELETE notification as expected before being freed so I could do some additional cleanup. And I would expect RefCounted to just maintain a count of references and then call free() when this reaches zero, but perhaps it doesn't? When they are RefCounted subclasses they still get the NOTIFICATION_PREDELETE notification, but it seems that the objects have already been freed and are null when this happens.

Seems like this bug https://github.com/godotengine/godot/issues/6784 for 3.x Reference is still present in the 4.x RefCounted class.

dev-mico commented 2 weeks ago

Yep, having the same issue here. Hope this gets fixed soon!