SCI: Moved hunk pointer handling to the GC, and removed some related workarounds

SCI scripts can contain stale pointers, which are used later on. We now delete
the contents of hunk entries without invalidating the relevant pointers and let
the GC clear the references. Many thanks to waltervn and wjp for all their work
and help on this.

8 files changed, 39 insertions, 234 deletions

diff --git a/engines/sci/engine/gc.cpp b/engines/sci/engine/gc.cpp
index 7692613ee5..e080ad6e4b 100644
--- a/engines/sci/engine/gc.cpp
+++ b/engines/sci/engine/gc.cpp
@@ -25,6 +25,7 @@
 
 #include "sci/engine/gc.h"
 #include "common/array.h"
+#include "sci/graphics/ports.h"
 
 namespace Sci {
 
@@ -84,6 +85,18 @@ static void processWorkList(SegManager *segMan, WorklistManager &wm, const Commo
 	}
 }
 
+static void processEngineHunkList(WorklistManager &wm) {
+	PortList windowList = g_sci->_gfxPorts->_windowList;
+
+	for (PortList::const_iterator it = windowList.begin(); it != windowList.end(); ++it) {
+		// FIXME: We also store Port objects in the window list.
+		// We should add a check that we really only pass windows here...
+		Window *wnd = ((Window *)*it);
+		wm.push(wnd->hSaved1);
+		wm.push(wnd->hSaved2);
+	}
+}
+
 AddrSet *findAllActiveReferences(EngineState *s) {
 	assert(!s->_executionStack.empty());
 
@@ -142,6 +155,7 @@ AddrSet *findAllActiveReferences(EngineState *s) {
 	debugC(kDebugLevelGC, "[GC] -- Finished explicitly loaded scripts, done with root set");
 
 	processWorkList(s->_segMan, wm, heap);
+	processEngineHunkList(wm);
 
 	return normalizeAddresses(s->_segMan, wm._map);
 }
diff --git a/engines/sci/engine/script_patches.cpp b/engines/sci/engine/script_patches.cpp
index 33ec5ef5ea..7431707e69 100644
--- a/engines/sci/engine/script_patches.cpp
+++ b/engines/sci/engine/script_patches.cpp
@@ -60,55 +60,6 @@ struct SciScriptSignature {
 //  - if not EOS, an adjust offset and the actual bytes
 //  - rinse and repeat
 
-#if 0
-
-// ===========================================================================
-// Castle of Dr. Brain
-// cipher::init (script 391) is called on room 380 init. This resets the word
-//  cipher puzzle. The puzzle sadly operates on some hep strings, which aren't
-//  saved in our sci. So saving/restoring in this room will break the puzzle
-//  Because of this issue, we just init the puzzle each time it's accessed.
-//  this is not 100% sierra behaviour, in fact we will actually reset the puzzle
-//  during each access which makes it impossible to cheat.
-const byte castlebrainSignatureCipherPuzzle[] = {
-	22,
-	0x35, 0x00,        // ldi 00
-	0xa3, 0x26,        // sal local[26]
-	0xa3, 0x25,        // sal local[25]
-	0x35, 0x00,        // ldi 00
-	0xa3, 0x2a,        // sal local[2a] (local is not used)
-	0xa3, 0x29,        // sal local[29] (local is not used)
-	0x35, 0xff,        // ldi ff
-	0xa3, 0x2c,        // sal local[2c]
-	0xa3, 0x2b,        // sal local[2b]
-	0x35, 0x00,        // ldi 00
-	0x65, 0x16,        // aTop highlightedIcon
-	0
-};
-
-const uint16 castlebrainPatchCipherPuzzle[] = {
-	0x39, 0x6b,        // pushi 6b (selector init)
-	0x76,              // push0
-	0x55, 0x04,        // self 04
-	0x35, 0x00,        // ldi 00
-	0xa3, 0x25,        // sal local[25]
-	0xa3, 0x26,        // sal local[26]
-	0xa3, 0x29,        // sal local[29]
-	0x65, 0x16,        // aTop highlightedIcon
-	0x34, 0xff, 0xff,  // ldi ffff
-	0xa3, 0x2b,        // sal local[2b]
-	0xa3, 0x2c,        // sal local[2c]
-	PATCH_END
-};
-
-//    script, description,                                      magic DWORD,                             adjust
-const SciScriptSignature castlebrainSignatures[] = {
-	{    391, "cipher puzzle save/restore break",            1, PATCH_MAGICDWORD(0xa3, 0x26, 0xa3, 0x25),    -2, castlebrainSignatureCipherPuzzle, castlebrainPatchCipherPuzzle },
-	SCI_SIGNATUREENTRY_TERMINATOR
-};
-
-#endif
-
 // ===========================================================================
 // stayAndHelp::changeState (0) is called when ego swims to the left or right
 //  boundaries of room 660. Normally a textbox is supposed to get on screen
@@ -497,74 +448,6 @@ const SciScriptSignature gk1Signatures[] = {
 	SCI_SIGNATUREENTRY_TERMINATOR
 };
 
-#if 0
-
-// ===========================================================================
-// this here gets called on entry and when going out of game windows
-//  uEvt::port will not get changed after kDisposeWindow but a bit later, so
-//  we would get an invalid port handle to a kSetPort call. We just patch in
-//  resetting of the port selector. We destroy the stop/fade code in there,
-//  it seems it isn't used at all in the game.
-const byte hoyle4SignaturePortFix[] = {
-	28,
-	0x39, 0x09,        // pushi 09
-	0x89, 0x0b,        // lsg 0b
-	0x39, 0x64,        // pushi 64
-	0x38, 0xc8, 0x00,  // pushi 00c8
-	0x38, 0x2c, 0x01,  // pushi 012c
-	0x38, 0x90, 0x01,  // pushi 0190
-	0x38, 0xf4, 0x01,  // pushi 01f4
-	0x38, 0x58, 0x02,  // pushi 0258
-	0x38, 0xbc, 0x02,  // pushi 02bc
-	0x38, 0x20, 0x03,  // pushi 0320
-	0x46,              // calle [xxxx] [xxxx] [xx]
-	+5, 43,            // [skip 5 bytes]
-	0x30, 0x27, 0x00,  // bnt 0027 -> end of routine
-	0x87, 0x00,        // lap 00
-	0x30, 0x19, 0x00,  // bnt 0019 -> fade out
-	0x87, 0x01,        // lap 01
-	0x30, 0x14, 0x00,  // bnt 0014 -> fade out
-	0x38, 0xa7, 0x00,  // pushi 00a7
-	0x76,              // push0
-	0x80, 0x29, 0x01,  // lag 0129
-	0x4a, 0x04,        // send 04 - call song::stop
-	0x39, 0x27,        // pushi 27
-	0x78,              // push1
-	0x8f, 0x01,        // lsp 01
-	0x51, 0x54,        // class 54
-	0x4a, 0x06,        // send 06 - call PlaySong::play
-	0x33, 0x09,        // jmp 09 -> end of routine
-	0x38, 0xaa, 0x00,  // pushi 00aa
-	0x76,              // push0
-	0x80, 0x29, 0x01,  // lag 0129
-	0x4a, 0x04,        // send 04
-	0x48,              // ret
-	0
-};
-
-const uint16 hoyle4PatchPortFix[] = {
-	PATCH_ADDTOOFFSET | +33,
-	0x38, 0x31, 0x01,  // pushi 0131 (selector curEvent)
-	0x76,              // push0
-	0x80, 0x50, 0x00,  // lag 0050 (global var 80h, "User")
-	0x4a, 0x04,        // send 04 - read User::curEvent
-
-	0x38, 0x93, 0x00,  // pushi 0093 (selector port)
-	0x78,              // push1
-	0x76,              // push0
-	0x4a, 0x06,        // send 06 - write 0 to that object::port
-	0x48,              // ret
-	PATCH_END
-};
-
-//    script, description,                                   magic DWORD,                                 adjust
-const SciScriptSignature hoyle4Signatures[] = {
-    {      0, "port fix when disposing windows",             PATCH_MAGICDWORD(0x64, 0x38, 0xC8, 0x00),    -5, hoyle4SignaturePortFix,   hoyle4PatchPortFix },
-    {      0, NULL,                                          0,                                            0, NULL,                     NULL }
-};
-
-#endif
-
 // ===========================================================================
 // at least during harpy scene export 29 of script 0 is called in kq5cd and
 //  has an issue for those calls, where temp 3 won't get inititialized, but
@@ -826,50 +709,10 @@ const uint16 qfg1vgaPatchFightEvents[] = {
 	PATCH_END
 };
 
-// FIXME:
-// When QFG1VGA and QFG3 dispose of a child window. For example, when choosing
-// a spell (parent window), if the spell can't be casted, a subsequent window
-// opens, notifying that it can't be casted. When showing the child window, the
-// scripts restore the area below the parent window, draw the child window, and
-// then attempt to redraw the parent window, which leads to the background
-// picture (which has just been restored) overwriting the child window.
-//
-// This faulty redraw is caused by a used and freed SaveBits handle that is
-// still stored in spellWin::pUnderBits being re-assigned with a SaveBits
-// call for the child window. We should ensure that invalidated SaveBits handles
-// can't (soon?) become valid again.
-//
-// However, we can just remove the window redraw and update calls when the
-// window is supposed to be disposed, and the window is disposed of correctly.
-// This is a workaround for bug #3053093.
-const byte qfg1vgaWindowDispose[] = {
-	17,
-	0x39, 0x05,       // pushi 05
-	0x39, 0x0d,       // pushi 0d
-	0x67, 0x2e,       // pTos 2e
-	0x67, 0x30,       // pTos 30
-	0x67, 0x32,       // pTos 32
-	0x67, 0x34,       // pTos 34
-	0x43, 0x6c, 0x0a, // callk kGraph 10
-	0x39, 0x06,       // pushi 06
-	0
-};
-
-const uint16 qfg1vgaPatchWindowDispose[] = {
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x33, 0x3e,       // jmp 0x3e (skip 62 bytes - this skips the subsequent 2 kGraph(update) calls, before kDisposeWindow is invoked)
-	PATCH_END
-};
-
 //    script, description,                                      magic DWORD,                                  adjust
 const SciScriptSignature qfg1vgaSignatures[] = {
 	{    215, "fight event issue",                           1, PATCH_MAGICDWORD(0x6d, 0x76, 0x51, 0x07),    -1, qfg1vgaSignatureFightEvents,       qfg1vgaPatchFightEvents },
 	{    216, "weapon master event issue",                   1, PATCH_MAGICDWORD(0x6d, 0x76, 0x51, 0x07),    -1, qfg1vgaSignatureFightEvents,       qfg1vgaPatchFightEvents },
-	{    559, "window dispose",                              1, PATCH_MAGICDWORD(0x39, 0x05, 0x39, 0x0d),     0,        qfg1vgaWindowDispose,     qfg1vgaPatchWindowDispose },
 	SCI_SIGNATUREENTRY_TERMINATOR
 };
 
@@ -933,37 +776,6 @@ const uint16 qfg3PatchImportDialog[] = {
 	PATCH_END
 };
 
-// When QFG1VGA and QFG3 dispose of a child window. For example, when choosing
-// a spell (parent window), if the spell can't be casted, a subsequent window
-// opens, notifying that it can't be casted. When showing the child window, the
-// scripts restore the area below the parent window, draw the child window, and
-// then attempt to redraw the parent window, which leads to the background
-// picture (which has just been restored) overwriting the child window. It
-// appers that kGraph(redrawBox) is different in QFG1VGA and QFG3. However, we
-// can just remove the window redraw and update calls when the window is
-// supposed to be disposed, and the window is disposed of correctly. Fixes bug
-// #3053093.
-const byte qfg3WindowDispose[] = {
-	15,
-	0x39, 0x05,       // pushi 05
-	0x39, 0x0d,       // pushi 0d
-	0x67, 0x2e,       // pTos 2e
-	0x67, 0x30,       // pTos 30
-	0x67, 0x32,       // pTos 32
-	0x67, 0x34,       // pTos 34
-	0x43, 0x6c, 0x0a, // callk kGraph 10
-	0
-};
-
-const uint16 qfg3PatchWindowDispose[] = {
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	0x34, 0x00, 0x00, // ldi 0000 (dummy)
-	PATCH_END
-};
-
 // Script 23 in QFG3 has a typo/bug which makes it loop endlessly and
 // read garbage. Fixes bug #3040722.
 const byte qfg3DialogCrash[] = {
@@ -982,7 +794,6 @@ const uint16 qfg3PatchDialogCrash[] = {
 
 //    script, description,                                      magic DWORD,                                  adjust
 const SciScriptSignature qfg3Signatures[] = {
-	{     22, "window dispose",                                 1, PATCH_MAGICDWORD(0x39, 0x05, 0x39, 0x0d),   0,         qfg3WindowDispose,        qfg3PatchWindowDispose },
 	{     23, "dialog crash",                                   1, PATCH_MAGICDWORD(0xe7, 0x03, 0x22, 0x33),  -1,           qfg3DialogCrash,          qfg3PatchDialogCrash },
 	{    944, "import dialog continuous calls",                 1, PATCH_MAGICDWORD(0x2a, 0x31, 0x0b, 0x7a),  -1, qfg3SignatureImportDialog,         qfg3PatchImportDialog },
 	SCI_SIGNATUREENTRY_TERMINATOR
@@ -1242,12 +1053,6 @@ int32 Script::findSignature(const SciScriptSignature *signature, const byte *scr
 void Script::matchSignatureAndPatch(uint16 scriptNr, byte *scriptData, const uint32 scriptSize) {
 	const SciScriptSignature *signatureTable = NULL;
 	switch (g_sci->getGameId()) {
-	// Dr. Brain now works because we properly maintain the state of the string heap in savegames
-#if 0
-	case GID_CASTLEBRAIN:
-		signatureTable = castlebrainSignatures;
-		break;
-#endif
 	case GID_ECOQUEST:
 		signatureTable = ecoquest1Signatures;
 		break;
@@ -1263,12 +1068,6 @@ void Script::matchSignatureAndPatch(uint16 scriptNr, byte *scriptData, const uin
 	case GID_GK1:
 		signatureTable = gk1Signatures;
 		break;
-	// hoyle4 now works due to workaround inside GfxPorts
-#if 0
-	case GID_HOYLE4:
-		signatureTable = hoyle4Signatures;
-		break;
-#endif
 	case GID_KQ5:
 		signatureTable = kq5Signatures;
 		break;
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index ffc81f0fde..f77bdf684e 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -403,7 +403,7 @@ void SegManager::freeHunkEntry(reg_t addr) {
 		return;
 	}
 
-	ht->freeEntry(addr.offset);
+	ht->freeEntryContents(addr.offset);
 }
 
 reg_t SegManager::allocateHunkEntry(const char *hunk_type, int size) {
diff --git a/engines/sci/engine/segment.cpp b/engines/sci/engine/segment.cpp
index 05d914cffb..d1f56f0da2 100644
--- a/engines/sci/engine/segment.cpp
+++ b/engines/sci/engine/segment.cpp
@@ -275,6 +275,11 @@ Common::Array<reg_t> DataStack::listAllOutgoingReferences(reg_t object) const {
 }
 
 
+//-------------------- hunk ---------------------
+void HunkTable::freeAtAddress(SegManager *segMan, reg_t sub_addr) {
+	freeEntry(sub_addr.offset);
+}
+
 //-------------------- lists --------------------
 void ListTable::freeAtAddress(SegManager *segMan, reg_t sub_addr) {
 	freeEntry(sub_addr.offset);
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index 9aaa3a4b08..56a6c71590 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -315,15 +315,18 @@ struct ListTable : public Table<List> {
 struct HunkTable : public Table<Hunk> {
 	HunkTable() : Table<Hunk>(SEG_TYPE_HUNK) {}
 
-	virtual void freeEntry(int idx) {
-		Table<Hunk>::freeEntry(idx);
-
-		if (!_table[idx].mem)
-			warning("Attempt to free an already freed hunk");
+	void freeEntryContents(int idx) {
 		free(_table[idx].mem);
 		_table[idx].mem = 0;
 	}
 
+	virtual void freeEntry(int idx) {
+		Table<Hunk>::freeEntry(idx);
+		freeEntryContents(idx);
+	}
+
+	virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr);
+
 	virtual void saveLoadWithSerializer(Common::Serializer &ser);
 };
 
diff --git a/engines/sci/engine/workarounds.cpp b/engines/sci/engine/workarounds.cpp
index 64cbc5ec90..c87ab59f8f 100644
--- a/engines/sci/engine/workarounds.cpp
+++ b/engines/sci/engine/workarounds.cpp
@@ -386,10 +386,7 @@ const SciWorkaroundEntry kStrLen_workarounds[] = {
 
 //    gameID,           room,script,lvl,          object-name, method-name,    call,index,                workaround
 const SciWorkaroundEntry kUnLoad_workarounds[] = {
-	{ GID_CAMELOT,       921,   921,  1,             "Script", "changeState",  0x36,    0, { WORKAROUND_IGNORE, 0 } }, // DEMO: While showing Camelot (and other places), the reference is invalid - bug #3035000
-	{ GID_CAMELOT,       921,   921,  1,             "Script", "init",         0x36,    0, { WORKAROUND_IGNORE, 0 } }, // DEMO: When being attacked by the boar (and other places), the reference is invalid - bug #3035000
-	{ GID_CASTLEBRAIN,   320,   377,  0,              "SWord", "upDate",         -1,    0, { WORKAROUND_IGNORE, 0 } }, // after solving the cross-word-puzzle, trying to unload invalid reference
-	{ GID_CASTLEBRAIN,   320,   377,  0,            "theWord", "show",           -1,    0, { WORKAROUND_IGNORE, 0 } }, // 2nd word puzzle, when exiting before solving, trying to unload invalid reference - bug #3034473
+	// TODO: Some of these workarounds for invalid references can now be removed, test which ones
 	{ GID_ECOQUEST,      380,    61,  0,              "gotIt", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // after talking to the dolphin the first time
 	{ GID_ECOQUEST,      380,    69,  0,   "lookAtBlackBoard", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // German version, when closing the blackboard closeup in the dolphin room - bug #3098353
 	{ GID_LAURABOW2,       1,     1,  0,           "sCartoon", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // DEMO: during the intro, a 3rd parameter is passed by accident - bug #3034902
@@ -400,7 +397,6 @@ const SciWorkaroundEntry kUnLoad_workarounds[] = {
 	{ GID_LSL6,          130,   130,  0,    "recruitLarryScr", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // during intro, a 3rd parameter is passed by accident
 	{ GID_LSL6,          740,   740,  0,        "showCartoon", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // during ending, 4 additional parameters are passed by accident
 	{ GID_LSL6HIRES,     130,   130,  0,    "recruitLarryScr", "changeState",    -1,    0, { WORKAROUND_IGNORE, 0 } }, // during intro, a 3rd parameter is passed by accident
-	{ GID_PQ3,           877,   998,  0,               "View", "delete",         -1,    0, { WORKAROUND_IGNORE, 0 } }, // when getting run over on the freeway, the reference is invalid
 	{ GID_SQ1,            43,   303,  0,            "slotGuy", "dispose",        -1,    0, { WORKAROUND_IGNORE, 0 } }, // when leaving ulence flats bar, parameter 1 is not passed - script error
 	{ GID_SQ3,             2,   998,  0,               "View", "delete",         -1,    0, { WORKAROUND_IGNORE, 0 } }, // clicking the mouse button during the intro, after the escape pod gets pulled into the garbage freighter, the reference is invalid - bug #3050856
 	SCI_WORKAROUNDENTRY_TERMINATOR
diff --git a/engines/sci/graphics/paint16.cpp b/engines/sci/graphics/paint16.cpp
index 935dd4e62e..53c43d94c3 100644
--- a/engines/sci/graphics/paint16.cpp
+++ b/engines/sci/graphics/paint16.cpp
@@ -360,7 +360,7 @@ void GfxPaint16::bitsRestore(reg_t memoryHandle) {
 
 		if (memoryPtr) {
 			_screen->bitsRestore(memoryPtr);
-			_segMan->freeHunkEntry(memoryHandle);
+			bitsFree(memoryHandle);
 		}
 	}
 }
@@ -532,20 +532,7 @@ reg_t GfxPaint16::kernelDisplay(const char *text, int argc, reg_t *argv) {
 		case SCI_DISPLAY_RESTOREUNDER:
 			bitsGetRect(argv[0], &rect);
 			rect.translate(-_ports->getPort()->left, -_ports->getPort()->top);
-			if (g_sci->getGameId() == GID_PQ3 && g_sci->getEngineState()->currentRoomNumber() == 29) {
-				// WORKAROUND: PQ3 calls this without calling the associated
-				// kDisplay(SCI_DISPLAY_SAVEUNDER) call before. Theoretically,
-				// this would result in no rect getting restored. However, we
-				// still maintain a pointer from the previous room, resulting
-				// in invalidated content being restored on screen, and causing
-				// graphics glitches. Thus, we simply don't restore a rect in
-				// that room. The correct fix for this would be to erase hunk
-				// pointers when changing rooms, but this will suffice for now,
-				// as restoring from a totally invalid pointer is very rare.
-				// Fixes bug #3037945.
-			} else {
-				bitsRestore(argv[0]);
-			}
+			bitsRestore(argv[0]);
 			kernelGraphRedrawBox(rect);
 			// finishing loop
 			argc = 0;
diff --git a/engines/sci/graphics/ports.h b/engines/sci/graphics/ports.h
index 21c6d31ebd..b94d54ab10 100644
--- a/engines/sci/graphics/ports.h
+++ b/engines/sci/graphics/ports.h
@@ -49,6 +49,9 @@ enum {
 	SCI_WINDOWMGR_STYLE_USER        = (1 << 7)
 };
 
+typedef Common::List<Port *> PortList;
+typedef Common::Array<Port *> PortArray;
+
 /**
  * Ports class, includes all port managment for SCI0->SCI1.1 games. Ports are some sort of windows in SCI
  *  this class also handles adjusting coordinates to a specific port
@@ -115,8 +118,12 @@ public:
 
 	virtual void saveLoadWithSerializer(Common::Serializer &ser);
 
+	/** The list of open 'windows' (and ports), in visual order. */
+	PortList _windowList;
+
 private:
-	typedef Common::List<Port *> PortList;
+	/** The list of all open 'windows' (and ports), ordered by their id. */
+	PortArray _windowsById;
 
 	SegManager *_segMan;
 	GfxPaint16 *_paint16;
@@ -130,12 +137,6 @@ private:
 	// counts windows that got disposed but are not freed yet
 	uint16 _freeCounter;
 
-	/** The list of open 'windows' (and ports), in visual order. */
-	PortList _windowList;
-
-	/** The list of all open 'windows' (and ports), ordered by their id. */
-	Common::Array<Port *> _windowsById;
-
 	Common::Rect _bounds;
 
 	// Priority Bands related variables