Access Violation when calling Input SetCustomMouseCursor

ghostradiogames commented 1 year ago

Godot version

4.0 beta 4

System information

Windows 10, Vulkan Nvidia GeForce GTX 1660 Super

Issue description

I'm working on implementing a custom cursor, and when I call Input.SetCustomMouseCursor), I get an access violation. This happens in Visual Studio. I don't know how to stop it from happening, I've tried omitting the Vector2 altogether for the hotspot, tried a different shape, tried giving it a Texture2D and a Resource object; same crash occurs. It doesn't happen in the Editor, but I'm not sure there isn't a problem because I've noticed that the game hangs when closing with the code added. The access violation does not appear to be thrown until closing the game or stopping the debugger.

public override void _Ready()
{
        Input.MouseMode = Input.MouseModeEnum.Hidden;
        var pointerCursor = ResourceLoader.Load("res://Assets/Cursors/pointer.png");
        Input.SetCustomMouseCursor(pointerCursor, Input.CursorShape.Arrow, testVector);
}

public override void _Process(double delta)
{
        Position = GetGlobalMousePosition();
}

Seems to be an uninitialized pointed but I'm not seeing where that's happening, I have no code that should be doing that. I've included a sample project.

This is the Visual Studio console output: Exception thrown at 0x00007FF7ABB0F332 in Godot_v4.0-beta4_mono_win64.exe: 0xC0000005: Access violation reading location 0x0000000000000000.

Call stack:

>   Godot_v4.0-beta4_mono_win64.exe!00007ff7abb0f332()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e9fc69()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7acab37bc()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e14785() Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e15786() Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e35ca0()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e53b22() Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8e548a9() Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7ad5f0ce5()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8dc13c1()  Unknown
    Godot_v4.0-beta4_mono_win64.exe!00007ff7a8dc14d6() Unknown
    kernel32.dll!00007ffdf5317034() Unknown
    ntdll.dll!00007ffdf7062651()    Unknown

Steps to reproduce

Create a scene, add a sprite to serve as the mouse cursor. Create a C# script with the code shown above, or grab the sample project. You'll need to run it in Visual Studio. Open the solution file and edit the Profile with the settings in the image below, change the executable path to your executable obviously.

Minimal reproduction project

TestGame.zip

raulsntos commented 1 year ago

I tried to reproduce this in GDScript. Here's the ported MRP for your convenience:

TestGameGD.zip

I'm getting a segmentation fault in Linux when closing the game caused by freeing a CompressedTexture2D. Here's the GDB stack trace:

#0  0x000055555de2315f in CompressedTexture2D::~CompressedTexture2D (this=0x55556323c380)
    at scene/resources/texture.cpp:1047
#1  0x0000555559d97a4f in memdelete<RefCounted> (p_class=0x55556323c380)
    at ./core/os/memory.h:109
#2  0x000055555f125e05 in Variant::_clear_internal (this=0x555563377260)
    at core/variant/variant.cpp:1354
#3  0x0000555559cc759c in Variant::clear (this=0x555563377260) at ./core/variant/variant.h:302
#4  0x0000555559cc5765 in Variant::~Variant (this=0x555563377260)
    at ./core/variant/variant.h:778
#5  0x0000555559cfed84 in CowData<Variant>::_unref (this=0x5555633a2920, p_data=0x555563377260)
    at ./core/templates/cowdata.h:213
#6  0x0000555559cfec28 in CowData<Variant>::~CowData (this=0x5555633a2920)
    at ./core/templates/cowdata.h:412
#7  0x0000555559cf95a9 in Vector<Variant>::~Vector (this=0x5555633a2918)
    at ./core/templates/vector.h:287
#8  0x0000555559d02cb9 in KeyValue<DisplayServer::CursorShape, Vector<Variant> >::~KeyValue (
    this=0x5555633a2910) at ./core/templates/pair.h:82
#9  0x0000555559d02c99 in HashMapElement<DisplayServer::CursorShape, Vector<Variant> >::~HashMapElement (this=0x5555633a2900) at ./core/templates/hash_map.h:55
#10 0x0000555559d02c6c in memdelete<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > > (p_class=0x5555633a2900) at ./core/os/memory.h:109
#11 0x0000555559d02c39 in DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > >::delete_allocation (this=0x55556090f234, p_allocation=0x5555633a2900)
    at ./core/os/memory.h:206
#12 0x0000555559d0b7e1 in HashMap<DisplayServer::CursorShape, Vector<Variant>, HashMapHasherDefault, HashMapComparatorDefault<DisplayServer::CursorShape>, DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > > >::clear (this=0x55556090f228)
    at ./core/templates/hash_map.h:265
#13 0x0000555559cfbc49 in HashMap<DisplayServer::CursorShape, Vector<Variant>, HashMapHasherDefault, HashMapComparatorDefault<DisplayServer::CursorShape>, DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > > >::~HashMap (this=0x55556090f228)

qarmin commented 1 year ago

Problem happens also on Ubuntu without any scripts, just open and close project(godot4 --quit) - A.zip

