Custom class instance extending Reference already freed when it receives NOTIFICATION_PREDELETE

[PrestonKnopp]

Operating system or device - Godot version:

macOS 10.11.6 - v2.1.stable.official

Issue description (what happened, and what was expected):

Created subclass, overrides _notification. When the instance should be sent NOTIFICATION_PREDELETE (the only notification Reference gets?) and freed the game crashes.

Expected to receive notification.

Steps to reproduce:

• Create subclass of Reference and override _notification
• Create an instance.
• Set it to null, or let go out of scope.
• Then crash.

Link to minimal example project (optional but very welcome):

godot-ref-test.zip

[neikeq]

I experienced this crash today when doing some tests. This is the cause:

• The refcount reaches 0 therefore memdelete is called.
• memdelete calls _predelete_handler which ends up calling _notification(NOTIFICATION_PREDELETE) on the script instance.
• GDFunction::call tries to reference the instance but, at this point, the refcount is already 0, so atomic_conditional_increment won't increase it and this condition will fail resulting in the Ref::reference pointer never being initialized, therefore undefined behavior.

ping @reduz

[reduz]

I think we discussed this before, and the proposed solution was to simply not send that notification to the script, as it does not make much sense. Was this implemented somehow?

[neikeq]

No, it wasn't. You proposed not calling NOTIFICATION_PREDELETE for scripts. I don't have anything against it, but I wonder if there is people who rely on this notification.

[reduz]

Ah, alright, then will simply do that.. should be enough

On Aug 5, 2017 2:43 PM, "Ignacio Etcheverry" notifications@github.com wrote:

No, it wasn't. You proposed not calling NOTIFICATION_PREDELETE for scripts. I don't have anything against it, but I wonder if there is people who rely on this notification.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6784#issuecomment-320458118, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z27RcQ2dFcYdvi3L_s0z0vkjp_Hdrks5sVKnCgaJpZM4KTJ2c .

[karroffel]

ups.

[neikeq]

@karroffel This is what happens when you switch from rats to squids.

[neikeq]

@reduz Maybe, instead of disabling NOTIFICATION_PREDELETE for scripts, we could change this condition to check for static_cast<Reference *>(p_instance->owner)->reference_get_count() > 0. Actually, shouldn't is_referenced() not only check if the refcount is initialized, but also check if the refcount is not zero?

[reduz]

I would disable the predelete notification in Mono, not needed in the language and no need for hacks. in any case, this is not serious so kicking to 3.1

[neikeq]

I can disable it in Mono if not really needed. But this is a GDScript issue. What happened with 65cc56c? I can't see it in the history.

[akien-mga]

commit 54106627203ec11259424a0b5c5ee655ac219fa8
Author: Juan Linietsky <reduzio@gmail.com>
Date:   Mon Nov 13 14:01:32 2017 -0300

    Revert this, karroffel makes sense that this should be implemented per language.

commit 2e3a1caa069c3eddfbf0b890bff7f7d85925b476
Author: Juan Linietsky <reduzio@gmail.com>
Date:   Mon Nov 13 13:40:07 2017 -0300

    Disable OpenGL warnings unless running with -v, closes #7171

commit 65cc56c35da357ac7799ed9616aba4898f17f232
Author: Juan Linietsky <reduzio@gmail.com>
Date:   Mon Nov 13 13:36:09 2017 -0300

    Do not send NOTIFICATION_PREDELETE to scripts, as it can cause problmes in garbage collected languages. Closes #6784

[reduz]

So, what do we do with this?

[neikeq]

¯_(ツ)_/¯

I was re-reading the part of the code I mentioned above is causing the problem, and I noticed we could change the is_referenced() condition with is_referenced() && reference_get_count() > 0 (actually is_reference() should be checking that, considering its name...). This way the condition would fail, and self would be assigned Object * instead of Ref<T>.

https://github.com/godotengine/godot/blob/53070437514e448c87f6cb31cf5b27a3839dbfa1/modules/gdscript/gdscript_function.cpp#L368-L374

However, it's still not safe, during this call, to pass self to the engine. Variant will only store Object * not RefPtr, so it cannot be used to construct a Ref<T>. The following code will also be unable to construct a Ref<T>:

Reference *ref = Object::cast_to<Reference>(obj); // obj is the `self` we passed to the engine
if (ref) {
    Ref<Reference> r(ref);
}

It may be simply better to not send NOTIFICATION_PREDELETE to Reference types. It's okay for other types, and Node actually requires it for children lifetime management.

[Xrayez]

I stumbled upon this issue in 3.2, linking a more thorough and updated reproduction project: predelete-instance-deleted.zip

Also wonder if there's any connection with #17681.

[Xrayez]

The motivation to run predelete on Reference is when I want to free some nested data structures:

class_name DoublyLinkedList
extends Reference

var front : Element
var back : Element

# implementation details...

func clear():
    var e = front
    while e:
        var next = e.next
        e.free()
        e = null
        e = next

