From 9f59f5c495f80432d9fd70c224fa88f86f30e6ab Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:57:04 +0200 Subject: [PATCH] [CrashReporting] Breakpad capture, session detection and symbol upload (#7523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CrashReporting] Breakpad everywhere, session detection, symbol CI (inc 3) Breakpad on all platforms (no crashpad packaging); remove the Windows MiniDumpWithFullMemory filter and the installer WER LocalDumps full-dump registration. QLockFile per-process session tracking replaces the Windows-only .dmp scan, placed after parser.process and the instance hand-off so help/version exits and handed-off secondaries never leak a lock. CI: Release-only DSN bake from SENTRY_DSN secret, tag-only sentry-cli symbol upload (org cockatrice, project native). Tests: 533/533 ctest green, format clean, code gate PASS. * [CrashReporting] Enable http_retry, harden symbol upload, fix NSIS context Sets sentry_options_set_http_retry so the cache_keep-held crash envelope in Ask mode is written with a retry filename and actually flushes on giveConsent() instead of sitting in /cache until pruning. Adds continue-on-error to both sentry-cli install steps so a Sentry registry outage or broken Git Bash install cannot block release assets. Cleans the legacy WER CrashDumps folder under SetShellVarContext current (under all it resolves to ProgramData, not the user's AppData) in installer and uninstaller; registry delete works in either context. * [CrashReporting] Extract the startup crash-report flow from main() per review * [CrashReporting] Break up the CrashReporter class doc per review * [CrashReporting] Drop the invalid --release from the symbol upload `debug-files upload` never accepted `--release` (checked against the pinned sentry-cli 3.8.0: the subcommand declares --org/--project and a set of type/id/scan options, and execute() reads no release; the only global flags are --url/--header/--auth-token/--log-level/--quiet/--allow-failure). Clap rejects the unknown flag before any upload happens, so every tag build has been shipping without symbols, with continue-on-error keeping the release pipeline green. Release association is also the wrong mechanism: debug information files are matched to events by debug ID via the event's debug_meta. The Sentry release of a native event comes from the event itself (COCKATRICE_SENTRY_RELEASE), not from the DIF upload. Remove the flag and the now-unused SENTRY_RELEASE env from both invocations (Linux, and Windows/macOS). The symbol upload stays continue-on-error (a Sentry outage must not block the release assets); revisit once it has proven itself on a tag. * [CrashReporting] Split debug information out of packaged release binaries -g is required so the CI symbol upload has DWARF to send, but it was left in the shipped binaries too: the Linux Release client is 563 MiB, 552 MiB of it debug info, and every distribution channel pays for that. Keep the build tree unchanged (CI uploads symbols from build/) and strip only the packaged copy. A CPack pre-build script extracts each ELF's debug info to COCKATRICE_DEBUG_SYMBOL_DIR and then runs `strip --strip-debug --strip-unneeded`. .eh_frame is allocatable and survives, so unwinding is unaffected, and the build ID is identical in the stripped binary and the sidecar. Measured on a local Release build (GCC -O2 -g, WITH_CRASH_REPORTING=ON), bytes: binary build tree packaged before packaged after .debug sidecar cockatrice 590,722,560 590,722,560 28,086,480 579,623,576 oracle 53,432,608 53,432,608 1,649,888 52,075,344 tar.gz of both trees: 289,964,521 -> 19,018,058 (~15x smaller). Only ELF (Linux/BSD) is handled: Windows already links /DEBUG with separate PDBs, and macOS extracts a dSYM from the bundle. Opt-out via COCKATRICE_SPLIT_DEBUG_SYMBOLS; skipped with a warning when objcopy/strip are missing. CPACK_PRE_BUILD_SCRIPTS (not CPACK_INSTALL_SCRIPTS, which runs before staging) invokes it, and the script refuses to run unless CPACK_TEMPORARY_DIRECTORY is a real CPack staging tree so a missing variable can never turn its glob into a filesystem-wide walk. * [CrashReporting] Upload Windows PDBs from where Ninja actually puts them The Windows branch of the symbol upload looked in build//Release, which is the Visual Studio generator's layout. The Windows leg uses Ninja, which is single-config: linker PDBs land in build//, next to the binaries — the same paths the "[Windows] Upload PDBs" artifact step below globs with if-no-files-found: error. So paths was always empty on Windows, the step printed "No symbol paths found, skipping symbol upload" and exited 0: a silent no-op behind continue-on-error. Every Windows build would then report "A required debug information file was missing" for cockatrice.exe with a PDB-style debug id (GUID-age), exactly as if no upload step existed at all. Use the artifact step's globs. Also turn the empty-path case into a failure in both symbol steps: a silent skip is indistinguishable from a successful upload (which is how the wrong path went unnoticed), and continue-on-error already keeps a broken upload from failing the release. Verified by extracting both steps' run blocks and executing the Windows branch against a Ninja-style tree: it now passes the three linker PDBs to sentry-cli (and not the CMakeFiles/*.pdb intermediates), and exits non-zero with an ::error:: annotation when there is nothing to upload. * [CrashReporting] Archive PDBs on tag builds so a failed upload is recoverable The "[Windows] Upload PDBs" artifact step skipped tag builds (github.ref_type != 'tag'), so for a release the symbols existed only inside the Sentry upload — which is continue-on-error. A failed upload therefore lost the ability to symbolicate that release permanently: no rebuild produces the same PDB, since the debug id is a per-build GUID+age. That is how every Windows crash reported so far became unreadable. Archive the PDBs for tag builds as well, and order the step after "Upload to release" so a missing PDB can never block a release from being published. Retention stays at the repository default; nothing consumes the artifact automatically, it is the manual recovery path (gh run download -n -PDBs, then sentry-cli debug-files upload). Verified by re-parsing the workflow and asserting the step order (release -> PDBs -> attest) and the broadened condition. * [CrashReporting] Upload the Windows executables with their PDBs Sentry needs two things per image on Windows: symbolication and stack unwinding. The PDB covers symbolication, but for a 64-bit PE the Call Frame Information (.pdata/.xdata) lives in the executable and is not copied into the PDB, so uploading PDBs alone leaves the image at "Stack Unwinding: Missing". That is exactly what the event's Images panel showed for cockatrice.exe, while Sentry's own Microsoft symbol server resolved KERNELBASE.dll and ucrtbase.dll from the same dump. sentry-cli also needs the pair in the same invocation: only then does it associate the PE's code identifier with the PDB's debug identifier, otherwise "the identifiers diverge and Sentry might not be able to resolve the correct file for symbolication" (docs: Debug Identifiers). Do the same for macOS, where the dSYM carries the debug information but the Mach-O carries the unwind information, and include the executables in the symbol artifact so a re-upload still has the pair to offer. Verified by extracting the step and running both branches against stub harnesses: Windows passes the three exe/pdb pairs in a single sentry-cli invocation and still fails loudly on an empty tree; macOS passes the dSYM and the Mach-O together. * [CrashReporting] Upload the shipped Qt runtime DLLs too Qt ships no PDBs with its MSVC SDK, so every Qt frame in a crash report resolved to nothing: an image with neither PDB nor PE has no unwind information (on 64-bit Windows the Call Frame Information lives in the PE and is not copied into the PDB) and no symbol source to fall back on. The list is required-runtime.txt, the manifest .ci/compile.sh already asserts the installer against, so what gets uploaded is exactly what an install carries. Qt frames now get unwinding plus export-table names; line numbers still need Qt's debug information files, which the SDK omits. The guards are not cosmetic: the step runs under `bash -eo pipefail`, where a sed over a missing file or an unmatched ls glob aborts the whole step, and continue-on-error makes that look identical to a successful upload. * [CrashReporting] Upload only the binaries, archive symbols only on tags The Linux leg handed sentry-cli whole build directories: 471 candidate files, of which 7 were object files that can never match a stack frame. Only the three linked binaries carry the build-ids Sentry looks up; the packaged copy is split and stripped but keeps that same build-id. The Windows symbol archive ran on every packaging build even though only tag builds upload to Sentry, so a PR build paid for an artifact nothing could ever consume. Tags keep it as the durable copy for a failed upload. --------- Co-authored-by: Lukas Brübach --- .ci/compile.sh | 8 + .github/workflows/desktop-build.yml | 222 +++++++++++++++++- cmake/CrashReporting.cmake | 60 ++++- cmake/NSIS.template.in | 28 ++- cmake/SplitDebugSymbols.cmake.in | 80 +++++++ .../src/client/crash/crash_reporter.cpp | 90 ++++++- cockatrice/src/client/crash/crash_reporter.h | 54 +++-- cockatrice/src/main.cpp | 183 +++++---------- tests/crash_reporter_test.cpp | 79 +++++++ 9 files changed, 648 insertions(+), 156 deletions(-) create mode 100644 cmake/SplitDebugSymbols.cmake.in diff --git a/.ci/compile.sh b/.ci/compile.sh index 2aeeaa6ec..63c027473 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -206,6 +206,14 @@ if [[ ${WITH_CRASH_REPORTING:-} == "ON" ]]; then # developer builds never reach Sentry. flags+=("-DWITH_CRASH_REPORTING=ON") fi +if [[ ${BUILDTYPE:-Release} == "Release" && -n ${COCKATRICE_SENTRY_DSN:-} ]]; then + # Release-only: Debug builds stay DSN-free so crashes from developer builds + # never reach Sentry. Empty (local builds, forks without the secret, and non-tag + # CI builds, which never receive the secret) keeps today's mock fallback via the + # compiled-in default. CI passes the value from the SENTRY_DSN secret on tag + # Release builds only, so pre-release test artifacts never report to Sentry. + flags+=("-DCOCKATRICE_SENTRY_DSN=$COCKATRICE_SENTRY_DSN") +fi # Add cmake --build flags buildflags=(--config "$BUILDTYPE") diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 2215ac2d2..c97b48342 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -46,6 +46,11 @@ jobs: outputs: tag: ${{ steps.configure.outputs.tag }} sha: ${{ steps.configure.outputs.sha }} + # MAJ.MIN.PATCH[-beta[.N]] of a tag build; empty otherwise. Mirrors what + # cmake/getversion.cmake derives as PROJECT_VERSION, which the client bakes as its + # Sentry release (cmake/CrashReporting.cmake) — symbol uploads must use the same + # string so stack traces de-symbolicate against this build's binaries. + sentry_release: ${{ steps.configure.outputs.sentry_release }} steps: - name: "Configure" @@ -56,6 +61,9 @@ jobs: run: | if [[ "$GITHUB_REF_TYPE" == 'tag' ]]; then # release echo "tag=$GITHUB_REF_NAME" >> "$GITHUB_OUTPUT" + # Tag format is YYYY-MM-DD-{Release|Development}-MAJ.MIN.PATCH[-beta[.N]]; + # PROJECT_VERSION is the tail after the type (see cmake/getversion.cmake). + echo "sentry_release=$(sed -E 's/^[0-9]{4}-[0-9]{2}-[0-9]{2}-(Release|Development)-//' <<< "$GITHUB_REF_NAME")" >> "$GITHUB_OUTPUT" fi echo "sha=$RESOLVED_SHA" >> "$GITHUB_OUTPUT" @@ -240,6 +248,11 @@ jobs: package: '${{ matrix.package }}' server_only: '${{ matrix.server_only }}' WITH_CRASH_REPORTING: 'ON' + # Tag builds only: artifacts from master pushes and PR branches must not report to + # Sentry. Forks get no secrets on pull_request, so they keep the mock fallback either way. + # configure.outputs.tag is set for ANY tag, so beta (Development) tags get the DSN + # and the symbol upload below just like full releases. + COCKATRICE_SENTRY_DSN: ${{ needs.configure.outputs.tag != null && secrets.SENTRY_DSN || '' }} run: | source .ci/docker.sh args=() @@ -248,9 +261,58 @@ jobs: args+=(--ccache "$CCACHE_SIZE") args+=(--cmake-generator "$CMAKE_GENERATOR") args+=(--suffix "$SUFFIX") + # The container inherits no environment, so the DSN rides in explicitly. -e without + # =value passes it through (possibly empty); compile.sh only consumes it for Release + # configures, and empty keeps the mock fallback. + export RUN_ARGS="${RUN_ARGS:-} -e COCKATRICE_SENTRY_DSN" RUN --server --release --package "$package" "${args[@]}" + - name: "Install sentry-cli" + if: matrix.package != 'skip' && needs.configure.outputs.tag != null + # Downloads from Sentry's release registry: a Sentry outage or a broken install under + # Git Bash must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_CLI_VERSION: '3.8.0' + run: | + export INSTALL_DIR="$GITHUB_WORKSPACE/.sentry-cli" + curl -sL https://sentry.io/get-cli/ | bash + echo "$GITHUB_WORKSPACE/.sentry-cli" >> "$GITHUB_PATH" + + - name: "Upload debug symbols to Sentry" + # Like the DSN above, this runs for every tag — beta (Development) tags included. + if: matrix.package != 'skip' && needs.configure.outputs.tag != null + # A Sentry outage or rotated token must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + run: | + paths=() + # The binaries only, not the directories: a directory makes sentry-cli walk the + # whole build tree, where it finds (and uploads) object files and other fragments + # that can never match a stack frame. The packaged copy is split and stripped by + # cpack, but keeps this binary's GNU build-id, which is how Sentry matches it. + for app in cockatrice oracle servatrice; do + binary="build/$app/$app" + [[ -f $binary ]] && paths+=("$binary") + done + if [[ ${#paths[@]} == 0 ]]; then + # Fail loudly instead of skipping: see the Windows/macOS leg below — this step + # is the only way symbols reach Sentry and `continue-on-error` keeps the job + # green either way, so a silent skip looks exactly like a successful upload. + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + # No --release here: `debug-files upload` has no such option (sentry-cli errors on + # the unknown flag and uploads nothing), and debug information files are matched to + # events by debug ID via the event's debug_meta, not by release. The release grouping + # comes from the client event itself (COCKATRICE_SENTRY_RELEASE). + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" + # Delete used cache to emulate a ccache update. See https://github.com/actions/cache/issues/342 - name: "Delete remote compiler cache (ccache)" if: github.ref == 'refs/heads/master' && steps.restore_ccache.outputs.cache-hit @@ -536,6 +598,11 @@ jobs: BUILDTYPE: '${{ matrix.type }}' CCACHE_EVICTION_AGE: ${{ matrix.ccache_eviction_age }} CMAKE_GENERATOR: ${{ matrix.cmake_generator }} + # Tag builds only: artifacts from master pushes and PR branches must not report to + # Sentry. Forks get no secrets on pull_request, so they keep the mock fallback either way. + # configure.outputs.tag is set for ANY tag, so beta (Development) tags get the DSN + # and the symbol upload below just like full releases. + COCKATRICE_SENTRY_DSN: ${{ needs.configure.outputs.tag != null && secrets.SENTRY_DSN || '' }} DEVELOPER_DIR: '/Applications/Xcode_${{ matrix.xcode }}.app/Contents/Developer' GITHUB_TOKEN: ${{ github.token }} # needed for vcpkg dependency graph updates, see VCPKG_FEATURE_FLAGS MACOS_CERTIFICATE: ${{ secrets.PROD_MACOS_CERTIFICATE }} @@ -563,6 +630,121 @@ jobs: WITH_CRASH_REPORTING: ${{ matrix.type == 'Release' && 'ON' || '' }} run: .ci/compile.sh --server --test --vcpkg + - name: "Install sentry-cli" + if: needs.configure.outputs.tag != null && matrix.make_package == '1' + # Same as the Linux leg above: the install downloads from Sentry's registry, so a + # failure here must not block the release upload below. + continue-on-error: true + shell: bash + env: + SENTRY_CLI_VERSION: '3.8.0' + run: | + export INSTALL_DIR="$GITHUB_WORKSPACE/.sentry-cli" + curl -sL https://sentry.io/get-cli/ | bash + echo "$GITHUB_WORKSPACE/.sentry-cli" >> "$GITHUB_PATH" + + - name: "Upload debug symbols to Sentry" + # Like the DSN above, this runs for every tag — beta (Development) tags included. + if: needs.configure.outputs.tag != null && matrix.make_package == '1' + # A Sentry outage or rotated token must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + run: | + paths=() + if [[ '${{ matrix.os }}' == 'Windows' ]]; then + # Top-level CMakeLists.txt links /DEBUG even in release, so the linker PDBs + # land next to the binaries. Ninja is single-config, so that is + # build// and NOT build//Release/ — the same globs the symbol + # artifact step below uses. + # + # The executables are uploaded as well, in the same invocation. On 64-bit + # Windows the Call Frame Information (.pdata/.xdata) lives in the PE and is + # not copied into the PDB, so a PDB on its own leaves Sentry reporting + # "Stack Unwinding: Missing" for the image. sentry-cli also needs to see the + # PE and its PDB together: only then does it associate the PE's code + # identifier with the PDB's debug identifier. + shopt -s nullglob + for target in cockatrice oracle servatrice; do + paths+=("build/$target"/*.exe "build/$target"/*.pdb) + done + shopt -u nullglob + + # Qt's own runtime DLLs, taken from required-runtime.txt — the manifest + # .ci/compile.sh asserts the installer against — so the set uploaded is + # the set an install actually carries. The PE is the only carrier of + # unwind information for a 64-bit image, and sending it in the same + # invocation as a PDB is what lets sentry-cli associate the PE's code + # identifier with the PDB's debug identifier. Qt ships no PDBs with the + # SDK, so Qt frames resolve through the export table: names, no lines. + qt_dir= + qt_core_dir= + # Existence guards matter: the step runs under `bash -eo pipefail`, so a + # redirection-less sed over a missing file would abort the step outright + # and continue-on-error would hide it. + if [[ -f build/CMakeCache.txt ]]; then + qt_core_dir=$(sed -n 's/^Qt6Core_DIR:PATH=//p' build/CMakeCache.txt | head -1) + fi + if [[ -n $qt_core_dir ]]; then + qt_dir=$(dirname "$(dirname "$(dirname "$qt_core_dir")")") + fi + if [[ -z $qt_dir || ! -d $qt_dir ]]; then + # Warnings, not failures: the upload of our own binaries is what this + # step is for, and continue-on-error already means an upload failure + # cannot be told apart from a successful one. + echo "::warning::Qt runtime directory not resolved from build/CMakeCache.txt; Qt images get no unwind information" + elif [[ ! -f build/required-runtime.txt ]]; then + echo "::warning::build/required-runtime.txt is missing; cannot tell which Qt DLLs ship" + else + while IFS= read -r runtime; do + name=${runtime##*/} + if [[ $runtime == */* ]]; then + # A Qt plugin: the install root keeps it as plugins//. + # `|| true`: an unmatched glob makes ls exit non-zero, and pipefail + # turns that into a step abort. + runtime_path=$(ls "$qt_dir/plugins"/*/"$name" 2>/dev/null | head -1) || true + else + runtime_path="$qt_dir/bin/$name" + fi + if [[ -f ${runtime_path:-} ]]; then + paths+=("$runtime_path") + else + echo "::warning::shipped Qt runtime $runtime not found under $qt_dir" + fi + done < <(grep -E '^(Qt6[^/]*\.dll|Plugins/)' build/required-runtime.txt) + fi + else + # Release builds carry DWARF (-g, kept by CrashReporting.cmake); macOS keeps its + # debug info in dSYM bundles, so materialize them before uploading. + for app in cockatrice oracle servatrice; do + binary="build/$app/$app.app/Contents/MacOS/$app" + if [[ -f $binary ]]; then + if [[ '${{ matrix.os }}' == 'macOS' ]]; then + dsymutil "$binary" -o "$binary.dSYM" + paths+=("$binary.dSYM") + # The dSYM holds the debug information, but the unwind information + # Sentry needs to walk a minidump stack lives in the Mach-O itself. + paths+=("$binary") + else + paths+=("$binary") + fi + fi + done + fi + if [[ ${#paths[@]} == 0 ]]; then + # Fail loudly instead of skipping: this step is the only way symbols reach + # Sentry, and `continue-on-error` keeps the job green either way, so a + # silent skip is indistinguishable from a successful upload. It also hid a + # wrong Windows path (build//Release, which Ninja never creates). + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + # No --release: see the Linux leg above — the flag does not exist on + # `debug-files upload` and DIFs are matched by debug ID, not release. + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" + # CPack reports a failed makensis run as "Problem running NSIS command" # plus a path to NSISOutput.log, and that log lives only on the runner, so # the actual compile error is otherwise nowhere in the workflow output. @@ -706,19 +888,6 @@ jobs: if-no-files-found: error path: ${{ steps.build.outputs.path }} - # Ninja is single-config: binaries and their PDBs land directly in the - # target directories instead of a Release subdirectory. - - name: "[Windows] Upload PDBs (Program Databases)" - if: matrix.os == 'Windows' && github.ref_type != 'tag' - uses: actions/upload-artifact@v7 - with: - if-no-files-found: error - name: ${{ steps.build.outputs.name }}-PDBs - path: | - build/cockatrice/*.pdb - build/oracle/*.pdb - build/servatrice/*.pdb - - name: "Upload to release" if: needs.configure.outputs.tag != null && matrix.make_package == '1' id: upload_release @@ -730,6 +899,33 @@ jobs: tag_name: ${{ needs.configure.outputs.tag }} run: gh release upload "$tag_name" "$asset_path#$asset_name" + # Ninja is single-config: binaries and their PDBs land directly in the + # target directories instead of a Release subdirectory. + # + # Archived for tag builds only, and ordered after the release upload so + # a missing PDB can never block a release from being published. On a tag + # build this artifact is the only durable copy of the symbols: the Sentry + # upload is continue-on-error, so without it a failed upload would lose + # the ability to symbolicate that release permanently. Other builds upload + # no symbols at all, so archiving them there would only cost storage. + # + # The executables travel with their PDBs: re-uploading the pair together is + # what lets sentry-cli associate the PE's code identifier with the PDB's + # debug identifier, and the PE is what carries the unwind information. + - name: "[Windows] Upload debug symbols (exe + pdb)" + if: matrix.os == 'Windows' && needs.configure.outputs.tag != null + uses: actions/upload-artifact@v7 + with: + if-no-files-found: error + name: ${{ steps.build.outputs.name }}-Symbols + path: | + build/cockatrice/*.exe + build/cockatrice/*.pdb + build/oracle/*.exe + build/oracle/*.pdb + build/servatrice/*.exe + build/servatrice/*.pdb + - name: "Attest binary provenance" if: steps.upload_release.outcome == 'success' id: attestation diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake index abd029cc2..db016269f 100644 --- a/cmake/CrashReporting.cmake +++ b/cmake/CrashReporting.cmake @@ -46,12 +46,14 @@ if(WITH_CRASH_REPORTING AND WITH_CLIENT) if(ENABLE_SANITIZERS) # The crash handler cannot run under ASan/UBSan, so there is no backend to capture with. set(_sentry_backend "none") - elseif(WIN32 OR APPLE) - # Out-of-process handler; shipping the handler binary is a later increment, which unifies - # every OS on breakpad (in-process, no extra binary to ship). - set(_sentry_backend "crashpad") else() - # In-process, no extra binary to ship. + # Breakpad on every OS (increment 3 probe, 2026-10-14): the crashpad sources + # do vendor friction-free (FetchContent inits all submodules recursively by + # default, verified populated in a local build tree), but the crashpad backend + # would need its out-of-process crashpad_handler binary installed next to the + # exe, into the macOS bundle, and through NSIS, plus handler-path wiring and + # notarization care. Breakpad is in-process, needs no extra binary and no + # packaging on any OS sentry-native builds, so simplicity wins. set(_sentry_backend "breakpad") endif() # FORCE so toggling ENABLE_SANITIZERS on a reused build dir cannot leave a stale backend behind. @@ -61,6 +63,18 @@ if(WITH_CRASH_REPORTING AND WITH_CLIENT) ) unset(_sentry_backend) + if(NOT MSVC) + # Symbol upload needs debug info in Release binaries: the top-level flags build GCC + # Release as -s -O2 (stripped, no DWARF) and Clang/Apple Release as plain -O2 (no DWARF), + # both of which upload nothing useful. Keep -g and drop the strip flag while the SDK is + # on; the OFF default leaves local, distro and Debug builds untouched. MSVC already + # links /DEBUG with PDBs in release (see top-level CMakeLists.txt), so it needs nothing. + string(REGEX REPLACE "(^| )-s( |$)" "\\1\\2" CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE}") + if(NOT CMAKE_CXX_FLAGS_RELEASE MATCHES "(^| )-g( |$)") + set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -g") + endif() + endif() + # No Qt integration: it runs find_package(Qt6) inside the vendored build, which # cannot see our thin/aqt Qt layouts (macOS CI) under foreign toolchains, and its # breadcrumbs could carry unredacted log lines into native crash events that bypass @@ -129,3 +143,39 @@ else() "(WITH_CRASH_REPORTING=${WITH_CRASH_REPORTING}, WITH_CLIENT=${WITH_CLIENT})" ) endif() + +# Packaged release binaries must not ship the DWARF that the symbol upload needs: it is +# 80-90% of an unstripped binary and nobody downloads a 500 MB installer to get it. The +# build tree keeps -g (CI uploads symbols from there); the *packaged copy* is split at +# install time by a CPack install script, which extracts the debug info next to the staging +# dir and strips the binary. Only ELF (Linux/BSD) needs this: Windows links /DEBUG with +# separate PDBs, and macOS keeps its dSYM extracted from the bundle. +option(COCKATRICE_SPLIT_DEBUG_SYMBOLS "Strip DWARF out of packaged release binaries" ON) +set(COCKATRICE_DEBUG_SYMBOL_DIR + "${CMAKE_BINARY_DIR}/debug-symbols" + CACHE PATH "Where the packaged binaries' split debug information is written (kept out of the package)" +) +if(WITH_CRASH_REPORTING + AND WITH_CLIENT + AND COCKATRICE_SPLIT_DEBUG_SYMBOLS + AND CMAKE_BUILD_TYPE STREQUAL "Release" + AND UNIX + AND NOT APPLE +) + find_program(COCKATRICE_OBJCOPY NAMES objcopy) + find_program(COCKATRICE_STRIP_TOOL NAMES strip) + if(COCKATRICE_OBJCOPY AND COCKATRICE_STRIP_TOOL) + configure_file( + "${CMAKE_CURRENT_LIST_DIR}/SplitDebugSymbols.cmake.in" "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake" @ONLY + ) + # PRE_BUILD runs after CPack stages the install tree and before the package is produced. + # CPACK_INSTALL_SCRIPTS would run too early (staging is still empty); the script walks + # CPACK_TEMPORARY_DIRECTORY, never CMAKE_INSTALL_PREFIX, and refuses to run otherwise. + list(APPEND CPACK_PRE_BUILD_SCRIPTS "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake") + message(STATUS "Crash reporting: packaged binaries stripped, debug info split to ${COCKATRICE_DEBUG_SYMBOL_DIR}") + else() + message(WARNING "COCKATRICE_SPLIT_DEBUG_SYMBOLS is ON but objcopy/strip were not found; " + "packaged release binaries will keep their debug information" + ) + endif() +endif() diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 811a3cb7f..3647303b9 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -1530,11 +1530,22 @@ ${If} $PortableMode = 0 ${GetSize} "$INSTDIR" "/S=0K" $0 $1 $2 IntFmt $0 "0x%08X" $0 - ; Enable Windows User-Mode Dumps + ; No Windows Error Reporting LocalDumps registration: crash capture belongs to + ; the in-app reporter (Sentry/breakpad, MiniDumpNormal-class data only). Opting + ; machines into OS-level full dumps (DumpType 2) would leave unredacted memory + ; dumps on user machines that nothing consumes. ; https://learn.microsoft.com/en-us/windows/win32/wer/collecting-user-mode-dumps - WriteRegExpandStr HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpFolder" "%LOCALAPPDATA%\CrashDumps\Cockatrice" - WriteRegDWORD HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpCount" "5" - WriteRegDWORD HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpType" "2" + + ; Older installers did register cockatrice.exe above (and the removed + ; MiniDumpWithFullMemory writer filled that folder), and that key survives this + ; install, so WER would keep writing full dumps. Remove it, and clear the + ; legacy dumps nothing consumes anymore. $LOCALAPPDATA resolves per shell context: + ; under "all" it points at ProgramData, so switch to "current" for the per-user + ; dumps folder and back afterwards; the registry delete works in either context. + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + SetShellVarContext current + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" + SetShellVarContext all WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayIcon" "$INSTDIR\cockatrice.exe" WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayName" "Cockatrice" @@ -1635,6 +1646,15 @@ Section "un.Application" UnSecApplication DeleteRegKey HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" + ; Leftover WER LocalDumps key from pre-Sentry installers (see the installer note + ; above): removing it here covers machines that uninstall without reinstalling. + ; The legacy full-memory dumps go with it; nothing consumes them anymore. Same shell + ; context note as the installer: the per-user dumps folder needs "current". + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + SetShellVarContext current + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" + SetShellVarContext all + ; Only remove the file/protocol associations if we registered them (i.e. the ; install was not portable) and .cod is still owned by Cockatrice, so we don't ; clobber a .cod association installed by another application. diff --git a/cmake/SplitDebugSymbols.cmake.in b/cmake/SplitDebugSymbols.cmake.in new file mode 100644 index 000000000..bfe954712 --- /dev/null +++ b/cmake/SplitDebugSymbols.cmake.in @@ -0,0 +1,80 @@ +# Splits DWARF out of the packaged ELF binaries, then strips them. +# +# This is a CPack pre-build script (CPACK_PRE_BUILD_SCRIPTS, wired in CrashReporting.cmake): +# CPack runs it after staging the install tree and before producing the package, so it only +# ever touches the copy that becomes the release artifact. The build tree is left alone: -g +# stays on, and CI uploads symbols to Sentry straight from build/. +# +# For every ELF file under CPACK_TEMPORARY_DIRECTORY the script extracts the debug information +# to COCKATRICE_DEBUG_SYMBOL_DIR and removes the debug sections from the packaged binary. +# COCKATRICE_DEBUG_SYMBOL_DIR is deliberately outside the staging tree, so sidecars never end +# up in the package; CI can archive/upload them if wanted (it normally uploads straight from +# the build tree). The only thing that must survive stripping is .eh_frame (allocatable, used +# for unwinding), which neither --only-keep-debug nor --strip-debug removes. +# +# Tool paths, the symbol directory and the tree to walk are baked in / provided at configure / +# run time; the guards below refuse to run on anything that is not a CPack staging directory, +# so a missing variable can never turn the glob into a filesystem-wide walk. + +set(_objcopy "@COCKATRICE_OBJCOPY@") +set(_strip "@COCKATRICE_STRIP_TOOL@") +set(_symbolDir "@COCKATRICE_DEBUG_SYMBOL_DIR@") + +if(_objcopy STREQUAL "" OR _strip STREQUAL "") + message(WARNING "split debug symbols: objcopy/strip not found at configure time, packaging unstripped binaries") + return() +endif() +if(_symbolDir STREQUAL "") + message(FATAL_ERROR "split debug symbols: COCKATRICE_DEBUG_SYMBOL_DIR is empty") +endif() + +# CPack sets CPACK_TEMPORARY_DIRECTORY to the staging tree; CMAKE_INSTALL_PREFIX is still the +# configured /usr prefix here, so globbing it would walk system directories. Refuse to run +# unless the path exists and really is a CPack staging tree. +if(NOT CPACK_TEMPORARY_DIRECTORY OR NOT IS_DIRECTORY "${CPACK_TEMPORARY_DIRECTORY}") + message(STATUS "split debug symbols: no CPack staging directory, skipping") + return() +endif() +if(NOT CPACK_TEMPORARY_DIRECTORY MATCHES "_CPack_Packages") + message(WARNING "split debug symbols: ${CPACK_TEMPORARY_DIRECTORY} is not a CPack staging tree, skipping") + return() +endif() + +file(MAKE_DIRECTORY "${_symbolDir}") + +file( + GLOB_RECURSE _entries + LIST_DIRECTORIES true + "${CPACK_TEMPORARY_DIRECTORY}/*" +) +set(_processed 0) +foreach(_file IN LISTS _entries) + if(IS_DIRECTORY "${_file}" OR IS_SYMLINK "${_file}") + continue() + endif() + # Only ELF: objcopy would fail on the icons, translations and Qt resources next to it. + file( + READ "${_file}" _magic + OFFSET 0 + LIMIT 4 + HEX + ) + if(NOT _magic STREQUAL "7f454c46") + continue() + endif() + get_filename_component(_name "${_file}" NAME) + set(_debug "${_symbolDir}/${_name}.debug") + execute_process(COMMAND "${_objcopy}" --only-keep-debug "${_file}" "${_debug}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: objcopy --only-keep-debug failed for ${_file}") + continue() + endif() + execute_process(COMMAND "${_strip}" --strip-debug --strip-unneeded "${_file}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: strip failed for ${_file}") + continue() + endif() + math(EXPR _processed "${_processed} + 1") +endforeach() + +message(STATUS "Split debug symbols for ${_processed} packaged ELF file(s); sidecars in ${_symbolDir}") diff --git a/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp index 720bcd350..07a0389c7 100644 --- a/cockatrice/src/client/crash/crash_reporter.cpp +++ b/cockatrice/src/client/crash/crash_reporter.cpp @@ -5,12 +5,15 @@ #endif #include +#include #include #include #include #include +#include #include #include +#include #include #include #include @@ -263,8 +266,16 @@ void CrashReporter::init(const QString &dsn, sentry_options_set_release(options, release.toUtf8().constData()); sentry_options_set_database_path(options, sentryDatabasePath.toUtf8().constData()); // Consent-gated uploads: crashes are captured into the local database but - // nothing leaves the machine until giveConsent(). + // nothing leaves the machine until giveConsent(). cache_keep holds the previous run's + // crash envelope in /cache instead of dropping it at init, so the Ask dialog + // decides its fate: giveConsent() flushes it via the transport retry, revokeConsent() + // deletes it below. This also covers the first crash after switching to AlwaysSend, + // which giveConsent() (running after init) would otherwise miss. http_retry is required + // for the flush: without it the held envelope is written with a plain filename the retry + // scanner skips, and giveConsent()'s sentry_transport_retry() sends nothing. sentry_options_set_require_user_consent(options, 1); + sentry_options_set_cache_keep(options, SENTRY_CACHE_KEEP_OFFLINE); + sentry_options_set_http_retry(options, 1); sentry_options_set_before_send(options, scrubEventBeforeSend, nullptr); ready = (sentry_init(options) == 0); #else @@ -291,6 +302,79 @@ void CrashReporter::shutdown() ready = false; } +CrashReporter::~CrashReporter() = default; +CrashReporter::CrashReporter() = default; + +bool CrashReporter::beginSession(const QString &dir) +{ + // Restartable: releasing first keeps begin->end->begin round-trips clean. + endSession(); + + QDir().mkpath(dir); + const QDir sessionDir(dir); + const QString selfName = QStringLiteral("session-%1").arg(QCoreApplication::applicationPid()); + + bool unclean = false; + const QStringList leftovers = sessionDir.entryList({QStringLiteral("session-*")}, QDir::Files); + for (const QString &name : leftovers) { + if (name == selfName) { + continue; + } + const QString path = sessionDir.filePath(name); + QLockFile probe(path); + // Long-lived session lock: stale means "owner dead", regardless of file age, so a + // crash followed by an immediate relaunch (younger than the 30 s default) is still + // detected. tryLock() only deletes the file when it finds it stale, so a live + // concurrent instance's lock is never touched here. (The static + // QLockFile::removeStaleLockFile(path) form does not exist in Qt5/Qt6, and the + // member removeStaleLockFile() force-removes without a staleness check, so neither + // is usable for this scan.) + probe.setStaleLockTime(0); + if (probe.tryLock(0)) { + unclean = true; + probe.unlock(); // deletes the stale file + continue; + } + // tryLock failed: either a live concurrent instance owns the file, or it is + // unreadable (garbage/empty) and therefore has no live owner. Only the latter is + // cleaned up; a live instance is left alone. On Unix QLockFile creates the file + // before writing the pid, so a second instance starting at the same moment can + // briefly observe an empty (or half-written) file for a live owner: re-read once + // after a beat so a live owner's fresh lock is never mistaken for garbage and deleted. + qint64 pid = 0; + QString hostname, appname; + if (!probe.getLockInfo(&pid, &hostname, &appname)) { + QThread::msleep(100); + if (!probe.getLockInfo(&pid, &hostname, &appname)) { + QFile::remove(path); + unclean = true; + } + } + } + + sessionLockPath = sessionDir.filePath(selfName); + sessionLock = std::make_unique(sessionLockPath); + sessionLock->setStaleLockTime(0); + if (!sessionLock->tryLock(0)) { + qCWarning(CrashReportLog) << "Failed to claim session lock at" << sessionLockPath; + } + return unclean; +} + +void CrashReporter::endSession() +{ + if (sessionLock) { + // unlock() removes the lock file; nothing left to clean up. + sessionLock->unlock(); + sessionLock.reset(); + } else if (!sessionLockPath.isEmpty()) { + // The lock was never claimed (a live concurrent instance owns its file): only our + // own unclaimed file, if tryLock left one behind, is removed. + QFile::remove(sessionLockPath); + } + sessionLockPath.clear(); +} + void CrashReporter::giveConsent() { #if COCKATRICE_WITH_CRASH_REPORTING @@ -307,6 +391,10 @@ void CrashReporter::revokeConsent() // return to unknown, which blocks uploads too but loses the decision. if (ready) { sentry_user_consent_revoke(); + // Drop the held crash envelope: without this it would sit in /cache and + // upload on the next give (e.g. after switching to AlwaysSend), after the user + // explicitly refused it. The SDK recreates the directory as needed. + QDir(sentryDatabasePath + QStringLiteral("/cache")).removeRecursively(); } #endif } diff --git a/cockatrice/src/client/crash/crash_reporter.h b/cockatrice/src/client/crash/crash_reporter.h index 598b6445c..aecc6ade3 100644 --- a/cockatrice/src/client/crash/crash_reporter.h +++ b/cockatrice/src/client/crash/crash_reporter.h @@ -11,6 +11,7 @@ #include #include #include +#include #ifndef COCKATRICE_WITH_CRASH_REPORTING #define COCKATRICE_WITH_CRASH_REPORTING 0 @@ -24,31 +25,46 @@ #define COCKATRICE_SENTRY_RELEASE "" #endif +class QLockFile; + Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); /** * @brief Sentry-backed crash-report facade, owned by main(). * - * Single responsibility: init/shutdown the sentry-native SDK behind consent - * gating, plus the shared log redaction helpers. An empty DSN (or Never - * consent) skips the SDK entirely, so the mock dialog fallback behaves exactly - * as before. With a DSN, init() arms require-user-consent and before_send - * scrubbing (stale persisted consent from a previous AlwaysSend run is cleared - * before init when the mode is not AlwaysSend); giveConsent()/revokeConsent() - * gate uploads. attachToPendingCrashEnvelopes() folds the redacted log and the - * optional user note into the pending native crash event(s) before consent - * releases them, so dump, stack, log and note arrive as ONE Sentry event; - * sendReport() is the fallback for unclean exits that produced no native crash - * event, capturing a redacted message event with the log preview attached. All - * SDK calls are guarded so OFF builds compile to no-ops. + * An empty DSN (or Never consent) skips the SDK entirely, so the mock dialog + * fallback behaves exactly as before. All SDK calls are guarded so OFF builds + * compile to no-ops. + * + * @par Consent gating + * init() arms require-user-consent and before_send scrubbing. Stale persisted + * consent from a previous AlwaysSend run is cleared before init when the mode is + * not AlwaysSend, and cached crash envelopes are held for the dialog instead of + * being dropped. giveConsent()/revokeConsent() gate uploads. + * + * @par Envelope and log helpers + * attachToPendingCrashEnvelopes() folds the redacted log and the optional user + * note into the pending native crash event(s) before consent releases them, so + * dump, stack, log and note arrive as ONE Sentry event. sendReport() is the + * fallback for unclean exits that produced no native crash event, capturing a + * redacted message event with the log preview attached. The log redaction + * helpers are static and SDK-independent. + * + * @par Session tracking + * beginSession()/endSession() are SDK-independent: a QLockFile per process in + * the database dir detects unclean exits cross-platform, replacing the legacy + * Windows-only .dmp scan. The held lock makes the reporter non-copyable and + * non-movable. */ class CrashReporter { public: - CrashReporter() = default; + CrashReporter(); CrashReporter(const CrashReporter &) = delete; CrashReporter &operator=(const CrashReporter &) = delete; - ~CrashReporter() = default; + CrashReporter(CrashReporter &&) = delete; + CrashReporter &operator=(CrashReporter &&) = delete; + ~CrashReporter(); void init(const QString &dsn, const QString &release, @@ -140,12 +156,22 @@ public: */ static QString describeOriginMismatch(const QString &envelopePath, const QString ¤tDsn); + /** + * @brief Claim this process's session lock, reporting a previous unclean exit. + * @return true if a leftover session lock from a dead process (or an unreadable + * garbage file) was found and cleaned; live concurrent instances are ignored. + */ + bool beginSession(const QString &dir); + void endSession(); + static QString sanitizeLogLine(const QString &line); static QString redactCockatriceUrls(const QString &line); private: bool ready = false; QString sentryDatabasePath; + QString sessionLockPath; + std::unique_ptr sessionLock; }; #endif diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index fd98b1945..7b8648d69 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -46,9 +46,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -108,60 +105,6 @@ static void CockatriceLogger(QtMsgType type, const QMessageLogContext &ctx, cons Logger::getInstance().log(type, ctx, logMessage); } -#ifdef Q_OS_WIN -// clang-format off -#include -#include -#include -#include -#include - -#pragma comment(lib, "DbgHelp.lib") // Link the DbgHelp library -// clang-format on - -LONG WINAPI CockatriceUnhandledExceptionFilter(EXCEPTION_POINTERS *exceptionPointers) -{ - std::filesystem::path path; - - // Find %LOCALAPPDATA% (or cheat at finding it) - wchar_t *localAppDataFolder; - if (SHGetKnownFolderPath(FOLDERID_LocalAppData, KF_FLAG_CREATE, NULL, &localAppDataFolder) != S_OK) { - path = std::filesystem::temp_directory_path().parent_path().parent_path(); - } else { - path = std::filesystem::path(localAppDataFolder); - } - - // Plan on writing crash files into %LOCALAPPDATA%/CrashDumps/Cockatrice/cockatrice.crash.*.dmp - path /= "CrashDumps"; - path /= "Cockatrice"; - if (!std::filesystem::exists(path)) { - std::filesystem::create_directories(path); - } - path /= "cockatrice.crash." + std::to_string(std::time(0)) + ".dmp"; - - // Create and write crash files -#ifdef UNICODE - HANDLE hDumpFile = - CreateFile(path.wstring().c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); -#else - HANDLE hDumpFile = - CreateFile(path.string().c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); -#endif - - MINIDUMP_EXCEPTION_INFORMATION mei; - mei.ExceptionPointers = exceptionPointers; - mei.ThreadId = GetCurrentThreadId(); - mei.ClientPointers = 1; - - MiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(), hDumpFile, MiniDumpWithFullMemory, &mei, nullptr, - nullptr); - - CloseHandle(hDumpFile); - - return EXCEPTION_EXECUTE_HANDLER; -} -#endif - void installNewTranslator() { QString lang = SettingsCache::instance().personal().getLang(); @@ -202,11 +145,52 @@ static QString redactActivationUrl(const QString &url) return parsed.scheme() + "://" + parsed.host(); } +/** + * @brief Run the startup crash-report flow for an unclean exit (or --simulate-crash). + * + * With a configured DSN the SDK owns capture and upload (consent-gated); with an + * empty DSN the flow falls back to a local mock file. AlwaysSend uploads silently + * (the log was folded into the pending native envelope(s) at startup); Ask offers + * the dialog; NeverSend skips silently. + * + * @param attachedEnvelopes number of pending native envelopes that already carry the + * redacted log, so an AlwaysSend run only sends a standalone + * log event when no crash event existed. + */ +static void runCrashReportFlow(CrashReporter &crashReporter, QWidget *parent, int attachedEnvelopes) +{ + const PersonalSettings::CrashReportingMode crashMode = SettingsCache::instance().personal().getCrashReportingMode(); + const bool crashReady = crashReporter.isReady(); + if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { + if (crashReady) { + // Silent path: the redacted log was folded into the pending native crash + // envelope(s) at startup and consent already released them. Only when no + // pending crash event existed (e.g. --simulate-crash) does a standalone + // log event still need to go out here. + if (attachedEnvelopes == 0) { + const QString crashPreview = DlgCrashReport::buildRedactedPreview(); + if (!crashReporter.sendReport(crashPreview)) { + const QString savedPath = DlgCrashReport::writeMockReport(crashPreview); + qCWarning(MainLog) << "Failed to send crash report, saved locally to" << savedPath; + } + } + } else { + DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); + } + } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { + // Real mode: Send gives consent + captures the event, Don't send revokes + // consent; the dialog owns both via the reporter. + DlgCrashReport crashDialog(parent, crashReady, crashReady ? &crashReporter : nullptr); + crashDialog.exec(); + } + // NeverSend: skip silently (no dialog, nothing written or sent). +} + int main(int argc, char *argv[]) { -#ifdef Q_OS_WIN - SetUnhandledExceptionFilter(CockatriceUnhandledExceptionFilter); -#endif + // No custom unhandled-exception filter: sentry owns native crash capture on all + // platforms. (The legacy MiniDumpWithFullMemory writer was removed as a privacy + // liability; unclean exits are detected via CrashReporter::beginSession below.) // Logging setup #ifdef Q_OS_APPLE @@ -404,6 +388,16 @@ int main(int argc, char *argv[]) } } + // Unclean-exit detection (cross-platform QLockFile session tracking, replacing the + // legacy Windows-only .dmp scan): a leftover session lock means the previous run died + // without reaching endSession(). Our own lock is held until the event loop finishes. + // Deliberately placed after parser.process() (--help/--version exit inside it and must + // not leak a lock) and after the instance hand-off (a handed-off secondary exits + // immediately and must never take a lock). Note this counts any death without + // endSession() as a crash, including SIGTERM at logout, Task Manager kills and + // session-end shutdowns; in AlwaysSend those send a report too. + const bool uncleanExit = crashReporter.beginSession(crashDatabasePath); + rng = new RNG_SFMT(CryptoUtil::randomUInt64()); themeManager = new ThemeManager; soundEngine = new SoundEngine; @@ -497,63 +491,11 @@ int main(int argc, char *argv[]) ui.show(); qCInfo(MainLog) << "ui.show() finished"; - // Crash-report flow: with a configured DSN the SDK owns capture and upload - // (consent-gated); with an empty DSN the dialog falls back to a local mock - // file and behaves exactly as before. Triggered by --simulate-crash or by - // leftover Windows crash dumps, which are consumed after handling so one - // crash never prompts twice. - { - QString crashDumpDir; -#ifdef Q_OS_WIN - // Mirrors the writer (CockatriceUnhandledExceptionFilter): it appends CrashDumps/Cockatrice - // to %LOCALAPPDATA%, or, when its known-folder lookup fails, to the temp directory walked up - // two parent_path() steps. %LOCALAPPDATA% is this scan's stand-in for FOLDERID_LocalAppData: - // both come from the same profile, so when the lookup fails the variable is normally unset. - QString crashDumpRootPath = QString::fromLocal8Bit(qgetenv("LOCALAPPDATA")); - if (crashDumpRootPath.isEmpty()) { - // The writer's fallback expression, verbatim — QDir::cdUp() is existence-checked and - // would silently leave the path alone here, parent_path() is not. - const std::filesystem::path fallback = std::filesystem::temp_directory_path().parent_path().parent_path(); - crashDumpRootPath = QString::fromStdWString(fallback.wstring()); - } - crashDumpDir = QDir(crashDumpRootPath).absoluteFilePath(QStringLiteral("CrashDumps/Cockatrice")); -#else - crashDumpDir = QDir::tempPath(); -#endif - const QStringList crashDumps = - QDir(crashDumpDir).entryList({QStringLiteral("cockatrice.crash.*.dmp")}, QDir::Files); - if (parser.isSet("simulate-crash") || !crashDumps.isEmpty()) { - const PersonalSettings::CrashReportingMode crashMode = - SettingsCache::instance().personal().getCrashReportingMode(); - const bool crashReady = crashReporter.isReady(); - if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { - if (crashReady) { - // Silent path: the redacted log was folded into the pending native crash - // envelope(s) at startup and consent already released them. Only when no - // pending crash event existed (e.g. --simulate-crash) does a standalone - // log event still need to go out here. - if (crashLogAttachedEnvelopes == 0) { - const QString crashPreview = DlgCrashReport::buildRedactedPreview(); - if (!crashReporter.sendReport(crashPreview)) { - const QString savedPath = DlgCrashReport::writeMockReport(crashPreview); - qCWarning(MainLog) << "Failed to send crash report, saved locally to" << savedPath; - } - } - } else { - DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); - } - } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { - // Real mode: Send gives consent + captures the event, Don't - // send revokes consent; the dialog owns both via the reporter. - DlgCrashReport crashDialog(&ui, crashReady, crashReady ? &crashReporter : nullptr); - crashDialog.exec(); - } - // NeverSend: skip silently (no dialog, nothing written or sent). - // Handled dumps are consumed so one crash never prompts twice. - for (const QString &dump : crashDumps) { - QFile::remove(crashDumpDir + QStringLiteral("/") + dump); - } - } + // Crash-report flow: the trigger is --simulate-crash or an unclean previous exit + // (leftover session lock; the old %LOCALAPPDATA%/CrashDumps .dmp scan is gone with + // the exception filter). See runCrashReportFlow() for the per-mode handling. + if (parser.isSet("simulate-crash") || uncleanExit) { + runCrashReportFlow(crashReporter, &ui, crashLogAttachedEnvelopes); } // force shortcuts to be shown/hidden in right-click menus, regardless of system defaults @@ -580,8 +522,6 @@ int main(int argc, char *argv[]) int ret = app.exec(); - crashReporter.shutdown(); - qCInfo(MainLog) << "Event loop finished, terminating..."; delete rng; PingPixmapGenerator::clear(); @@ -589,5 +529,10 @@ int main(int argc, char *argv[]) UserLevelPixmapGenerator::clear(); ManaSymbolPixmapGenerator::clear(); + // Released last: crashes anywhere in the teardown above (a common place for Qt apps to + // crash) are still captured by the SDK and detected as unclean exits on the next launch. + crashReporter.shutdown(); + crashReporter.endSession(); + return ret; } diff --git a/tests/crash_reporter_test.cpp b/tests/crash_reporter_test.cpp index e15007f7a..e44a34a60 100644 --- a/tests/crash_reporter_test.cpp +++ b/tests/crash_reporter_test.cpp @@ -1,10 +1,14 @@ #include "client/crash/crash_reporter.h" #include +#include #include #include +#include #include +#include #include +#include #include #include #include @@ -339,6 +343,81 @@ TEST(CrashReporterTest, AttachSkipsNoteWhenEmptyAndFindsRunDirEnvelopes) ASSERT_FALSE(content.contains(QByteArrayLiteral("user-note.txt"))); } +TEST(CrashReporterTest, BeginSessionOnEmptyDirReportsClean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionIgnoresLiveConcurrentInstance) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // A second QLockFile held in-process stands in for a live concurrent instance: same + // pid, alive, so the scan must leave it alone and report a clean start. + QLockFile liveInstance(dir.filePath(QStringLiteral("session-424242"))); + ASSERT_TRUE(liveInstance.tryLock(0)); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + ASSERT_TRUE(QFile::exists(liveInstance.fileName())); + reporter.endSession(); + liveInstance.unlock(); +} + +TEST(CrashReporterTest, BeginSessionDetectsStaleDeadOwnerLock) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // A well-formed lock whose owner is dead: our own pid (so the host matches) but a + // process name that cannot match, which is how QLockFile spots pid reuse. Must be + // reported unclean and cleaned up, even though the file is brand new (this is why + // the scan probes with staleLockTime 0 rather than the 30 s default). + const QString stalePath = dir.filePath(QStringLiteral("session-stale-owner")); + QFile stale(stalePath); + ASSERT_TRUE(stale.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&stale); + out << QCoreApplication::applicationPid() << '\n' + << QStringLiteral("definitely-not-the-test-binary") << '\n' + << QSysInfo::machineHostName() << '\n'; + stale.close(); + + CrashReporter reporter; + ASSERT_TRUE(reporter.beginSession(dir.path())); + ASSERT_FALSE(QFile::exists(stalePath)); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionCleansGarbageFileAndReportsUnclean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // An unreadable lock file has no live owner by definition: report unclean and remove it. + const QString garbagePath = dir.filePath(QStringLiteral("session-garbage")); + QFile garbage(garbagePath); + ASSERT_TRUE(garbage.open(QIODevice::WriteOnly)); + garbage.write("not-a-lock-file\x00\xff", 17); + garbage.close(); + + CrashReporter reporter; + ASSERT_TRUE(reporter.beginSession(dir.path())); + ASSERT_FALSE(QFile::exists(garbagePath)); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionEndSessionRoundTripReportsClean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); +} + } // namespace int main(int argc, char **argv)