C# `_GetPropertyList` return a Callable to the `Clear` method, instead of a custom-made `Clear` property

[aroelke]

Tested versions

v4.2.1.stable.mono.official [b09f793f5]

System information

Godot v4.2.1.stable.mono - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 31.0.15.3640) - Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz (12 Threads)

Issue description

When exporting a long list of properties in a custom Resource in C# using _GetPropertyList, that also implements the IDictionary interface, I encounter the following error whenever I try to create an instance of the Resource or open one in the inspector:

ERROR: BUG: Unreferenced static string to 0: Clear
   at: unref (core/string/string_name.cpp:129)

I don't know the exact number of properties that causes this, but I know it's more than 23 (the number of constants defined by the JoyButton enum) and no more than 193 (the number of constants defined by the Key enum).

Steps to reproduce

Create a new C# script that extends Resource and IDictionary, then override _GetPropertyList, _Get, and _Set in the new Resource such that the number of properties is large (making it 193 items, such as by using the names of the Key enum constants, will trigger this).

Build, then create an instance of the new Resource. After a few seconds, the above error will appear in the output window and Godot may crash. In my experience, deleting the %APPDATA%/Godot directory temporarily fixes the crash, but eventually it will start crashing every time this error occurs.

Minimal reproduction project (MRP)

Test.zip

[austin226]

I was able to repro on 4.2.1.stable.mono with Linux Mint x64.

[austin226]

Running from command line with 4.3.dev.mono at 2ababdcc065e76decc12747a3c84e01f7bdc90dd:

ERROR: BUG: Unreferenced static string to 0: Clear
   at: unref (core/string/string_name.cpp:129)
ERROR: Trying to unreference a SafeRefCount which is already zero is wrong and a symptom of it being misused.
Upon a SafeRefCount reaching zero any object whose lifetime is tied to it, as well as the ref count itself, must be destroyed.
Moreover, to guarantee that, no multiple threads should be racing to do the final unreferencing to zero.
   at: _check_unref_safety (./core/templates/safe_refcount.h:187)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.3.dev.mono.custom_build (2ababdcc065e76decc12747a3c84e01f7bdc90dd)
Dumping the backtrace. Please include this when reporting the bug to the project developer.

[1] /home/austin/.dotnet/shared/Microsoft.NETCore.App/8.0.1/libcoreclr.so(+0x5e76f5) [0x7f7b656ed6f5] (??:0)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f7b87cd4520] (??:0)
[3] StringName::unref() (/home/austin/dev/godot-src/./core/templates/safe_refcount.h:187)
[4] StringName::~StringName() (/home/austin/dev/godot-src/./core/string/string_name.h:195)
[5] Callable::~Callable() (/home/austin/dev/godot-src/core/variant/callable.cpp:384)
[6] Variant::_clear_internal() (/home/austin/dev/godot-src/core/variant/variant.cpp:1374)
[7] Variant::clear() (/home/austin/dev/godot-src/./core/variant/variant.h:305)
[8] Variant::~Variant() (/home/austin/dev/godot-src/./core/variant/variant.h:802)
[9] CSharpInstance::get(StringName const&, Variant&) const (/home/austin/dev/godot-src/modules/mono/csharp_script.cpp:1654 (discriminator 1))
[10] Object::get(StringName const&, bool*) const (/home/austin/dev/godot-src/core/object/object.cpp:318)
[11] EditorProperty::_get_cache_value(StringName const&, bool&) const (/home/austin/dev/godot-src/editor/editor_inspector.cpp:780)
[12] EditorProperty::is_cache_valid() const (/home/austin/dev/godot-src/editor/editor_inspector.cpp:787 (discriminator 3))
[13] EditorInspector::_notification(int) (/home/austin/dev/godot-src/editor/editor_inspector.cpp:3992 (discriminator 1))
[14] EditorInspector::_notificationv(int, bool) (/home/austin/dev/godot-src/editor/editor_inspector.h:460 (discriminator 14))
[15] Object::notification(int, bool) (/home/austin/dev/godot-src/core/object/object.cpp:840)
[16] SceneTree::_process_group(SceneTree::ProcessGroup*, bool) (/home/austin/dev/godot-src/scene/main/scene_tree.cpp:951)
[17] SceneTree::_process(bool) (/home/austin/dev/godot-src/scene/main/scene_tree.cpp:1023 (discriminator 2))
[18] SceneTree::process(double) (/home/austin/dev/godot-src/scene/main/scene_tree.cpp:510)
[19] Main::iteration() (/home/austin/dev/godot-src/main/main.cpp:3790)
[20] OS_LinuxBSD::run() (/home/austin/dev/godot-src/platform/linuxbsd/os_linuxbsd.cpp:933)
[21] /home/austin/dev/godot-src/bin/godot.linuxbsd.editor.dev.x86_64.mono(main+0x19f) [0x55acfb15f268] (/home/austin/dev/godot-src/platform/linuxbsd/godot_linuxbsd.cpp:76)
[22] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7b87cbbd90] (??:0)
[23] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f7b87cbbe40] (??:0)
[24] /home/austin/dev/godot-src/bin/godot.linuxbsd.editor.dev.x86_64.mono(_start+0x25) [0x55acfb15f005] (??:?)
-- END OF BACKTRACE --

