SCI: GK1: Mime crash in Jackson Square

[sluicebox]

There is a concurrency bug in the scripts for the mime in Jackson Square that can crash the game in ScummVM. This is a script bug in the original game, though I don't know if it crashes the original interpreter. It is difficult, maybe impossible, to reproduce on purpose due to the large timeouts and small timing windows involved, but it's easy to simulate in the ScummVM debugger. Although it is unlikely to happen, it did happen to me, and I was able to keep the process open and use the debugger to inspect the state and verify these findings.

On Day 1, if you take the mime to the southeast corner of the park and walk near the blues band then the mime will stand in front of them and annoy them. Where he's standing is in the path of the grandma and if she walks near him during this then he will break away and start annoying her. Depending on the timing, this will crash the game with:

[CelObjView::getNumCels]: loop number 4 is equal to loop count in view 4201, method xMime::lastCel (room 430, script 64998, localCall ffffffff)'''

This error gets thrown by ScummVM SCI32 code that's trying to trap places in games that depend on a bug in the original interpreter that returned random memory when asking for the number of cells of one past the last loop index. In this case, the game isn't relying on that, this trap just happens to have caught two game scripts colliding and setting an invalid loop.

The script bug technically exists on all four Jackson Square rooms but as far as I can tell the conditions to trigger it are only in the southeast room, 430. In other rooms the pedestrian paths aren't located where they interrupt the mime when annoying a performer.

If you get near the mime, he follows you. If he gets close to a pedestrian, he will annoy them and be shooed away and return to his original location. If he gets close to a stationary performer then he will annoy them for 20 seconds before being shooed away. If you walk back to the mime while he's annoying a performer then he will stop and follow you again. So, you lose the mime to pedestrians and performers but you have 20 seconds to reclaim him from performers.

Pedestrian scripts and mime scripts are in script 401 and are used by all four Jackson Square rooms.
Stationary performer scripts are independently implemented in their own room scripts, 430 for southeast.
It's probably not a good sign that the room-specific Code instances are named "specialMimeCode"...

• When walking by the blues band, sAnnoyBlues (430) is run and sets mimeTimer4 for 20 seconds. mimeTimer4 in turn runs sMimeLeaves.
• When the mime is near a pedestrian, sMimeFollowsPed (401) is run, causing the mime to annoy, be shooed, and leave.
• When reclaiming the mime from the blues bands, mimeTimer4 is stopped.
• When a pedestrian gets near the mime while he's annoying the blues band, mimeTimer4 *isn't* stopped!

mimeTimer4 not being stopped means it can fire while the mime is annoying the pedestrian, causing sMimeLeaves and sMimeFollowsPed to run at the same time and step on each other as they both manipulate xMime (401). Usually this results in harmless graphical glitching (the mime is shooed away by grandma, then teleports back to be shooed away by the band), but if the timing is right then sMimeLeaves changes xMime:loop in between sMimeFollowsPed states and sMimeFollowsPed increments the loop number out of bounds.

Egregious oversimplification of scripts:

sMimeFollowsPed (401)
State 0:
	local7 = (pedestrian == grandma or woman) ? 0 : 1
	cycles = 1
State 2:
	xMime:view = 4201
	xMime:loop = local7
State 4:
	grandma dialog: "I'll call the police!"
	ticks = 120
State 5
	xMime:cel = 0
	xMime:loop = xMime:loop + 2 // uh-oh
	xMime:setCycle(End self) // kaboom if xMime:loop is 4
...	

sMimeLeaves (430)
State 0:
	ticks = 60
State 1
	xMime:view = 4201
	xMime:cel = 0
	xMime:loop = 2 // UH-OH!
	xMime:setCycle(End self)
...

That's a 120-ish tick window in between sMimeFollowsPed setting the loop to zero and incrementing it by two. If sMimeLeaves:changeState(1) occurs during that then the loop is set to two before being incremented to four and is then out of bounds.

To reproduce the crash with the debugger, get the mime to annoy the grandma. Break into the debugger as soon as she speaks, sMimeFollowsPed:state should be 4 and xMime:loop should be 0. Simulate sMimeLeaves' interference with "send xMime loop 2". Leave the debugger and the game will quickly crash. This is the same backtrace that I got from the real crash. I confirmed in the real crash that sMimeLeaves was in state 1.

To attempt to reproduce the real crash, get the mime to annoy the band soon before the grandma appears on the screen. Even if it doesn't crash, you'll probably see the mime glitch out and teleport around.

Ideally these scripts wouldn't run at the same time, but I doubt that's possible with script patching since these scripts are spread across two resources, half is common code and the other is room-specific, and I don't imagine backporting synchronization to be easy, even without bytecode constraints.

I think the bug can be mitigated by patching sMimeFollowsPed:changeState(5) to set xMime:loop to local7 + 2 instead of xMime:loop + 2. local7 has the same value and isn't set anywhere else so it's safe to use instead of the mutable shared state. You still have the two scripts stepping on each other and bouncing the mime around but as far as I can tell there's nothing that crashes. I omitted a lot of stuff from my script summary though so someone who knows what they're doing should take a look.

Attached is a save game in southeast Jackson Square right before the grandma shows up, and a screenshot of the backtrace from the real crash.

[csnover]

Wow, what an exhaustive write-up! I feel bad that you spent so much time on it, since the crash is a well-known defect caused by an engine bug in SSCI. Fixed in e8c429832f7b6393f853fd6d9ce8ba2e62f6a93c.