SCUMM: (HE) - fix wiz drawing mem leaks

[athrxx]

Currently, buffers get allocated that do not get freed. Apparently, the reason is that it is not well known when it is actually safe to do that.
The spy watch mini game in Spy Fox 3 leaks A LOT. This here should hopefully fix the situation. Or at least give some inspiration on how to fix it...

[AndywinXp]

First of all, amazing work! And I'm grateful for the fact that you used a sizable chunk of your time to fix my stuff 🙂 sorry for the fact that I haven't been able to take care of it myself, but my real job has been quite intense during the last month and a half so I've had much less time to dedicate to the project than I would have wanted.

That being said: I love how well structured this solution is and I would like to merge it. But! I have been testing the various games and while the usual suspects work very well (except for one outlier) the various Backyard games seem to crash frequently, here are some examples:

• Backyard Baseball 2001: there's a certain chance the game crashes after the intro (works with the demo too);
• Backyard Baseball 2003: ditto;
• Backyard Basketball: ditto;
• Backyard Soccer MLS: going back and forth between menus should eventually trigger a crash;
• Pajama Sam: Games to Play on Any Day: skip the intro, the game should crash.

What these games have in common is their high HE version and their overuse of Wiz newer effects (except for Pajama Sam), but that's just speculation on my part, although I tried I haven't been able to exactly pinpoint why the crash is happening.

All these games crash at the destructor of WizPxShrdBuffer at the following section:

if (_lifes && !*_lifes)
    delete _lifes;

If activate the WIZ_DEBUG_BUFFERS define, it appears that the crash happens in this previous part of the destructor:

Common::Array<DbgEntry>::iterator i = Common::find(_allocLocs->begin(), _allocLocs->end(), _buff);

Now, I mentioned there was an outlier: Pajama Sam 3. Whenever you go to the credits (put the mouse pointer on the lower left part of the screen, click on the computer icon, and then press "Credits"), you will see polaroid pictures being positioned on the screen via a series of WizWarp operations. It seems that these changes broke this warping effect, here's a frame:

From the look of it, this corruption could be happening because a buffer which was being used by the warping operations has being freed prematurely.

[athrxx]

Thanks for testing all these. I have turned it into a draft, until it can be debugged.

[athrxx]

I have made a small update. Pajama Sam 3 seems to be okay, now. And the Spyfox waste mini game seems to be still
fine, too. Haven't done any extensive testing yet, though. I don't have all that many of these games, anyway.

One thing that looks kind of unpleasant is wizwarp_he.cpp, lines 130 - 137. I can't imagine that this would go well
with an owned/allocated bufferPtr. But I haven't found a situation where it actually uses an owned/allocated bufferPtr.
I'll fix that anyway, but would prefer to have a testing scenario...

[AndywinXp]

Greetings from the Land of the Dead 😛 here just to say that the latest changes successfully fix all the issues I described in my original comment 🙂 thank you so much!

One thing that looks kind of unpleasant is wizwarp_he.cpp, lines 130 - 137. I can't imagine that this would go well
with an owned/allocated bufferPtr. But I haven't found a situation where it actually uses an owned/allocated bufferPtr.
I'll fix that anyway, but would prefer to have a testing scenario...

In which kind of situations would the casts cause issues? The 16-bit case is extensively used within the Spyfox 3 minigame, while the 8-bit one is from the Pajama Sam 3 credits; but of course I'm assuming that plain usage is not the issue per se.

[athrxx]

I hope you're getting better. Thanks for testing again 😃

No, the cast is not the problem here. The problem is that an allocated buffer would be freed in lines 133, 137 by the destructor (but still get assigned again, after adding the xstart). So I have to do that a little differently. Since the whole point seems to be adding the xstart to the buffer, I should try not to overcomplicate this.

From my testing, I never got any situation with an allocated buffer, anyway. It is a bit hard to predict, since the buffer usually gets forwarded as part of optionalDestBitmap. Still, this should work correctly, regardles of how the buffer was allocated.

[athrxx]

Okay, I have added a fix for that, too. Although, thinking about it, it really seems unlikely that a newly allocated would even have an xstart.

The solution is a bit fishy, I will not get a code design award for this, but it works without making things complicated.

[AndywinXp]

Thank you for the update! I have re-tested all the games mentioned before (and more), and I can confirm there are no issues with them.

[AndywinXp]

Finished the code review as well, everything looks great. I'm for keeping the debug define around (just as you did), just in case. Merging, thanks again! 🙏