Skip to content

bugfix(system): Update crash message - #2069

Draft
JohnsterID wants to merge 7 commits into
TheSuperHackers:mainfrom
JohnsterID:improve-crash-message
Draft

bugfix(system): Update crash message#2069
JohnsterID wants to merge 7 commits into
TheSuperHackers:mainfrom
JohnsterID:improve-crash-message

Conversation

@JohnsterID

Copy link
Copy Markdown

No description provided.

@JohnsterID
JohnsterID force-pushed the improve-crash-message branch from ef722a3 to 24d2f11 Compare January 8, 2026 11:01
@JohnsterID
JohnsterID marked this pull request as ready for review January 8, 2026 21:05
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
@JohnsterID
JohnsterID force-pushed the improve-crash-message branch from 24d2f11 to 85a17f6 Compare January 10, 2026 21:13
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker System Is Systems related labels Jan 11, 2026
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 11, 2026
Add -testcrash command line flag to deliberately trigger a null pointer
dereference crash. This allows capturing screenshots of the crash dialog
for PR TheSuperHackers#2069 review.

Usage: generalszh.exe -testcrash

This commit should NOT be merged to main - it exists only for testing
and documentation purposes.
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
… strings

Add null and empty string checks after calling g_LastErrorDump.str() to
handle cases where isEmpty() doesn't work reliably across different build
configurations and STL implementations.

Issue discovered in testing: Location field showed different values across builds
when calling ReleaseCrash() without exception context:
- Win32 Release: Empty (correct)
- VC6 Release: Shows 'T' (incorrect - isEmpty() failed)
- Win32 Debug: Shows corrupted data (incorrect - isEmpty() failed)

The additional check ensures consistent behavior across all builds.
This doesn't affect real crashes - exceptions always populate g_LastErrorDump
through DumpExceptionInfo() which properly clears and fills the string.

Relates to TheSuperHackers#2069
@JohnsterID
JohnsterID marked this pull request as draft January 12, 2026 00:25
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
… strings

Add null and empty string checks after calling g_LastErrorDump.str() to
handle cases where isEmpty() doesn't work reliably across different build
configurations and STL implementations.

Issue discovered in testing: Location field showed different values across builds
when calling ReleaseCrash() without exception context:
- Win32 Release: Empty (correct)
- VC6 Release: Shows 'T' (incorrect - isEmpty() failed)
- Win32 Debug: Shows corrupted data (incorrect - isEmpty() failed)

The additional check ensures consistent behavior across all builds.
This doesn't affect real crashes - exceptions always populate g_LastErrorDump
through DumpExceptionInfo() which properly clears and fills the string.

Relates to TheSuperHackers#2069
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
Add check to verify stack trace starts with two spaces (valid format from
WriteStackLine). This prevents extraction of garbage/uninitialized data from
g_LastErrorDump when ReleaseCrash() is called without exception context.

Issue: Previous defensive check (!stackStr || !*stackStr) didn't work because
g_LastErrorDump contained garbage data ('T', '^', corrupted bytes) which are
not NULL or empty - they're just invalid.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or leftover data. When ReleaseCrash() is called directly, DumpExceptionInfo()
never runs to clear() and populate it properly. Different STL implementations
(VC6/STLPort vs Win32) initialize globals differently, causing inconsistent
behavior across builds.

Solution: Valid stack traces from WriteStackLine() always start with "  "
(two spaces). By checking for this prefix, we can distinguish between:
- Valid exception stack traces: "  filename(line) : function" ✓
- Garbage data: "T", "^", corrupted bytes ✗

Test results before fix:
- VC6 Release: Shows "T" in Location field
- Win32 Release: Shows "^" in Location field
- Win32 Debug: Shows corrupted data in Location field

Expected after fix: All builds show empty Location field when no exception.

Relates to TheSuperHackers#2069
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
Add check to verify stack trace starts with two spaces (valid format from
WriteStackLine). This prevents extraction of garbage/uninitialized data from
g_LastErrorDump when ReleaseCrash() is called without exception context.

