Apply singleton instance binding clean-up fix no matter how it is created

[dsnopek]

This was originally created in an attempt to address #1900

It does fix a crash that I'm able to reproduce when accessing a singleton via Engine::get_singleton()->get_singleton("Name") which I've added a test for.

This fix is a little weird in that it only addresses the Wrapped(GodotObject *) constructor, but I think that's actually correct, because we only want to do this when the instance binding creation was initiated on the engine side using a pre-existing instance. If someone creates a new instance of a singleton (which they really shouldn't do, but there's nothing preventing it) then it shouldn't register that as an engine singleton.

However, it seems it's still possible to reproduce #1900 even with this PR

[Naros]

Hi @dsnopek so the game does not crash, but the Godot Editor (beta2) still crashes on exit. I am updating my local windows builds to rc2 with debug symbols and will report back.

================================================================
CrashHandlerException: Program crashed with signal 11
Engine version: Godot Engine v4.6.beta2.official (551ce8d47feda9c81c870314745366b24957624b)
Dumping the backtrace.
[1] error(-1): no debug info in PE/COFF executable
[2] error(-1): no debug info in PE/COFF executable
[3] error(-1): no debug info in PE/COFF executable
[4] error(-1): no debug info in PE/COFF executable
[5] error(-1): no debug info in PE/COFF executable
[6] error(-1): no debug info in PE/COFF executable
-- END OF C++ BACKTRACE --
================================================================

