fix: crash when replay file is deleted during version mismatch prompt

[bobtista]

• Fixes Deleting a replay during the compatibility prompt leads to a crash #1732

Testing (Done)

Reproduction steps verified:

1. Select a replay with different executable hash/version
2. Double-click or press Load button
3. While version mismatch prompt is visible, delete the replay file externally
4. Click OK on the prompt
5. Should not crash, should show "REPLAY NOT FOUND" error message and refresh the replay list

[Stubbjax]

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action	Refreshes list	Message
DELETE REPLAY	Yes	The system cannot find the specified file.
LOAD REPLAY	No	This replay cannot be loaded because the file no longer exists on this device.

[bobtista]

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action Refreshes list Message
DELETE REPLAY Yes The system cannot find the specified file.
LOAD REPLAY No This replay cannot be loaded because the file no longer exists on this device.

Thank you - I implemented it this way, rebased the latest main, and force pushed

[greptile-apps]

Greptile Summary

Fixes a crash when a replay file is deleted externally after version mismatch prompt appears. The fix has two components: (1) delays setting playback mode in RecorderClass::playbackFile() until after successful file validation to prevent NULL pointer access during updates, and (2) adds pre-validation in reallyLoadReplay() using GetFileAttributes() to detect file deletion before attempting playback, displaying an error message and refreshing the replay list instead of crashing.

• Changed RECORDERMODETYPE_PLAYBACK assignment to occur after readReplayHeader() succeeds
• Added file existence check using GetFileAttributes() before calling playbackFile()
• Menu now only hides when playbackFile() returns true
• Added needsListRefresh flag to trigger list refresh after file-not-found errors
• Used 0xFFFFFFFF literal instead of INVALID_FILE_ATTRIBUTES for VC6 compatibility
• Changes replicated identically to both Generals and GeneralsMD codebases

Confidence Score: 4/5

• This PR is safe to merge with minor considerations about error handling consistency
• The fix correctly addresses the crash by preventing NULL pointer access and adding defensive file validation. The logic is sound and follows existing patterns in the codebase. Minor deduction for the known limitation that error messages from GetLastError() after playbackFile() failures may not always be accurate (as noted in previous review threads), though this PR doesn't worsen that issue and the new file-not-found path works correctly.
• No files require special attention

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/Common/Recorder.cpp	Moves m_mode = RECORDERMODETYPE_PLAYBACK after file validation to prevent crash if file is NULL during playback updates
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp	Adds pre-playback file existence check and conditional menu hiding; uses 0xFFFFFFFF for VC6 compatibility instead of INVALID_FILE_ATTRIBUTES
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp	Identical change to vanilla Generals: delays playback mode assignment until after successful file validation
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp	Identical change to vanilla Generals: adds file existence validation and conditional menu hiding with VC6-compatible constant

_{Last reviewed commit: c50a803}

[greptile-apps]

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Wrong error source

In reallyLoadReplay, the failure path uses GetLastError() to build the UI message after TheRecorder->playbackFile(asciiFilename) returns false. playbackFile() fails for multiple reasons (e.g., invalid header/GameInfo) and doesn’t necessarily set the Win32 last-error to something meaningful, so this can show an unrelated/empty message even when the replay exists. Consider deriving the message from the known failure (e.g., explicitly handle “file not found” via an existence check on the replay path, or have playbackFile/readReplayHeader provide a reason) instead of relying on GetLastError() here.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
Line: 539:542

Comment:
**Wrong error source**

In `reallyLoadReplay`, the failure path uses `GetLastError()` to build the UI message after `TheRecorder->playbackFile(asciiFilename)` returns false. `playbackFile()` fails for multiple reasons (e.g., invalid header/GameInfo) and doesn’t necessarily set the Win32 last-error to something meaningful, so this can show an unrelated/empty message even when the replay exists. Consider deriving the message from the known failure (e.g., explicitly handle “file not found” via an existence check on the replay path, or have `playbackFile`/`readReplayHeader` provide a reason) instead of relying on `GetLastError()` here.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Wrong error source

Same issue as vanilla: this failure message is built from GetLastError() after TheRecorder->playbackFile(...) returns false, but the recorder failure isn’t guaranteed to set a relevant Win32 last-error. This can surface an incorrect/empty error message when the file exists but the header/GameInfo parse fails; it should use a recorder-provided reason or an explicit file-not-found check for this UI.

Prompt To Fix With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
Line: 539:542

Comment:
**Wrong error source**

Same issue as vanilla: this failure message is built from `GetLastError()` after `TheRecorder->playbackFile(...)` returns false, but the recorder failure isn’t guaranteed to set a relevant Win32 last-error. This can surface an incorrect/empty error message when the file exists but the header/GameInfo parse fails; it should use a recorder-provided reason or an explicit file-not-found check for this UI.

How can I resolve this? If you propose a fix, please make it concise.

[bobtista]

@greptileai

[bobtista]

The latest push does this:

• Use readReplayMapInfo() to validate the file before playback (same pattern as loadReplay)
• Show the "REPLAY NOT FOUND" message that loadReplay uses
• Refresh the listbox on failure (matching deleteReplay behavior)

[bobtista]

There is already an established pathway for handling file deletion during a confirmation popup in ReplayMenu::delateReplay. Adopting this logic would simplify the implementation and resolve the message and list refresh inconsistencies of this change, which I've tabulated below for clarity.

Action Refreshes list Message
DELETE REPLAY Yes The system cannot find the specified file.
LOAD REPLAY No This replay cannot be loaded because the file no longer exists on this device.

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

[Stubbjax]

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

Is the latest push actually pushed? Issues still persist:

1. The NOT FOUND message is inconsistent between load and delete actions.
2. When attempting to load a replay, the game crashes after clicking OK on the NOT FOUND message.

[bobtista]

The latest push uses readReplayMapInfo() to validate the file before playback (like loadReplay), shows the "REPLAY NOT FOUND" message, and refreshes the listbox on failure. Both the message and list refresh should now be consistent with deleteReplay. Ready for re-review when you get a chance.

Is the latest push actually pushed? Issues still persist:

1. The NOT FOUND message is inconsistent between load and delete actions.
2. When attempting to load a replay, the game crashes after clicking OK on the NOT FOUND message.

Yes latest was pushed, but you're right. RE Inconsistent messages — I'll switch to using the same FormatMessage/GetLastError pattern that deleteReplay uses, with "GUI:Error" as the title. RE Crash after clicking OK, I think that's something else - I'll investigate.