Issue: Previous defensive check (!stackStr || !*stackStr) didn't work because
g_LastErrorDump contained garbage data ('T', '^', corrupted bytes) which are
not NULL or empty - they're just invalid.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or leftover data. When ReleaseCrash() is called directly, DumpExceptionInfo()
never runs to clear() and populate it properly. Different STL implementations
(VC6/STLPort vs Win32) initialize globals differently, causing inconsistent
behavior across builds.

Solution: Valid stack traces from WriteStackLine() always start with "  "
(two spaces). By checking for this prefix, we can distinguish between:
- Valid exception stack traces: "  filename(line) : function" ✓
- Garbage data: "T", "^", corrupted bytes ✗

Test results before fix:
- VC6 Release: Shows "T" in Location field
- Win32 Release: Shows "^" in Location field
- Win32 Debug: Shows corrupted data in Location field

Expected after fix: All builds show empty Location field when no exception.

Relates to TheSuperHackers#2069
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
…) reliability

Explicitly call g_LastErrorDump.clear() in DebugInit() to ensure the global
AsciiString is properly initialized at program startup. This makes isEmpty()
work reliably across all build configurations.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or garbage data depending on STL implementation (VC6/STLPort vs Win32). Without
explicit initialization, isEmpty() returns false even though the string contains
invalid data, causing extractCrashLocation to extract garbage ('T', '^', etc).

Solution: Initialize to empty at startup. This is the correct fix rather than
trying to validate content format, because:
1. isEmpty() is the intended check - now it works correctly
2. No heuristics needed to detect valid vs invalid content
3. Simpler and more maintainable code
4. Guaranteed consistent behavior across all builds

