C#: Check if .NET runtime has been finalized in the instance binding reference callback

[RedworkDE]

Fixes #77305

Turns out you can't detach an instance binding from an object, so duplicate a check from _instance_binding_free_callback.

[akien-mga]

CC @raulsntos

[raulsntos]

I'm not very familiar with the reference count system so take this review with a grain of salt. In general it makes sense to me that if the .NET runtime is finalized, the reference_callback implementation would return true and let the object be freed.

[neikeq]

I'm preemptively blocking this from being merged before I review it tomorrow. But in advance I can say there's probably something wrong elsewhere as instance bindings should have been cleaned up before/after finalizing.

[RedworkDE]

But in advance I can say there's probably something wrong elsewhere as instance bindings should have been cleaned up before/after finalizing.

Yes, arguably the instance binding should have been detached from objects before

godot/modules/mono/csharp_script.cpp

Line 165 in f9fc77b

script_bindings.clear();

but there is no way of doing that currently and thus I went for the simple crash fix that doesn't require core changes.

Edit: alternatively #78157 also fixes the crash by moving the OnGodotShuttingDown to before the bindings cleanup, tho if there are cases where bound objects survive that this (or a similar) patch may still be needed.

[neikeq]

The problem is indeed somewhere else. We cannot remove the script bindings, but that's what the script_binding.inited. This method checks that, so for some reason it's not cleaned up. Probably a script binding is being created again after all the bindings were finalized, which would explain why #78157 fixes the issue.

[neikeq]

Wait, I just realized we check script_binding.inited after CRASH_COND(!rc_owner);. Does moving it to check that first work?

[RedworkDE]

Does moving it to check that first work?

When these issues occur p_binding is a dangling pointer to freed memory and we can no longer safely check script_binding.inited, so no that doesn't help.

I would use #78157 to fix the issue (It gracefully deletes the managed objects before dropping the instance bindings map) and then either keep this PR as-is as backup or just close this as superseded.

[raulsntos]

If #78157 fixes the issue, then I think I'm leaning towards closing this PR as superseded.

If, as you say, some bound objects survive after OnGodotShuttingDown, then it may be preferable to let Godot crash instead of potentially hiding an issue somewhere else.

As I understand it from the previous conversation, the bound objects should have been cleared before reaching _instance_binding_free_callback so if any bound objects survive it's likely a bug in the .NET module that should be fixed.

[RedworkDE]

Superseded by #78157