feat(crashreporting): Add optional local Crashpad backend - #3413
CryoTheRenegade wants to merge 5 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughAdds optional Crashpad reporting for supported Win32 game builds. Both games use a shared crash-reporting interface with MiniDumper fallback. The change also adds local report tools, build and symbol artifact handling, CI collection, documentation, and validation. ChangesCrashpad reporting
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant WinMain
participant CrashReporting
participant CrashpadBridge as rts_crashpad.dll
participant CrashpadHandler as Crashpad handler
participant CrashpadDatabase as Crashpad database
WinMain->>CrashReporting: initialize with user directory and version
CrashReporting->>CrashpadBridge: load DLL and resolve exports
CrashpadBridge->>CrashpadDatabase: create database with uploads disabled
CrashpadBridge->>CrashpadHandler: start handler and wait for IPC ping
WinMain->>CrashReporting: notify when user directory is ready
WinMain->>CrashReporting: captureFatal on fatal error
CrashReporting->>CrashpadBridge: request fatal dump
CrashpadBridge->>CrashpadHandler: capture dump
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable crash-handling regression is established in the reviewed change; the localized null path is not reached by its current in-repository caller. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The optional backend has meaningful local trust and lifecycle boundaries, but remains disabled by default and explicitly disables uploads. No introduced exploitable security path was established. Installation permissions, handler privileges, and concurrent cleanup behavior remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
I just scrolled over this and there is a lot of complicated stuff added. What was the prompt to create this?
Maybe this should be reprompted.
There was a problem hiding this comment.
Why is this here? Is this something that belongs into Google Test instead?
| name: ${{ inputs.game }}-${{ inputs.preset }}${{ inputs.tools && '+t' || '' }}${{ inputs.extras && '+e' || '' }} | ||
| path: build\${{ inputs.preset }}\${{ inputs.game }}\artifacts | ||
| retention-days: 30 | ||
| retention-days: 90 |
| # Neither the engine nor tools link the Crashpad client or DLL import library. | ||
| target_compile_definitions(${engine} PRIVATE RTS_USE_CRASHPAD=1) | ||
| target_include_directories(${engine} PRIVATE "${CMAKE_SOURCE_DIR}/Dependencies/Crashpad") | ||
| # Refresh runtime files before linking, even when only the DLL changed. | ||
| add_custom_target(${game}_crashpad_runtime | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory "$<TARGET_FILE_DIR:${game}>" | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different | ||
| "$<TARGET_FILE:rts_crashpad>" "$<TARGET_PDB_FILE:rts_crashpad>" | ||
| "${RTS_CRASHPAD_HANDLER}" | ||
| ${RTS_CRASHPAD_RUNTIME_FILES} "$<TARGET_FILE_DIR:${game}>" | ||
| COMMAND ${CMAKE_COMMAND} -E copy_directory | ||
| "${RTS_CRASHPAD_NOTICES}" "$<TARGET_FILE_DIR:${game}>/crashpad-notices" | ||
| DEPENDS rts_crashpad | ||
| VERBATIM) | ||
| add_dependencies(${game} ${game}_crashpad_runtime) | ||
| endfunction() |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This will be addressed in a follow up pr, the goal right now is implementation of the backend, fallback, and deployment.
2 participants
Adds optional, local-only Crashpad reporting. It is disabled by default, and MiniDumper remains the default backend and startup fallback (can be changed in future).
The aim is to reduce reliance on the crashing game process by capturing dumps in a separate handler. Developers still inspect reports with matching symbols. This change does not yet establish better capture reliability than MiniDumper.
The implementation includes:
The existing replay from #3185 produced a real gameplay crash that produced a crash dump
Build instructions are in
docs/crashpad.mdAI disclosure: AI (GPT-Astra) assisted the implementation, documentation, and testing of this change.