AGS: Crash when closing while there's an overlay open

[eriktorbjorn]

• Start the latest version of the game (1.10k).
• Open the menu.
• Press the button to change font (new to this version?) that reads either "Sans Font" or "Serif Font". This will cause a message box ("Font switched to ...") to open.
• While the message box is still open, quit ScummVM.

At this point, ScummVM crashes:

#0  0x00005555583ccc4a in AGS3::free_dynamic_sprite
    (slot=7965, notify_all=false)
    at engines/ags/engine/ac/dynamic_sprite.cpp:454
#1  0x0000555558446c7b in AGS3::ScreenOverlay::~ScreenOverlay
    (this=0x555562ae0ba0, __in_chrg=<optimized out>)
    at engines/ags/engine/ac/screen_overlay.cpp:40
#2  0x00005555582f70a1 in Common::Array<AGS3::ScreenOverlay>::freeStorage
    (this=0x555560b422d0, storage=0x555562ae0b60, elements=2)
    at ./common/array.h:481
#3  0x00005555582f44ae in Common::Array<AGS3::ScreenOverlay>::~Array
    (this=0x555560b422d0, __in_chrg=<optimized out>) at ./common/array.h:136
#4  0x00005555582eeae2 in Std::vector<AGS3::ScreenOverlay>::~vector
    (this=0x555560b422d0, __in_chrg=<optimized out>)
    at ./common/std/vector.h:39
#5  0x00005555582ea36b in AGS3::Globals::~Globals
    (this=0x55556108dfd0, __in_chrg=<optimized out>)
    at engines/ags/globals.cpp:602
#6  0x00005555582dffe2 in AGS::AGSEngine::~AGSEngine
    (this=0x555560f31780, __in_chrg=<optimized out>) at engines/ags/ags.cpp:107
#7  0x00005555582e0026 in AGS::AGSEngine::~AGSEngine
    (this=0x555560f31780, __in_chrg=<optimized out>) at engines/ags/ags.cpp:110
#8  0x00005555582f96a1 in AdvancedMetaEngine<AGS::AGSGameDescription>::deleteInstance
    (this=0x55555f200c80, engine=0x555560f31780, gameDescriptor=..., meDescriptor=0x55555fa86160) at ./engines/advancedDetector.h:722
#9  0x0000555557e7804c in runGame
    (enginePlugin=0x55555f200c60, system=..., game=..., meDescriptor=0x55555fa86160) at base/main.cpp:324
#10 0x0000555557e7a4c6 in scummvm_main (argc=1, argv=0x7fffffffdbd8)
    at base/main.cpp:796
#11 0x0000555557e74daa in main (argc=1, argv=0x7fffffffdbd8)
    at backends/platform/sdl/posix/posix-main.cpp:44

Ironically, the line that it crashes on appears to be an assert(), but it's not the assert that's tripped?

[tag2015]

Thanks for the report.
It doesn't seem to be restricted to The Crimson Diamond, in general, ScummVM crashes if you attempt to close it when there's an overlay open (I replicated the issue on windows in blackwell1, if you alt-f4 while Rosa is speaking). Probably because it attempts to deallocate all the sprites after the global game pointer is no longer valid, or maybe it is called twice.
I'll investigate

[antoniou79]

Reading this ticket, I think it's likely that the issue I've just reported here for another AGS game (5 days a stranger) is the same:
https://bugs.scummvm.org/ticket/15510

[bluegr]

Renaming this to indicate it's a general problem with AGS: ScummVM crashes if you attempt to close it when there's an overlay open

[bluegr]

Here's some info I found:

When closing the game while an overlay is shown, this is the sequence of events:

• quit_free()
• unload_game()
• this calls _GP(spriteset).Reset()
• GameSetupStruct::Free()
• this calls SpriteInfos.clear() <---- this is important
• AGSEngine() destructor
• Globals() destructor
• this resets g_globals to nullptr
• this calls delete _screenover;
• this calls free_dynamic_sprite()

At this point, we got no globals, and SpriteInfos is empty, so this assert fails:

assert((slot > 0) && (static_cast<size_t>(slot) < _GP(game).SpriteInfos.size()) &&

(_GP(game).SpriteInfos[slot].Flags & SPF_DYNAMICALLOC));

I tried moving the deletion of _screenover before g_globals is cleared, but SpriteInfos is empty.

So, given the above, we can place this check at the top of free_dynamic_sprite(), which fixes the crash, and I believe it brings no memory leak, but I wanted an opinion first:

if (!g_globals)

return;

[criezy]

In 9a336aaa:

AGS: Clear all screen overlays in unload_game()

This is what upstream AGS does in the 3.6.1 and master branches,
while our code corresponded to the 3.6.0 branch.
This fixes an issue where deleting the screenoverlay vector later
on would try to access an already cleared SpriteCache.

This sould fix bugs #15488 and bug #15510.

[criezy]

In 50f971d2:

AGS: Clear all screen overlays in unload_game()

This is what upstream AGS does in the 3.6.1 and master branches,
while our code corresponded to the 3.6.0 branch.
This fixes an issue where deleting the screenoverlay vector later
on would try to access an already cleared SpriteCache.

This sould fix bugs #15488 and bug #15510.

[criezy]

I pushed a different fix (clearing the screen overlays vector before the SpriteCache is reset) that matches what is done upstream. I checked that this seems to fix the bug in 5 days a stranger. Can you confirm that this fixes the crash for you as well?

[tag2015]

@criezy thanks a lot for spotting the issue! I tested a few games and it seems to work properly now

[criezy]

Thank you for the additional tests. I think this can be closed now.