Skip to content

Commit d78dfed

Browse files
ZephkekDutchman101
authored andcommitted
Crash handler polish (#4867)
#### Summary Architectural cleanup and hardening of the Crash Handler. * Unifies DbgHelp symbol lifecycle by replacing fragmented `SymInitialize`/`SymCleanup` calls with a shared `CrashHandler::InitializeSymbolHandler()`. * Deduplicates WER (Windows Error Reporting) registry setup by moving canonical ownership entirely to the loader. * Removes redundant classification and double-logging in `CCrashDumpWriter::HandleExceptionGlobal`. * Replaces a dangerous `TerminateThread` call in the watchdog with clean abandonment to prevent CRT lock deadlocks during shutdown. #### Motivation This is a follow-up architectural polish to the recent crash handler fixes. It resolves technical debt around overlapping symbol management across `CrashHandler`, `CCrashDumpWriter`, and `CExceptionInformation_Impl`, centralizes WER setup, and hardens the shutdown sequence against deadlocks. #### Test plan 1. Compiled and ran successfully. 2. Verified that MTA launches without regression. 3. **To verify**: Trigger a crash and confirm that `mta/dumps/private` still correctly populates with dumps, and that `core.log` still successfully resolves symbols in the stack trace. #### Checklist * [x] Your code should follow the [coding guidelines](https://wiki.multitheftauto.com/index.php?title=Coding_guidelines). * [x] Smaller pull requests are easier to review. If your pull request is beefy, your pull request should be reviewable commit-by-commit.
1 parent 943266c commit d78dfed

4 files changed

Lines changed: 82 additions & 218 deletions

File tree

Client/core/CCrashDumpWriter.cpp

Lines changed: 5 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -282,62 +282,8 @@ static bool InvokeClientHandleExceptionSafe(CClientBase* pClient, CExceptionInfo
282282

283283
namespace
284284
{
285-
void ConfigureDbgHelpOptions()
286-
{
287-
static std::atomic_flag configured = ATOMIC_FLAG_INIT;
288-
if (!configured.test_and_set(std::memory_order_acq_rel))
289-
{
290-
SymSetOptions(SYMOPT_LOAD_LINES | SYMOPT_UNDNAME | SYMOPT_FAIL_CRITICAL_ERRORS | SYMOPT_DEFERRED_LOADS);
291-
}
292-
}
293-
294-
std::mutex& GetSymInitMutex()
295-
{
296-
static std::mutex symMutex;
297-
return symMutex;
298-
}
299285
} // namespace
300286

301-
class SymbolHandlerGuard
302-
{
303-
public:
304-
explicit SymbolHandlerGuard(HANDLE process, bool enableSymbols) : m_process(process), m_initialized(false)
305-
{
306-
if (!enableSymbols)
307-
return;
308-
309-
if (m_process != nullptr)
310-
{
311-
std::lock_guard<std::mutex> lock{GetSymInitMutex()};
312-
313-
ConfigureDbgHelpOptions();
314-
315-
const SString& processDir = SharedUtil::GetMTAProcessBaseDir();
316-
const char* searchPath = processDir.empty() ? nullptr : processDir.c_str();
317-
318-
if (SymInitialize(m_process, searchPath, TRUE) != FALSE)
319-
m_initialized = true;
320-
}
321-
}
322-
323-
~SymbolHandlerGuard()
324-
{
325-
if (m_initialized)
326-
SymCleanup(m_process);
327-
}
328-
329-
SymbolHandlerGuard(const SymbolHandlerGuard&) = delete;
330-
SymbolHandlerGuard& operator=(const SymbolHandlerGuard&) = delete;
331-
SymbolHandlerGuard(SymbolHandlerGuard&&) = delete;
332-
SymbolHandlerGuard& operator=(SymbolHandlerGuard&&) = delete;
333-
334-
bool IsInitialized() const { return m_initialized; }
335-
336-
private:
337-
HANDLE m_process;
338-
bool m_initialized;
339-
};
340-
341287
[[nodiscard]] static bool NormalizePathForValidation(const SString& inputPath, SString& outputPath)
342288
{
343289
if (inputPath.empty())
@@ -767,9 +713,11 @@ static void AppendCrashDiagnostics(const SString& text)
767713
frame.AddrFrame.Mode = AddrModeFlat;
768714
frame.AddrStack.Mode = AddrModeFlat;
769715

770-
SymbolHandlerGuard symbolGuard(hProcess, hasSymbols);
771-
772-
const bool useDbgHelp = symbolGuard.IsInitialized();
716+
bool useDbgHelp = false;
717+
if (hasSymbols)
718+
{
719+
useDbgHelp = CrashHandler::InitializeSymbolHandler();
720+
}
773721
const auto routines = useDbgHelp ? StackTraceHelpers::MakeStackWalkRoutines(true) : StackTraceHelpers::MakeStackWalkRoutines(false);
774722

775723
static_assert(MAX_SYM_NAME > 1, "MAX_SYM_NAME must include room for a terminator");
@@ -1475,20 +1423,6 @@ long WINAPI CCrashDumpWriter::HandleExceptionGlobal(_EXCEPTION_POINTERS* pExcept
14751423
return EXCEPTION_EXECUTE_HANDLER;
14761424
}
14771425

1478-
if (IsFatalException(exceptionCode) == FALSE)
1479-
{
1480-
std::array<char, DEBUG_BUFFER_SIZE> szDebug;
1481-
SAFE_DEBUG_PRINT_C(szDebug.data(), szDebug.size(), "CCrashDumpWriter: Non-fatal exception 0x%08X - ignoring\n", exceptionCode);
1482-
ms_bInCrashHandler.store(false, std::memory_order_release);
1483-
return EXCEPTION_CONTINUE_SEARCH;
1484-
}
1485-
1486-
const BOOL exceptionLogged = LogExceptionDetails(pException);
1487-
if (exceptionLogged == FALSE)
1488-
{
1489-
SAFE_DEBUG_OUTPUT("CCrashDumpWriter: WARNING - LogExceptionDetails failed\n");
1490-
}
1491-
14921426
SAFE_DEBUG_OUTPUT("CCrashDumpWriter: ======================================\n");
14931427
SAFE_DEBUG_OUTPUT("CCrashDumpWriter: FATAL EXCEPTION - Begin crash processing\n");
14941428
SAFE_DEBUG_OUTPUT("CCrashDumpWriter: ======================================\n");

Client/core/CExceptionInformation_Impl.cpp

Lines changed: 5 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -129,59 +129,6 @@ struct StackFrameInfo
129129
};
130130
constexpr std::size_t MAX_FILE_NAME = 260;
131131

132-
class SymbolHandlerGuard
133-
{
134-
public:
135-
explicit SymbolHandlerGuard(HANDLE process, bool enableSymbols) : m_process(process), m_initialized(false), m_uncaughtExceptions(std::uncaught_exceptions())
136-
{
137-
if (!enableSymbols)
138-
{
139-
return;
140-
}
141-
142-
if (m_process != nullptr)
143-
{
144-
SymSetOptions(SYMOPT_LOAD_LINES | SYMOPT_UNDNAME | SYMOPT_FAIL_CRITICAL_ERRORS);
145-
146-
if (SymInitialize(m_process, nullptr, TRUE) != FALSE)
147-
{
148-
m_initialized = true;
149-
}
150-
else
151-
{
152-
const DWORD dwError = GetLastError();
153-
char debugBuffer[DEBUG_BUFFER_SIZE] = {};
154-
SAFE_DEBUG_PRINT_C(debugBuffer, DEBUG_BUFFER_SIZE, "%.*sSymbolHandlerGuard: SymInitialize failed, error: %u\n",
155-
static_cast<int>(DEBUG_PREFIX_EXCEPTION_INFO.size()), DEBUG_PREFIX_EXCEPTION_INFO.data(), dwError);
156-
}
157-
}
158-
}
159-
160-
~SymbolHandlerGuard()
161-
{
162-
if (m_initialized)
163-
{
164-
if (std::uncaught_exceptions() <= m_uncaughtExceptions)
165-
{
166-
SymCleanup(m_process);
167-
}
168-
m_initialized = false;
169-
}
170-
}
171-
172-
SymbolHandlerGuard(const SymbolHandlerGuard&) = delete;
173-
SymbolHandlerGuard& operator=(const SymbolHandlerGuard&) = delete;
174-
SymbolHandlerGuard(SymbolHandlerGuard&&) = delete;
175-
SymbolHandlerGuard& operator=(SymbolHandlerGuard&&) = delete;
176-
177-
bool IsInitialized() const { return m_initialized; }
178-
179-
private:
180-
HANDLE m_process;
181-
bool m_initialized;
182-
int m_uncaughtExceptions;
183-
};
184-
185132
[[nodiscard]] static std::optional<std::vector<std::string>> CaptureEnhancedStackTrace(_EXCEPTION_POINTERS* pException)
186133
{
187134
if (pException == nullptr || pException->ContextRecord == nullptr)
@@ -214,9 +161,11 @@ class SymbolHandlerGuard
214161
frame.AddrFrame.Mode = AddrModeFlat;
215162
frame.AddrStack.Mode = AddrModeFlat;
216163

217-
SymbolHandlerGuard symbolGuard(hProcess, hasSymbols);
218-
219-
const bool useDbgHelp = symbolGuard.IsInitialized();
164+
bool useDbgHelp = false;
165+
if (hasSymbols)
166+
{
167+
useDbgHelp = CrashHandler::InitializeSymbolHandler();
168+
}
220169

221170
const auto routines = useDbgHelp ? StackTraceHelpers::MakeStackWalkRoutines(true) : StackTraceHelpers::MakeStackWalkRoutines(false);
222171
alignas(SYMBOL_INFO) std::uint8_t symbolBuffer[sizeof(SYMBOL_INFO) + MAX_SYMBOL_NAME];

0 commit comments

Comments
 (0)