search

GodotVR / godot_openxr_vendors

Godot 4 wrapper for OpenXR vendors loaders and extensions
MIT License
90 stars 19 forks source link

Export logic code cleanup #80

Closed ghost closed 7 months ago

ghost commented 7 months ago

I looked at the stack trace with gdb after reproducing #67 to see a double free error: 

#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=139885056115072)
    at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=139885056115072)
    at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=139885056115072, signo=signo@entry=6)
    at ./nptl/pthread_kill.c:89
#3  0x00007f3987187476 in __GI_raise (sig=sig@entry=6)
    at ../sysdeps/posix/raise.c:26
#4  0x00007f398716d885 in __GI_abort () at ./stdlib/abort.c:100
#5  0x00007f39871ce676 in __libc_message (action=action@entry=do_abort, 
    fmt=fmt@entry=0x7f3987320b77 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#6  0x00007f39871e5cfc in malloc_printerr (
    str=str@entry=0x7f39873236f0 "free(): double free detected in tcache 2")
    at ./malloc/malloc.c:5664
#7  0x00007f39871e80ab in _int_free (av=0x7f398735fc80 <main_arena>, 
    p=0x1faab050, have_lock=0) at ./malloc/malloc.c:4473
#8  0x00007f39871ea453 in __GI___libc_free (mem=<optimized out>)
    at ./malloc/malloc.c:3391
#9  0x00007f397f8126e2 in ?? ()
   from /path/to/game/addons/godotopenxrvendors/pico/.bin/libgodotopenxrpico.linux.template_debug.x86_64.so

I've noticed that remove_export_plugin frees the export plugin meaning that the memfree called after is throwing the error.

I can't test it, but I'm almost sure this fixes the error. I haven't worked with C++ yet, first time.

BastiaanOlij commented 7 months ago

Looking through the code, remove_export_plugin only removes the entry from the list of plugins, it should not free the object.

The real issue here is that plugins should be handed through references, we shouldn't be using memnew and memfree to create these objects. The result thus is that on the removal of the plugin from the array, the refcount hits 0, and the object is freed. But that's a fluke, not something to rely on.

To properly fix this, OpenXREditorExportPlugin *pico_export_plugin = nullptr; should become Ref<OpenXREditorExportPlugin>, and we should use pico_export_plugin.instantiate() and pico_export_plugin.unref() to instantiate and free these objects.

Similar changes should be applied to the other plugins.

ghost commented 7 months ago

Expected due to my very limited knowledge of C++. Effectively the build still has the issue, making that pr permitted to test my changes. Thanks for your understanding, going to update this pr with your recommended changes.