C#: Fix RefCounted not disposed correctly in certain case

[zaevi]

• Fixes: [.Net] Handle is not initialized. when assigning TweenMethod to a later-disposed Tween #100691

When the reference count is decreased to 1 and the current GCHandle is strong, it is assumed that the RefCounted is only referenced by managed side, so the strong handle needs to be replaced with a weak handle to ensure that the RefCounted can be correctly GCed.

See:

godot/modules/mono/csharp_script.cpp

Lines 1233 to 1309 in 2582793

	GDExtensionBool CSharpLanguage::_instance_binding_reference_callback(void *p_token, void *p_binding, GDExtensionBool p_reference) {
	// Instance bindings callbacks can only be called if the C# language is available.
	// Failing this assert usually means that we didn't clear the instance binding in some Object
	// and the C# language has already been finalized.
	DEV_ASSERT(CSharpLanguage::get_singleton() != nullptr);
	CRASH_COND(!p_binding);
	CSharpScriptBinding &script_binding = ((RBMap<Object *, CSharpScriptBinding>::Element *)p_binding)->get();
	RefCounted *rc_owner = Object::cast_to<RefCounted>(script_binding.owner);
	#ifdef DEBUG_ENABLED
	CRASH_COND(!rc_owner);
	#endif
	MonoGCHandleData &gchandle = script_binding.gchandle;
	int refcount = rc_owner->get_reference_count();
	if (!script_binding.inited) {
	return refcount == 0;
	}
	if (p_reference) {
	// Refcount incremented
	if (refcount > 1 && gchandle.is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
	// The reference count was increased after the managed side was the only one referencing our owner.
	// This means the owner is being referenced again by the unmanaged side,
	// so the owner must hold the managed side alive again to avoid it from being GCed.
	// Release the current weak handle and replace it with a strong handle.
	GCHandleIntPtr old_gchandle = gchandle.get_intptr();
	gchandle.handle = { nullptr }; // No longer owns the handle (released by swap function)
	GCHandleIntPtr new_gchandle = { nullptr };
	bool create_weak = false;
	bool target_alive = GDMonoCache::managed_callbacks.ScriptManagerBridge_SwapGCHandleForType(
	old_gchandle, &new_gchandle, create_weak);
	if (!target_alive) {
	return false; // Called after the managed side was collected, so nothing to do here
	}
	gchandle = MonoGCHandleData(new_gchandle, gdmono::GCHandleType::STRONG_HANDLE);
	}
	return false;
	} else {
	// Refcount decremented
	if (refcount == 1 && !gchandle.is_released() && !gchandle.is_weak()) { // The managed side also holds a reference, hence 1 instead of 0
	// If owner owner is no longer referenced by the unmanaged side,
	// the managed instance takes responsibility of deleting the owner when GCed.
	// Release the current strong handle and replace it with a weak handle.
	GCHandleIntPtr old_gchandle = gchandle.get_intptr();
	gchandle.handle = { nullptr }; // No longer owns the handle (released by swap function)
	GCHandleIntPtr new_gchandle = { nullptr };
	bool create_weak = true;
	bool target_alive = GDMonoCache::managed_callbacks.ScriptManagerBridge_SwapGCHandleForType(
	old_gchandle, &new_gchandle, create_weak);
	if (!target_alive) {
	return refcount == 0; // Called after the managed side was collected, so nothing to do here
	}
	gchandle = MonoGCHandleData(new_gchandle, gdmono::GCHandleType::WEAK_HANDLE);
	return false;
	}
	return refcount == 0;
	}
	}

It is expected in most cases, but there is one case that is just the opposite: The RefCounted has only one reference in unmanaged side, and the managed reference is about to be disposed. In this case, after replacing the GCHandle, can't use old handle to release managed binding.

This fix checks if it is this case when the managed reference is disposing, if so, use the replaced GCHandle to release binding.

[zaevi]

Minimal reproduction:

public override void _Ready()
{
	var rc = new RefCounted();
	rc.Reference(); // 1 -> 2

	var id = rc.GetInstanceId();
	rc.Dispose(); // 2 -> 1 (not release binding correctly)

	var rc2 = InstanceFromId(id) as RefCounted;

	// rc2.GetReferenceCount(); // ObjectDisposedException
	Debug.Assert(ReferenceEquals(rc, rc2) == false); // Fail
}

[raulsntos]

Thanks! Tested with the provided MRPs and works as expected. The code also looks good to me, thanks to your explanation on RC:

Passed in correctly at first, until rc->unreference that swap gchandle:

godot/modules/mono/glue/runtime_interop.cpp

Line 197 in e1b4101

if (rc->unreference()) {

I originally missed the call to rc->unreference() which causes the GCHandle to swap, and explains why p_gchandle_to_free becomes invalid in the middle of this function, even though we just retrieved it in:

godot/modules/mono/glue/GodotSharp/GodotSharp/Core/GodotObject.base.cs

Line 124 in e1b4101

IntPtr gcHandleToFree = NativeFuncs.godotsharp_internal_object_get_associated_gchandle(NativePtr);

[Repiteo]

Thanks!