func _notification(what):
    match what:
        NOTIFICATION_PREDELETE:
            clear() # hmm, instance is gone, can't free elements

# implementation details...

class Element extends Object:
    var data
    var next : Element
    var prev : Element

    func _init(p_data):
        data = p_data

Extending Reference for Element works in this case but I'd like to make it more lightweight., Might need to call unreference() manually? 🤔

[KoBeWi]

I tried the repro in 19f72beeb and neither object seems to receive NOTIFICATION_PREDELETE...

The issue is still reproducible in 3.2.3 though.

[CsloudX]

Hi, freinds, I need this notification for my code.

class_name Tree

class TreeNode:
    var data
    var parent : TreeNode
    var left : TreeNode
    var right : TreeNode

var head : TreeNode

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        clear() # Crashed, so I can't release all the TreeNode auto

func clear():
    free_node(head)

func free_node(node:TreeNode):
    if node==null:
        return
    if node.left:
        free_node(node.left)
        node.left = null
    if node.right:
        free_node(node.right)
        node.right = null

Any update for this issue?

[CsloudX]

The issue is still reproducible in 4.2 though.

[Wesmer]

Ok as a thread necromancer, it still doesn't work in version v4.2.1.stable.official [b09f793f5] (what a surprise).

As a hint for the people with the same problem: #82841 shows a workaround for the problem. Apparently script variables are not yet deleted at that point, so they can still be used. So just save everything to variables and the problem is solved, I suppose.

Big question: Why are script variables still valid, but the self pointer is not?

Finally, a very uneducated suggestion: Can the refcounter emit the notification just before the counter jumps to 0? Even if it should mean that - if you want to have a deconstructor (with a valid self pointer) - you have to inherit from RefCouned?

[ghost]

Unfortunately, I have to confirm what @Wesmer says. The NOTIFICATION_PREDELETE can, in fact, not be used as a destructor as described in the documentation because you no longer have access to class functions.

[dev-mico]

Bumping this issue, as I'm having the same problem. Has anyone found any good workarounds?

[Wesmer]

Bumping this issue, as I'm having the same problem. Has anyone found any good workarounds?

I played around a bit. (Version: v4.2.1.stable.official [b09f793f5]) It seems to help to save the self-pointer. @dev-mico let me know if it has solved your problem.

extends Node

class TEST extends Texture2D:
    var test_class = "class_abc"
    var this: TEST = self

    func _init() -> void:
        # Decrement the internal reference counter again
        # because this = self increments it
        unreference()

    func _notification(what):
        if what == NOTIFICATION_PREDELETE:
            print(test_class) # OK class_abc
            print(self.test_class) # OK class_abc
            print(self, self == null) # null , true
            print(this.has_alpha()) # OK true

            # print(self.has_alpha()) # crash
            # print(has_alpha()) # crash

            this.test_method() # OK hi

    func test_method():
        print("hi")

func _ready() -> void:
    var t = TEST.new()

[ghost]

That's awesome @Wesmer ! It also doesn't seem to cause any memory leaks. (Also tested with resource monitor on Windows.)

extends Node

class RefTest extends RefCounted:
    var foo := "bar"
    var this : RefTest = self

    func _init() -> void:
        unreference()

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

func _ready() -> void:
    print(OS.get_static_memory_usage())
    for i in range(10000000):
        var r := RefTest.new()
    print(OS.get_static_memory_usage()) # Outputs roughly the same as above.

I'd be a bit worried about using this in production though as it seems to rely on undocumented engine behavior?

[Wesmer]

I'd be a bit worried about using this in production though as it seems to rely on undocumented engine behavior?

Well, it depends on how much you like hacky engineering.

If you absolutely don't like the solution, use a node/objekt, as this problem (apparently) only applies to RefCounted-Objects.

extends Node

func deconstructor_code():
    prints("DECONSTRUCTOR", self) # valid
    is_queued_for_deletion() # ok

func _notification(what):
    if what == NOTIFICATION_PREDELETE:
        prints("Node deconstructor", self) # self is Valid

        deconstructor_code() # OK
        self.get_script() #OK

Or use a second class:

extends Node

class Cleaner extends RefCounted:
    func deconstructor_code():
        prints(" Cleaner DECONSTRUCTOR", self) # valid
        self.get_class() # ok
        some_method() # OK

    func some_method():
        pass

class SomeClass extends RefCounted:
    var t = Cleaner.new()

    func _notification(what):
        if what == NOTIFICATION_PREDELETE:
            prints("Objekt deconstructor", self) # unvalid
            t.deconstructor_code() # ok

func _ready() -> void:
    var t = SomeClass.new()

It is best to avoid the deconstructor of Refcounted objects. Otherwise you will probably not be able to avoid a workaround.

However, the two-class solution is better in the sense that it avoids tinkering with the counter. Because if you forget unreference(), in the worst case you have a 6-hour debug session ahead of you.