From 032c5bb27c3e44902ff89abf389fb864a6e19c9c Mon Sep 17 00:00:00 2001
From: James R <justsomejames2@gmail.com>
Date: Sun, 10 Mar 2024 06:46:28 -0700
Subject: [PATCH] Replays: fix ghosts not being freed if Time Attacking and
 letting the map exit without manually returning to the menu

- Fixes a very specific crash
- Record a Time Attack
- Let the map exit naturally
  - I tested by FALLING OUT of a Sealed Star
- Start recording another Time Attack
  - I tested by going to the same map again
- Result
  - One of three possibilities
  - Z_Free: wrong id
  - SIGSEGV
  - Game freezes and hangs forever
- I also wrote detailed comments in M_EndModeAttackRun so
  you know what it's supposed to be doing
---
 src/menus/transient/pause-replay.c | 44 ++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/menus/transient/pause-replay.c b/src/menus/transient/pause-replay.c
index ab30bac23..411643c46 100644
--- a/src/menus/transient/pause-replay.c
+++ b/src/menus/transient/pause-replay.c
@@ -54,12 +54,41 @@ menu_t PAUSE_PlaybackMenuDef = {
 
 void M_EndModeAttackRun(void)
 {
-	if (demo.playback)
+	// End recording / playback.
+	// Why not check demo.recording?
+	// Because for recording, this may be called from G_AfterIntermission.
+	// And before this function is called, G_SaveDemo is called, which sets demo.recording to false.
+	// Don't need to check demo.playback; G_CheckDemoStatus is safe to call even outside of demos.
+	// Check modeattacking because this function is recursively called (read on for an explanation).
+	if (modeattacking)
 	{
-		G_CheckDemoStatus(); // Cancel recording
-		return;
+		// This must be called for both playback and
+		// recording, because it both finishes playback and
+		// frees ghost data.
+		G_CheckDemoStatus();
+
+		// What does G_CheckDemoStatus do? Here's the answer!
+
+		// Playback:
+		// - Clears everything, including demo state and modeattacking.
+		// - It then calls the current function (M_EndModeAttackRun) AGAIN (after everything was cleared), so return.
+		if (!modeattacking)
+			return;
+
+		// Recording:
+		// - Only saves the demo and clears the demo state.
+		// - Now we need to clear the rest of the gamestate ourself!
 	}
 
+	// Playback: modeattacking is always false, so calling this returns to the menu.
+	// Recording: modeattacking is still true and this function call preserves that.
+	Command_ExitGame_f();
+
+	if (!modeattacking)
+		return;
+
+	// The rest of this is relevant for recording ONLY.
+
 	if (nextmapoverride != 0)
 	{
 		M_StartMessage(
@@ -76,13 +105,10 @@ void M_EndModeAttackRun(void)
 		);
 	}
 
-	Command_ExitGame_f(); // Clear a bunch of state
-
-	if (!modeattacking)
-		return;
-
-	modeattacking = ATTACKING_NONE; // Kept until now because of Command_ExitGame_f
+	// Command_ExitGame_f didn't clear this, so now we do.
+	modeattacking = ATTACKING_NONE;
 
+	// Return to the menu.
 	if (demo.attract == DEMO_ATTRACT_TITLE)
 	{
 		D_SetDeferredStartTitle(true);