With rc2 debug symbols enabled

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.6.rc.custom_build (78c6632eb174aabb2790975cf83e28fee065b43d)
Dumping the backtrace.
[0] <couldn't map PC to fn name>
[1] <couldn't map PC to fn name>
[2] Object::~Object (E:\GitHub\godotengine2\core\object\object.cpp:2466)
[3] GDExtensionManager::`scalar deleting destructor'
[4] unregister_core_types (E:\GitHub\godotengine2\core\register_core_types.cpp:426)
[5] Main::cleanup (E:\GitHub\godotengine2\main\main.cpp:5252)
[6] widechar_main (E:\GitHub\godotengine2\platform\windows\godot_windows.cpp:105)
[7] _main (E:\GitHub\godotengine2\platform\windows\godot_windows.cpp:128)
[8] main (E:\GitHub\godotengine2\platform\windows\godot_windows.cpp:140)
[9] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[10] <couldn't map PC to fn name>
-- END OF C++ BACKTRACE --
================================================================

This includes the godot-cpp v10 patch from this PR

[Naros]

@dsnopek where does this stand?

[dsnopek]

I haven't had a chance to come back to it.

I know I tested this when I made the PR, but other than the automated test this adds, I don't recall how I was doing that. But however I was testing it, it didn't crash for me like in your testing. Can you share an MRP that I can use?

[Naros]

Hi @dsnopek you can find a simple MRP:
https://github.com/Naros/godot-cpp-mrp

In the get-singleton-crash-windows branch, you can check that out, and compile it using scons and target editor:

git clone https://github.com/Naros/godot-cpp-mrp.git .
git checkout get-singleton-crash-windows
git submodule init
git submodule update
scons target=editor

Then load the sample project from ./project into the editor on Windows, making sure to start the editor from a terminal/cmd window, and then stop the editor. Upon stop you'll get the output:

OpenGL API 3.3.0 NVIDIA 591.86 - Compatibility - Using Device: NVIDIA - NVIDIA GeForce RTX 3080 Ti

Editing project: E:/GitHub/godot-cpp-mrp/project
Godot Engine v4.7.beta3.official.860821708 - https://godotengine.org
D3D12 12_0 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 3080 Ti


================================================================
CrashHandlerException: Program crashed with signal 11
Engine version: Godot Engine v4.7.beta3.official (86082170822e19ebb619c7f53c8fe54a6f8d3d2a)
Dumping the backtrace.
Load address: 7ff521b40000

[1] 7ff665c02321 (main+40c2321) - no debug info in PE/COFF executable
[2] 7ffd833e1732 (ntdll.dll+121732) - no debug info in PE/COFF executable
[3] 7ffd834263de (ntdll.dll+1663de) - no debug info in PE/COFF executable
[4] 7ffd832d9496 (ntdll.dll+19496) - no debug info in PE/COFF executable
[5] 7ffd83425d1d (ntdll.dll+165d1d) - no debug info in PE/COFF executable
[6] 7ffd010018ff (main+69f4c18ff) - no debug info in PE/COFF executable
-- END OF C++ BACKTRACE --
================================================================

If you remove the for loop in the MrpEditorPlugin::enter_tree() method, and recompile, and retest, the crash does not occur.

NOTE: The MRP is using master for godot-cpp and not this PR.

[Ivorforce]

I can reproduce the crash from the above MRP on macOS, and this PR doesn't fix it.
For me the crash only happens when importing (when .godot is created). When it's already imported, I see no crashes.

[Naros]

I can reproduce the crash from the above MRP on macOS, and this PR doesn't fix it. For me the crash only happens when importing (when .godot is created). When it's already imported, I see no crashes.

This has been the source of a number of concerns I've seen here and there, as the engine often does some early startup in other singletons that isn't always refired when the .gdextension resources are loaded, looking at the ScriptServer and CreateScriptDialog we fixed recently.

Is there a better way to unify this loading so we have consistent behavior?

[Ivorforce]

It might be possible to add a 'do not skip unnecessary work' flag to the engine startup. However, that sounds like it could be intrusive to a lot of code.

The realistic solution is probably to add more test cases so our CI has this covered. Import with a GDExtension installed is common enough that we should be testing it, if not as part of the PR CI, at least regularly.

[Ivorforce]

MRP backtraces to EditorDoc::doc being null in DocTools::generate:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x0000000101997010 godot.macos.editor.arm64`HashMap<String, DocData::ClassDoc, HashMapHasherDefault, HashMapComparatorDefault<String, void>, DefaultTypedAllocator<HashMapElement<String, DocData::ClassDoc>>>::operator[](this=0x0000000000000008, p_key=0x000000016fdfda88) at hash_map.h:561:17 [opt]
    frame #1: 0x0000000101998670 godot.macos.editor.arm64`DocTools::generate(this=0x0000000000000000, p_flags=(value = 3)) at doc_tools.cpp:452:4 [opt]
    frame #2: 0x00000001019db6f4 godot.macos.editor.arm64`EditorHelp::_gen_extensions_docs() at editor_help.cpp:3084:7 [opt]
    frame #3: 0x00000001041f3080 godot.macos.editor.arm64`CallQueue::_call_function(this=<unavailable>, p_callable=0x0000000ae9733328, p_args=0x0000000ae9733340, p_argcount=0, p_show_error=true) at message_queue.cpp:218:13 [opt]
    frame #4: 0x00000001041f3364 godot.macos.editor.arm64`CallQueue::flush(this=0x0000000ae9243e90) at message_queue.cpp:266:6 [opt]
    frame #5: 0x0000000100089ff8 godot.macos.editor.arm64`Main::cleanup(p_force=<unavailable>) at main.cpp:5251:17 [opt]
    frame #6: 0x000000010000c218 godot.macos.editor.arm64`OS_MacOS_Headless::run(this=0x000000010cd483f0) at os_macos.mm:1271:2 [opt]
    frame #7: 0x00000001000638c4 godot.macos.editor.arm64`main(argc=6, argv=0x000000016fdfeb78) at godot_main_macos.mm:132:6 [opt]

This suggests it's not a godot-cpp bug, but a documentation / shutdown bug. Wouldn't be the first one to interplay badly with GDExtensions.

[dsnopek]

Perhaps we should have this discussion on the issue?

Even if this PR doesn't fix the crash described in #1900, it does fix a crash that I'm able to reproduce (via the test this PR adds). For that reason, maybe I should just disconnect this PR from that issue, and this can be evaluated on its own, while we continue to debug #1900 there?