[austin226]

Actually, the issue doesn't seem to be strictly with the number of entries in the dictionary. If I replace the line

 private readonly Dictionary<string, int> _dict =
        Enum.GetNames<Key>().ToDictionary((n) => n, _ => Enum.GetValues<Key>().Length);

with

 private readonly Dictionary<string, int> _dict =
        Enumerable.Range(1, 200).ToDictionary(i => i.ToString(), i => i);

then it seems to work fine.

[austin226]

If I remove the entry with key "Clear" from the dictionary, the issue goes away:

    private static Dictionary<string, int> BuildDictionary()
    {
        var d = new Dictionary<string, int>();
        foreach (var k in Enum.GetNames<Key>())
        {
            if (k != "Clear")
            {
                d[k] = 193;
            }
        }

        return d;
    }

    private readonly Dictionary<string, int> _dict = BuildDictionary();

[austin226]

And in fact, using a dictionary that only has a single key "Clear" causes the issue to appear again:

    private static Dictionary<string, int> BuildDictionary()
    {
        var d = new Dictionary<string, int>();
        d["Clear"] = 193;

        return d;
    }

    private readonly Dictionary<string, int> _dict =
        BuildDictionary();

[aroelke]

It also doesn't seem to be dependent on implementing IDictionary, either. This code causes it as well:

[GlobalClass, Tool]
public partial class TestResource : Resource
{
    private Dictionary<string, int> _dict = new() {{ "Clear", 100 }};

    public override Godot.Collections.Array<Godot.Collections.Dictionary> _GetPropertyList() =>
        new(_dict.Select((e) => new Godot.Collections.Dictionary()
        {
            { "name", e.Key },
            { "type", Variant.From(Variant.Type.Int) }
        }));

    public override Variant _Get(StringName property) =>
        _dict.ContainsKey(property) ? _dict[property] : base._Get(property);

    public override bool _Set(StringName property, Variant value)
    {
        if (!_dict.ContainsKey(property))
            return base._Set(property, value);
        _dict[property] = value.As<int>();
        return true;
    }

    public void Clear()
    {
        throw new System.NotImplementedException();
    }
}

I also tried it without a private Dictionary instance, by just directly creating a "Clear" proprty in _GetPropertyList and directly comparing with it in _Get and _Set, but that didn't trigger the "Unreferenced static string to 0" error/crash as reliably (I think it happened once, but I couldn't replicate it).

Given that this bug appears to not actually be tied to the number or properties being exported and is also not tied to implementing IDictionary, should I change the title and/or description?

[zaevi]

Related to #87638.

When Get is called, it tries to access:

• "Real" property
• Signal
• Method
• Call _Get as fallback

So, when editing this property, the editor actually gets the Clear method as Callable, which causes crash as #87638 explained.

[austin226]

87682 is merged. Has this been fixed?

[aroelke]

Looks to be fixed on my end. Following the procedure I outlined does not cause the "Unreferenced static string to 0: Clear" message to be printed, and the editor does not crash.

[akien-mga]

The main issue was fixed, but according to https://github.com/godotengine/godot/pull/87682#pullrequestreview-1858101724 there might still be an issue with _Get (CC @raulsntos, I'm not sure I understood it right).

[raulsntos]

The crash should no longer happen, but when trying to retrieve Clear you'll still get the built-in method over anything custom that you defined because it takes precedence over the _Get method. The precedence is as described by https://github.com/godotengine/godot/issues/87129#issuecomment-1913684189. Although I'm not sure this is something that can be fixed.