scene/resources/texture.cpp:1044:28: runtime error: member access within null pointer of type 'struct RenderingServer'

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.beta.custom_build (204715ae957652a655b38a7a112aa20f59cabcd7)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[1] godot4s() [0x26f217c] (/home/runner/work/GodotBuilds/GodotBuilds/godot/platform/linuxbsd/crash_handler_linuxbsd.cpp:56)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x3bcf0) [0x7fd53a43bcf0] (??:0)
[3] CompressedTexture2D::~CompressedTexture2D() (/home/runner/work/GodotBuilds/GodotBuilds/godot/scene/resources/texture.cpp:1044)
[4] void memdelete<RefCounted>(RefCounted*) (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/os/memory.h:112)
[5] Variant::_clear_internal() (/home/runner/work/GodotBuilds/GodotBuilds/godot/core/variant/variant.cpp:1357)
[6] Variant::clear() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/variant/variant.h:304)
[7] Variant::~Variant() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/variant/variant.h:781)
[8] CowData<Variant>::_unref(void*) (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/cowdata.h:211 (discriminator 3))
[9] CowData<Variant>::~CowData() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/cowdata.h:413)
[10] Vector<Variant>::~Vector() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/vector.h:287)
[11] KeyValue<DisplayServer::CursorShape, Vector<Variant> >::~KeyValue() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/pair.h:82)
[12] HashMapElement<DisplayServer::CursorShape, Vector<Variant> >::~HashMapElement() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/hash_map.h:55)
[13] void memdelete<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > >(HashMapElement<DisplayServer::CursorShape, Vector<Variant> >*) (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/os/memory.h:112)
[14] DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > >::delete_allocation(HashMapElement<DisplayServer::CursorShape, Vector<Variant> >*) (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/os/memory.h:206)
[15] HashMap<DisplayServer::CursorShape, Vector<Variant>, HashMapHasherDefault, HashMapComparatorDefault<DisplayServer::CursorShape>, DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > > >::clear() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/hash_map.h:266)
[16] HashMap<DisplayServer::CursorShape, Vector<Variant>, HashMapHasherDefault, HashMapComparatorDefault<DisplayServer::CursorShape>, DefaultTypedAllocator<HashMapElement<DisplayServer::CursorShape, Vector<Variant> > > >::~HashMap() (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/templates/hash_map.h:584)
[17] DisplayServerX11::~DisplayServerX11() (/home/runner/work/GodotBuilds/GodotBuilds/godot/platform/linuxbsd/x11/display_server_x11.cpp:5214)
[18] void memdelete<DisplayServer>(DisplayServer*) (/home/runner/work/GodotBuilds/GodotBuilds/godot/./core/os/memory.h:112)
[19] finalize_display() (/home/runner/work/GodotBuilds/GodotBuilds/godot/main/main.cpp:270)
[20] Main::cleanup(bool) (/home/runner/work/GodotBuilds/GodotBuilds/godot/main/main.cpp:3392)
[21] godot4s(main+0x599) [0x26f1b4f] (/home/runner/work/GodotBuilds/GodotBuilds/godot/platform/linuxbsd/godot_linuxbsd.cpp:77)
[22] /lib/x86_64-linux-gnu/libc.so.6(+0x23510) [0x7fd53a423510] (??:0)
[23] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x89) [0x7fd53a4235c9] (??:0)
[24] godot4s(_start+0x25) [0x26f14f5] (??:?)
-- END OF BACKTRACE --
================================================================

and there is also memory leak

ERROR: 1 RID allocations of type 'N10RendererRD14TextureStorage7TextureE' were leaked at exit.
WARNING: 2 RIDs of type "Texture" were leaked.
     at: finalize (drivers/vulkan/rendering_device_vulkan.cpp:9774)
 - RID:3912715206798
 - RID:3908420239501

W4RH4WK commented 1 year ago

I've just looked into this. Currently there doesn't seem to be a crash anymore. The nullptr deref is caught (emitting an error), but the resource (cursor texture) is still not released correctly.

The DisplayServer maintains a cursor_cache which references the cursor texture. The DisplayServer's destructor clears the cache; however, the RenderingServer is needed for correctly releasing the cursor texture. The RenderingServer is destroyed before the DisplayServer.

I see two ways of handling this:

Either, the RenderingServer releases the cursor texture on destruction (may happen already), in which case the reference in the cursor_cache should be invalidated.
Or, add a member function to the DisplayServer along the lines of release_rendering_resources which is called before either of the two servers is destroyed. That function would be called in finalize_display and clears the cursor cache.

akien-mga commented 1 year ago

Sounds like the crash was fixed, and the remaining issue might be a duplicate of #74508, which has #74511 as a potential fix. Does that sound right after your analysis @W4RH4WK?

Edit: Or not, both seem related but not necessarily the same. CC @AThousandShips

AThousandShips commented 1 year ago

Will take a look

W4RH4WK commented 1 year ago

Sounds like the crash was fixed, and the remaining issue might be a duplicate of #74508, which has #74511 as a potential fix. Does that sound right after your analysis @W4RH4WK?

Edit: Or not, both seem related but not necessarily the same. CC @AThousandShips

Looking at these issues, the resource leak should not be present when the user calls set_custom_mouse_cursor(null) before application shutdown as this reset the cursor to the system's default cursor and removes the cache entry of the previously loaded cursor.

Here my thoughts:

I don't think we should require the user to call set_custom_mouse_cursor(null) just to clean up correctly.
I feel like removing a cache entry when the system cursor is restored might not be desirable. While this is the current behavior, it seems counterintuitive to me. (But this is another discussion orthogonal to this issue.)
The (original) issue has been confirmed on Ubuntu as well. Changing the Windows implementation might be not enough.
Here the DisplayServer is referencing rendering resources, which does not seem super uncommon. Having a dedicated function for releasing such resources before the RenderingServer is destroyed seems adequate to me.

AThousandShips commented 1 year ago

Wasn't aware it affected Ubuntu as well, updated the relevant PR.

zorbathut commented 6 days ago

Crash is still occurring on 4.3.

godotengine / godot