RigsOfRods / rigs-of-rods

Main development repository for Rigs of Rods soft-body physics simulator
https://www.rigsofrods.org
GNU General Public License v3.0
991 stars 175 forks source link

Fix #3041 - Random crashes when you spawn/switch/delete actor #3042

Closed ohlidalp closed 1 year ago

ohlidalp commented 1 year ago

See #3041 I fixed one potential crash due to dangling GfxActor pointer (hook holding pointer to DISPOSED actor which doesn't have GfxActor anymore).

UPDATE: This evolved into signifficant cleanup of the Actor internals. The motivation was to use safer memory handling with added debug checks, but it will also help future extensions. The original problem turned out to be using non-threadsafe RefCountingObject<>s from physics worker threads - see conclusion: https://github.com/RigsOfRods/rigs-of-rods/pull/3042#issuecomment-1557066997

UPDATE2 (2023-05-29): I surrendered and threw a mutex in RefCountingObject. Not proud of this at all, but I apparently used it wrong in too many places to look for it now. Technical debt ahoy!

UPDATE3 (2023-06-18): Closing in favor of #3057 (fixes the crashes) and #3059 (refactors the Actor memory management).

tritonas00 commented 1 year ago

bt points to the known delete this; // commit suicide! This is legit in C++

that line starts to escalate to a mini gravestone :laughing:

Thread 1 "RoR" received signal SIGABRT, Aborted.
0x00007ffff649f26c in ?? () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff649f26c in ?? () from /usr/lib/libc.so.6
#1  0x00007ffff644fa08 in raise () from /usr/lib/libc.so.6
#2  0x00007ffff6438538 in abort () from /usr/lib/libc.so.6
#3  0x00007ffff64392db in ?? () from /usr/lib/libc.so.6
#4  0x00007ffff64a91b7 in ?? () from /usr/lib/libc.so.6
#5  0x00007ffff64ab484 in ?? () from /usr/lib/libc.so.6
#6  0x00007ffff64adcb3 in free () from /usr/lib/libc.so.6
#7  0x0000555555865f9f in RefCountingObject<RoR::SoundScriptInstance>::Release (this=<optimized out>)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObject.h:46
#8  RefCountingObject<RoR::SoundScriptInstance>::Release (this=<optimized out>)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObject.h:40
#9  RefCountingObjectPtr<RoR::SoundScriptInstance>::ReleaseHandle (this=0x5555618b8f48)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObjectPtr.h:211
#10 RefCountingObjectPtr<RoR::SoundScriptInstance>::Set (ref=0x0, this=0x5555618b8f48)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObjectPtr.h:241
#11 RefCountingObjectPtr<RoR::SoundScriptInstance>::operator= (other=..., this=0x5555618b8f48)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObjectPtr.h:231
#12 RoR::Actor::dispose (this=this@entry=0x5555618a5c00) at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/physics/Actor.cpp:117
#13 0x000055555587ada0 in RoR::ActorManager::DeleteActorInternal (this=this@entry=0x555555c72730 <RoR::App::g_game_context+80>, actor=...)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/utils/memory/RefCountingObjectPtr.h:51
#14 0x000055555562c1e9 in RoR::GameContext::DeleteActor (this=<optimized out>, actor=...)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/GameContext.cpp:429
#15 0x00005555555f46a0 in main (argc=<optimized out>, argv=<optimized out>)
    at /home/babis/Downloads/ror-dependencies/rigs-of-rods/source/main/main.cpp:688
ohlidalp commented 1 year ago

I've piled up a series of "pointer -> index" refactoring commits here. I needed the Actor's node/beam buffers to become std::vector<>s to get the advantage of bounds checking and also get rid of memset()ing. I wanted to do this anyway for cleaner code and to make Actors editable.

Interestingly, I can't reproduce the "start 3 AI agoras, then stop, after a few tries -> boom" scenario anymore.

CuriousMike56 commented 1 year ago

Spamming start/stop on AI gives me: image

ohlidalp commented 1 year ago

It should be ready.

Turns out I was really careless when I made class Actor a RefCountingObject<> - sure, the refcounting was broken before and this helper works flawlessly, except you have to remember not to use it from threads, and I didn't even think about it. Tossing a mutex in RefCountingObject<> would be one way to treat it and I should probably do it as a debug check, but otherwise I'd rather know what's going on and use IDs instead of pointers in hazardous areas.

CuriousMike56 commented 1 year ago

Windmill Mountain crashes on final "Loading actors" stage (Release) image Debug result: image image

Debug: F1 track, "Grand prix long" preset, 2 agoras, spamming start/stop eventually gives image Also got this nasty debug error the first time, no back trace: image

Edit: Spawning trucks using the AI button in demo_script.as, 4+ trucks spawned or spawning another gives image

ohlidalp commented 1 year ago

Closing in favor of https://github.com/RigsOfRods/rigs-of-rods/pull/3057 (fixes the crashes) and https://github.com/RigsOfRods/rigs-of-rods/pull/3059 (refactors the Actor memory management).