C#: Fix NOTIFICATION_PREDELETE does not trigger ExitTree

[magian1127]

Fix #82279
It tests okay on Windows, and it tests using the program from #78634 and Godot.exe --test pass.

[akien-mga]

Despite the title this is a core change which can have significant implication for all languages, not just C#.

What's the problem with C# specifically that wouldn't affect GDScript or GDExtension? Or is there a more generalized problem?

[magian1127]

In C#, QueueFree doesn't trigger ExitTree,this is a serious bug for C #, but it does work in GDS.
I've been testing for a while, and so far, I haven't found this change affecting other languages.
For the functionality in PR #78634, I have tested it with the provided test cases, and they all passed the tests.

[magian1127]

I tested again, #78634 sorting in GDE is broken.

[Sauermann]

This PR reverts parts of #78634 and when this PR gets merged in its current form, the following issues need to be reopened, because they will be reintroduced:

• Object::notification() doesn't fully respect its p_reversed parameter #52325
• Include extensions in the call ordering of notifications godot-proposals#5585

An alternative approach (and my suggestion) would be to investigate in which steps the attached C# script gets forgotten and make necessary adjustments there, so that the scripts are still remembered, when they get called.

[magian1127]

@Sauermann @akien-mga
I changed the implementation method, only modifying C#.
Now, the reversed postpones CallDispose until NOTIFICATION_EXIT_TREE.
can now remove the regression tag.

[bitsawer]

Possibly related issues:

• Calling a class method on NOTIFICATION_PREDELETE will crash the editor #80834
• Custom class instance extending Reference already freed when it receives NOTIFICATION_PREDELETE #6784

[raulsntos]

This looks incorrect. Now it won't call Dispose when the notification call is reversed, at least until we receive notification 11 (which is NOTIFICATION_EXIT_TREE but we shouldn't be using an integer literal). However, this notification is only sent for Nodes, so that means Resources and RefCounteds won't be disposed now.

I don't think I understand issue #82279. Does it mean that NOTIFICATION_PREDELETE is being sent before NOTIFICATION_EXIT_TREE? That doesn't make sense to me, shouldn't the predelete notification be always the last one? The documentation says it can be used as a "destructor", so we should be able to dispose the C# object when receiving it.

Also, the issues mentioned by @bitsawer seem to affect GDScript as well, so maybe the issue is not exclusive to C# and then it should be fixed in Core for all languages.

[magian1127]

csharp_script.cpp instance_create()

return _create_instance(nullptr, 0, p_this, Object::cast_to<RefCounted>(p_this) != nullptr, unchecked_error);

Here the code shows that the RefCounted type sets base_ref_counted to true.
So in CSharpInstance::notification, the old code would not execute CSharpInstanceBridge_CallDispose.

The code comments also make it clear that RefCounted are not handled here.

[magian1127]

After testing, it is not necessary to evaluate p_notification here.

[raulsntos]

So I've looked a bit more into this, and this is what I've gathered.

NOTIFICATION_PREDELETE is sent when an Object is about to be deleted (called by memdelete). Then, Node overrides _notification and when it receives that notification it removes itself from its parent, thus sending NOTIFICATION_EXIT_TREE. Both notifications are sent with p_reversed set to true.

Since NOTIFICATION_PREDELETE is sent reversed, in a hierarchy like: Object > Node > Node2D the _notification method will be called first on Node2D, then Node. This means that NOTIFICATION_EXIT_TREE will be sent after the derived classes have received NOTIFICATION_PREDELETE. So in this case Node2D cleans up before receiving NOTIFICATION_EXIT_TREE.

I can imagine this being a problem. For example, if Node2D had some state that it clears when receiving NOTIFICATION_PREDELETE, then it won't be able to use it when receiving NOTIFICATION_EXIT_TREE. This looks backwards to me, I think NOTIFICATION_PREDELETE should always be the last one. Maybe we shouldn't send notifications inside NOTIFICATION_PREDELETE.

In the case of CSharpInstance, this means it will dispose the script before Node has a chance to send NOTIFICATION_EXIT_TREE, so by the time the notification is received there's no script to send it to. We want to make sure the C# script class is disposed always after all the classes in the hierarchy have received NOTIFICATION_PREDELETE and cleaned up (to ensure Node has sent NOTIFICATION_EXIT_TREE before disposing).

The current PR implementation is not good because it relies on receiving another notification after NOTIFICATION_PREDELETE which may work for Node because we know it will send NOTIFICATION_EXIT_TREE after, but I don't think we can rely on this behavior working for every class. Also, since it disposes the script in the next notification, if there are other notifications only the first one will be received.

Maybe we could not dispose at all when receiving NOTIFICATION_PREDELETE and let the CSharpInstance destructor take care of it. But I think this is a risky change for 4.2, there may be consequences to this that I'm not able to see right now.

Alternatively, we could change Object::notification so that it calls ScriptInstance::notification always after _notificationv. This would mean for a hierarchy like Object > Node > Node2D > ScriptA > ScriptB, the notifications will be sent in the order Node2D > Node > ScriptB > ScriptA. Unfortunately, as mentioned earlier, this would reintroduce #52325.

We could also add a new notification that is sent by memdelete right after sending NOTIFICATION_PREDELETE and it would be used by CSharpInstance to dispose C# scripts, or by any other class that needs to ensure the cleanup happens after everything else. It feels weird because I think that's already the purpose of NOTIFICATION_PREDELETE, but the idea is that we wouldn't send any notifications inside the new one to ensure it really is the last notification. And this seems easier than refactoring all the existing code that uses NOTIFICATION_PREDELETE to not send notifications (like Node does now with NOTIFICATION_EXIT_TREE).

[magian1127]

The inability to trigger ExitTree has already seriously affected the continued use of Godot. If the current PR solution is not suitable, hope that others can provide a better one.

[magian1127]

I think the solution of #83670 is good, but I still updated this PR, added a script_is_node variable, just as an extra reference for everyone.