#5708 DW2: Crash On Entering Sewers

[digitall]

In Discworld 2, upon entering the Sewers (From the Fool's Guild), ScummVM errors out with an assertion:
scummvm: engines/tinsel/actors.cpp:770: int Tinsel::GetLoopCount(int): Assertion `ano > 0 && ano <= NumActors' failed.
This may be linked to the animation of the Luggage jumping into the Sewers.

This only seems to occur when starting from a "New Game", as loading a savegame at the Fool's Guild does not trigger an issue.
Cutscenes were also skipped through using ESC.

ScummVM 1.4.0git471-gd2a55b4-dirty (May 22 2011 17:11:48)
Features compiled in: Vorbis FLAC MP3 ALSA SEQ TiMidity RGB zLib FluidSynth Theora
on Linux x86_32

As reported on IRC by Tron, but have replicated.

Will attempt to bisect for regressive commit.

[Torbjörn Andersson]

I get this too. I don't get the same error every time. Sometimes I get the assertion, sometimes it just crashes and a few times I've gotten an "Overlapping (in time) CD-plays" errors. And sometimes it doesn't crash at all, though going up and down a few times seems to do the trick in those cases.

I was able to capture one crash in Valgrind:

==31668== Invalid read of size 4
==31668== at 0x8ABEA82: Tinsel::ExtractActor(unsigned int) (play.cpp:1177)
==31668== by 0x8ABE631: Tinsel::PlayFilmc(Tinsel::CoroBaseContext*&, unsigned int, int, int, int, bool, bool, bool, int, bool) (play.cpp:1075)
==31668== by 0x8A7D75F: Tinsel::Play(Tinsel::CoroBaseContext*&, unsigned int, int, int, bool, int, bool, Tinsel::TINSEL_EVENT, int, int) (tinlib.cpp:1614)
==31668== by 0x8A879FB: Tinsel::CallLibraryRoutine(Tinsel::CoroBaseContext*&, int, int*, Tinsel::INT_CONTEXT const*, Tinsel::RESUME_STATE*) (tinlib.cpp:4946)
==31668== by 0x8AB8CC4: Tinsel::Interpret(Tinsel::CoroBaseContext*&, Tinsel::INT_CONTEXT*) (pcode.cpp:685)
==31668== by 0x8A76201: Tinsel::ProcessTinselProcess(Tinsel::CoroBaseContext*&, void const*) (sched.cpp:550)
==31668== by 0x8A7587F: Tinsel::Scheduler::schedule() (sched.cpp:191)
==31668== by 0x8A8C081: Tinsel::TinselEngine::NextGameCycle() (tinsel.cpp:1027)
==31668== by 0x8A8BEE1: Tinsel::TinselEngine::run() (tinsel.cpp:975)
==31668== by 0x80500CD: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:207)
==31668== by 0x8050D33: scummvm_main (main.cpp:421)
==31668== by 0x804F13E: main (posix-main.cpp:45)
==31668== Address 0xbf80c58 is not stack'd, malloc'd or (recently) free'd

I added a debug("pReel->mobj = %u", FROM_LE_32(pReel->mobj)); to ExtractActor(), and this is the output I got from it, playing the game from the beginning:

pReel->mobj = 505418960
pReel->mobj = 505425552
pReel->mobj = 505426756
pReel->mobj = 505427028
pReel->mobj = 505415672
pReel->mobj = 505425016
pReel->mobj = 505427260
pReel->mobj = 505422988
pReel->mobj = 505419584
pReel->mobj = 235993848
pReel->mobj = 134337452
pReel->mobj = 3652914432
Segmentation fault

Actually, the entire FILM struct seems messed up at this point. In the ones before, frate has been 8 and numreels has been between 2 and 7. For the last one, I just got 2477906393 and 2813428185. Perhaps significantly, these values do not appear to be random. I got them twice in a row now.

[Torbjörn Andersson]

I don't have the time to look for where it broke now, but if I didn't mess up with git, the error was there in a132bbb10c2f7349862c6c35d03d829ff76d5dea which was a while ago.

[Torbjörn Andersson]

I forgot to mention: ScummVM 1.2.1 works for me.

[Torbjörn Andersson]

It's possible that you have to wait for the ghost to appear before entering the sewers, in order to trigger the bug.

[Torbjörn Andersson]

This revision seems to work for me:

TINSEL: Removed some unused global static variables
c89f2276d1b52a99f08861c5acd86fedb349c718

While this revision is broken:

TINSEL: Removed some unused global static variables
cd085b1ae889dfa12d1b525810b218342c61a7a6

The only suspicious thing I see is the removal of cdPlayFileNum and cdPlaySceneNum. Now, cdPlayFileNum was never actually set, it seems, but cdPlaySceneNum was so maybe when it tested for "if (cdPlayFileNum == cdPlaySceneNum && start == cdBaseHandle)" it was actually testing for "if (cdPlaySceneNum == 0 && start == cdBaseHandle)".

Actually, I don't see cdPlaySceneNum ever being 0, so maybe LoadExtraGraphData() should just never return prematurely? Removing the "if (start == cdBaseHandle)" test from LoadExtraGraphData() seems to work for me.

[Max Horn]

Any particular reason why this has been assigned to me?

[digitall]

fingolfin: Apologies. I initially thought that this was a recent regression due to the pDispList issues, but this does not appear to be the case.

[Torbjörn Andersson]

This bug also happens in the 1.3.0 branch, so it shold probably be considered quite important.

[Torbjörn Andersson]

Oops, I see I got the titles of the commits wrong earlier. The working revision had the description, "TINSEL: Merged NewName() inside DoSave() in order to remove a static var"

[Torbjörn Andersson]

This should be fixed now, on both the trunk and the 1.3 branch. (I'm using a simpler fix on the trunk, but it carries a theoretical but unlikely chance of regressions. The fix on the branch should be perfectly safe.)