Previous attempts:
- a316c10: Added NULL/empty check (didn't help - string had content)
- 5a2ee72: Added two-space format check (didn't help - garbage started with spaces)

This fix addresses the root cause by ensuring isEmpty() is reliable.

Test results before fix:
- VC6: Shows 'T' or '  T' in Location
- Win32: Shows '^' or '  ^' in Location
- Debug: Shows corrupted data in Location

Expected after fix: All builds show empty Location when no exception.

Relates to TheSuperHackers#2069
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 12, 2026
…) reliability

Explicitly call g_LastErrorDump.clear() in DebugInit() to ensure the global
AsciiString is properly initialized at program startup. This makes isEmpty()
work reliably across all build configurations.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or garbage data depending on STL implementation (VC6/STLPort vs Win32). Without
explicit initialization, isEmpty() returns false even though the string contains
invalid data, causing extractCrashLocation to extract garbage ('T', '^', etc).

Solution: Initialize to empty at startup. This is the correct fix rather than
trying to validate content format, because:
1. isEmpty() is the intended check - now it works correctly
2. No heuristics needed to detect valid vs invalid content
3. Simpler and more maintainable code
4. Guaranteed consistent behavior across all builds

Previous attempts:
- a316c10: Added NULL/empty check (didn't help - string had content)
- 5a2ee72: Added two-space format check (didn't help - garbage started with spaces)

This fix addresses the root cause by ensuring isEmpty() is reliable.

Test results before fix:
- VC6: Shows 'T' or '  T' in Location
- Win32: Shows '^' or '  ^' in Location
- Debug: Shows corrupted data in Location

Expected after fix: All builds show empty Location when no exception.

Relates to TheSuperHackers#2069
@xezon

xezon commented Jan 17, 2026

Copy link
Copy Markdown

Has merge conflicts.

Comment thread Core/GameEngine/Source/Common/System/Debug.cpp
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
Comment thread Core/GameEngine/Source/Common/System/Debug.cpp Outdated
JohnsterID added a commit to JohnsterID/GeneralsGameCode that referenced this pull request Jan 20, 2026
…ne/Xvfb testing setup

Review Findings:
- Code quality: 9/10 - Excellent defensive programming and safety
- Critical issue: Crash dialog not shown for real exceptions (80-90% of crashes)
- PR comments: 6 concerns require attention from latest review (Jan 17)

Key Issues Identified:
1. Comment TheSuperHackers#26: Simplify extractCrashLocation() - show 5 lines, remove filters
2. Comment TheSuperHackers#25: Buffer size confusion - 400 vs 512 char limit
3. Comment TheSuperHackers#23: 'Location: T' bug needs investigation
4. UnHandledExceptionFilter: Needs ReleaseCrash() call for real exceptions

Required Actions (4-6 hours):
- Refactor extractCrashLocation per Comment TheSuperHackers#26 (1-2h)
- Address buffer size concerns (30min)
- Fix 'Location: T' bug (30-60min)
- Add UnHandledExceptionFilter fix (30min)

Environment Setup:
- Wine 11.0, widl, MinGW-w64 GCC 14, Xvfb, CMake installed
- test-wine-xvfb.sh: Automated build and test script for Wine/Xvfb

Documents Analyzed:
- All 26 PR review comments from TheSuperHackers#2069
- TEST_CRASH_BRANCH_README.md (test-crash-dialog-capture)
- IMPORTANT_CRASH_DIALOG_ISSUE.md (test-crash-dialog-capture)
- CI_TEST_RESULTS_ANALYSIS.md (test-crash-dialog-capture)
- CRASH_REVIEW_ANALYSIS.md (crash-message-review-analysis)

Recommendation: NEEDS WORK before merge
Status: Testing environment ready, detailed review complete
- Change crash report URL to GeneralsCrashReports repository
- Fix buffer write order in extractCrashLocation to check bufferSize first
- Replace manual whitespace checks with standard isspace function
…handling

- Add comments explaining crash location extraction behavior
- Fix overly conservative Unknown rejection that discarded partial stacks
- Only reject if first line (crash location) is Unknown, not entire trace
- Document 400 char limit rationale (512 byte buffer with safety margin)

Previous behavior rejected any stack trace containing <Unknown> anywhere,
which discarded partially useful traces where the crash location had symbols
but deeper frames didn't. Now allows display of useful crash locations even
when some stack frames lack symbol information.
… strings

Add null and empty string checks after calling g_LastErrorDump.str() to
handle cases where isEmpty() doesn't work reliably across different build
configurations and STL implementations.

Issue discovered in testing: Location field showed different values across builds
when calling ReleaseCrash() without exception context:
- Win32 Release: Empty (correct)
- VC6 Release: Shows 'T' (incorrect - isEmpty() failed)
- Win32 Debug: Shows corrupted data (incorrect - isEmpty() failed)

The additional check ensures consistent behavior across all builds.
This doesn't affect real crashes - exceptions always populate g_LastErrorDump
through DumpExceptionInfo() which properly clears and fills the string.

Relates to TheSuperHackers#2069
Add check to verify stack trace starts with two spaces (valid format from
WriteStackLine). This prevents extraction of garbage/uninitialized data from
g_LastErrorDump when ReleaseCrash() is called without exception context.

Issue: Previous defensive check (!stackStr || !*stackStr) didn't work because
g_LastErrorDump contained garbage data ('T', '^', corrupted bytes) which are
not NULL or empty - they're just invalid.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or leftover data. When ReleaseCrash() is called directly, DumpExceptionInfo()
never runs to clear() and populate it properly. Different STL implementations
(VC6/STLPort vs Win32) initialize globals differently, causing inconsistent
behavior across builds.

Solution: Valid stack traces from WriteStackLine() always start with "  "
(two spaces). By checking for this prefix, we can distinguish between:
- Valid exception stack traces: "  filename(line) : function" ✓
- Garbage data: "T", "^", corrupted bytes ✗

Test results before fix:
- VC6 Release: Shows "T" in Location field
- Win32 Release: Shows "^" in Location field
- Win32 Debug: Shows corrupted data in Location field

Expected after fix: All builds show empty Location field when no exception.

Relates to TheSuperHackers#2069
…) reliability

Explicitly call g_LastErrorDump.clear() in DebugInit() to ensure the global
AsciiString is properly initialized at program startup. This makes isEmpty()
work reliably across all build configurations.

Root cause: g_LastErrorDump is a global variable that may contain uninitialized
or garbage data depending on STL implementation (VC6/STLPort vs Win32). Without
explicit initialization, isEmpty() returns false even though the string contains
invalid data, causing extractCrashLocation to extract garbage ('T', '^', etc).

Solution: Initialize to empty at startup. This is the correct fix rather than
trying to validate content format, because:
1. isEmpty() is the intended check - now it works correctly
2. No heuristics needed to detect valid vs invalid content
3. Simpler and more maintainable code
4. Guaranteed consistent behavior across all builds

Previous attempts:
- a316c10: Added NULL/empty check (didn't help - string had content)
- 5a2ee72: Added two-space format check (didn't help - garbage started with spaces)

This fix addresses the root cause by ensuring isEmpty() is reliable.

Test results before fix:
- VC6: Shows 'T' or '  T' in Location
- Win32: Shows '^' or '  ^' in Location
- Debug: Shows corrupted data in Location

Expected after fix: All builds show empty Location when no exception.

Relates to TheSuperHackers#2069
…ptionFilter crash dialog

Addresses review comments TheSuperHackers#7-11:
- Simplified extractCrashLocation to show first 5 lines of stack trace
- Removed arbitrary 400 char limit, now uses full buffer (bufferSize - 1)
- Fixed buffer initialization order to prevent 'Location: T' garbage
- Updated g_LastErrorDump comment to clarify static initialization timing
- Show <Unknown> frames as-is without filtering
- Added ReleaseCrash call to UnHandledExceptionFilter so crash dialog
  appears for unhandled exceptions, not just DEBUG_CRASH macro calls
@JohnsterID
JohnsterID force-pushed the improve-crash-message branch from 0e72c06 to ad38778 Compare March 18, 2026 08:38
@JohnsterID
JohnsterID marked this pull request as ready for review March 22, 2026 07:13
@greptile-apps

greptile-apps Bot commented Mar 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR updates the crash dialog shown to players by replacing the stale EA-era message (referencing defunct EA forums) with a modern, informative one that shows the crash report file path, optional minidump directory, a stack-trace excerpt when debug symbols are available, and a link to the community crash-report GitHub repository. Both UnHandledExceptionFilter functions (Generals and Zero Hour) are also updated to call ReleaseCrash so that unhandled Win32 exceptions now surface the same dialog instead of silently exiting.

Key changes:

  • extractCrashLocation helper added to pull the first 5 lines of g_LastErrorDump for display in the crash dialog.
  • ReleaseCrash rebuilt with a structured message-building loop; ReleaseCrashLocalized appends the same paths to localized messages.
  • g_LastErrorDump.clear() now called at startup (guarded by DEBUG_STACKTRACE) to prevent uninitialized data being shown in the dialog on early crashes.
  • Both UnHandledExceptionFilter implementations now call ReleaseCrash("Unhandled exception") after the minidump is created, ensuring users always see the crash dialog.
  • Three new comment headers reference the date 06/01/2025, which is prior to the current year and should be updated (rule violation, also applies at lines 866 and 935 in Debug.cpp).
  • Two MessageBox/MessageBoxW calls changed from nullptr to NULL for the HWND parameter, conflicting with the project's nullptr-only style rule.

Confidence Score: 4/5

  • Safe to merge after addressing the minor style issues; the core logic is correct and well-guarded.
  • The functional changes are straightforward and beneficial — users now get actionable crash information. The only issues found are style-level: three comment dates referencing 2025 instead of 2026, and two MessageBox calls using NULL instead of nullptr. No logic errors or data-integrity concerns were identified.
  • Core/GameEngine/Source/Common/System/Debug.cpp — comment dates and nullptr/NULL inconsistencies.

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/System/Debug.cpp Core crash dialog logic rewritten — adds crash-path info, GitHub link, and stack-trace location excerpt. Three new comment headers are dated 2025 (violating the current-year rule) and two MessageBox calls switched from nullptr to NULL.
Generals/Code/Main/WinMain.cpp Adds a ReleaseCrash("Unhandled exception") call in UnHandledExceptionFilter so users see the new crash dialog for all unhandled exceptions, not just explicit DEBUG_CRASH calls.
GeneralsMD/Code/Main/WinMain.cpp Identical change to Generals/Code/Main/WinMain.cpp, applied to the Zero Hour variant of the exception filter.

Sequence Diagram

sequenceDiagram
    participant OS as Windows SEH
    participant UEF as UnHandledExceptionFilter
    participant MD as MiniDumper
    participant RC as ReleaseCrash
    participant MB as MessageBox

    OS->>UEF: Unhandled exception
    UEF->>UEF: DumpExceptionInfo()
    UEF->>MD: TriggerMiniDumpForException(Minimal)
    UEF->>MD: TriggerMiniDumpForException(Full)
    UEF->>MD: shutdownMiniDumper()
    UEF->>RC: ReleaseCrash("Unhandled exception") [NEW]
    RC->>RC: TriggerMiniDump() (guarded — dumper already shut down)
    RC->>RC: Build message (path + stack excerpt + GitHub link)
    RC->>MB: MessageBox("Game Crash", ...)
    MB-->>RC: User clicks OK
    RC->>RC: _exit(1)
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/System/Debug.cpp
Line: 908

Comment:
**NULL instead of nullptr**

`NULL` is used here for the `HWND` parameter, but the codebase uses `nullptr` for null pointer literals. The original code used `nullptr` for this same parameter; this change introduces an inconsistency.

```suggestion
	::MessageBox(nullptr, buff, "Game Crash", MB_OK|MB_SYSTEMMODAL|MB_ICONERROR);
```

**Rule Used:** Use nullptr instead of NULL for null pointer liter... ([source](https://app.greptile.com/review/custom-context?memory=c69cf1a9-c69a-4330-a013-69bb5d6701da))

**Learnt From**
[TheSuperHackers/GeneralsGameCode#2067](https://github.com/TheSuperHackers/GeneralsGameCode/pull/2067#discussion_r2703746722)

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

---

This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/System/Debug.cpp
Line: 959

Comment:
**NULL instead of nullptr**

`NULL` is used for the `HWND` parameter to `MessageBoxW`, which expects a pointer type. Use `nullptr` to stay consistent with the project style and the original code.

```suggestion
		::MessageBoxW(nullptr, fullMessage.str(), prompt.str(), MB_OK|MB_SYSTEMMODAL|MB_ICONERROR);
```

**Rule Used:** Use nullptr instead of NULL for null pointer liter... ([source](https://app.greptile.com/review/custom-context?memory=c69cf1a9-c69a-4330-a013-69bb5d6701da))

**Learnt From**
[TheSuperHackers/GeneralsGameCode#2067](https://github.com/TheSuperHackers/GeneralsGameCode/pull/2067#discussion_r2703746722)

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

---

This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/System/Debug.cpp
Line: 757

Comment:
**Comment date references 2025**

The comment date `06/01/2025` is prior to the current year (2026). All three newly added comment headers in this file carry the same 2025 date (`extractCrashLocation` at line 757, `ReleaseCrash` at line 866, and `ReleaseCrashLocalized` at line 935). These should be updated to a 2026 date.

```suggestion
// TheSuperHackers @bugfix JohnsterID 06/01/2026 Helper function to extract crash location from stack trace.
```

**Rule Used:** What: Flag newly created code comments that refere... ([source](https://app.greptile.com/review/custom-context?memory=fd72a556-4fd8-4db4-8b08-8e51516a64ad))

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

Last reviewed commit: "Fix review feedback:..."

@Caball009
Caball009 marked this pull request as draft August 26, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Minor Severity: Minor < Major < Critical < Blocker System Is Systems related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants