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..a093faaa8 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,47 @@ 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 + 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 }} + # Same release string the client reports its events under (COCKATRICE_SENTRY_RELEASE + # = PROJECT_VERSION, see cmake/CrashReporting.cmake), so stack traces from this build + # de-symbolicate against these symbols. + SENTRY_RELEASE: ${{ needs.configure.outputs.sentry_release }} + run: | + paths=() + for dir in build/cockatrice build/oracle build/servatrice; do + [[ -d $dir ]] && paths+=("$dir") + done + if [[ ${#paths[@]} == 0 ]]; then + echo "No build output dirs found, skipping symbol upload" + exit 0 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop --release "$SENTRY_RELEASE" "${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 +587,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 +619,58 @@ 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' + 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 }} + # Same release string the client reports its events under (COCKATRICE_SENTRY_RELEASE + # = PROJECT_VERSION, see cmake/CrashReporting.cmake), so stack traces from this build + # de-symbolicate against these symbols. + SENTRY_RELEASE: ${{ needs.configure.outputs.sentry_release }} + run: | + paths=() + if [[ '${{ matrix.os }}' == 'Windows' ]]; then + # Top-level CMakeLists.txt links /DEBUG even in release, so PDBs land + # next to the binaries (same dirs as the PDB artifact step below). + for dir in build/cockatrice/Release build/oracle/Release build/servatrice/Release; do + [[ -d $dir ]] && paths+=("$dir") + done + 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") + else + paths+=("$binary") + fi + fi + done + fi + if [[ ${#paths[@]} == 0 ]]; then + echo "No symbol paths found, skipping symbol upload" + exit 0 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop --release "$SENTRY_RELEASE" "${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. diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake index abd029cc2..2c8888800 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 diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 811a3cb7f..a973d0364 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -1530,11 +1530,18 @@ ${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. + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayIcon" "$INSTDIR\cockatrice.exe" WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayName" "Cockatrice" @@ -1635,6 +1642,12 @@ 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. + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" + ; 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/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp index 720bcd350..2a6c1a750 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,13 @@ 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. sentry_options_set_require_user_consent(options, 1); + sentry_options_set_cache_keep(options, SENTRY_CACHE_KEEP_OFFLINE); sentry_options_set_before_send(options, scrubEventBeforeSend, nullptr); ready = (sentry_init(options) == 0); #else @@ -291,6 +299,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 +388,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..3af33f88c 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,6 +25,8 @@ #define COCKATRICE_SENTRY_RELEASE "" #endif +class QLockFile; + Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); /** @@ -34,21 +37,28 @@ Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); * 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() + * 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. 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. + * Session tracking (beginSession/endSession) is 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 +150,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..45e5dcc90 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(); @@ -204,9 +147,9 @@ static QString redactActivationUrl(const QString &url) 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 +347,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; @@ -499,30 +452,11 @@ int main(int argc, char *argv[]) // 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. + // file and behaves exactly as before. 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. { - 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()) { + if (parser.isSet("simulate-crash") || uncleanExit) { const PersonalSettings::CrashReportingMode crashMode = SettingsCache::instance().personal().getCrashReportingMode(); const bool crashReady = crashReporter.isReady(); @@ -549,10 +483,6 @@ int main(int argc, char *argv[]) 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); - } } } @@ -580,8 +510,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 +517,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)