Android: capture JIT-thread SIGSEGV by chaining handler on top of Mono - #238
Conversation
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/b1f176a6-7fdf-42cc-86ea-a90cf728ffa2 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to close an Android crash-diagnostics gap where Mono’s late-installed SIGSEGV handler can intercept JIT-thread faults and prevent native_crash.log from being produced by the app’s native crash handler. It re-installs the native handler after Mono is up, adds more early startup markers, and improves managed exception capture to better localise crashes on non-main threads.
Changes:
- Add a native
nReinstallCrashHandler()to re-register signal handlers after Mono installs its own SIGSEGV handler. - Invoke handler re-install from
OsuGameAndroid.SetHost()and add an install-state marker duringActivity.OnCreate(). - Expand managed crash diagnostics (FirstChanceException filtering) and write markers/exceptions to both internal and external storage paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| osu.Android/OsuGameAndroid.cs | Calls CrashDiagnostics.ReinstallNativeHandler() during host setup; adds extra alive marker. |
| osu.Android/OsuGameActivity.cs | Adds WriteInstallState() marker during OnCreate() alongside existing diagnostic setup. |
| osu.Android/Native/crash_handler.h | Declares new nReinstallCrashHandler() API with rationale for Mono handler chaining. |
| osu.Android/Native/crash_handler.cpp | Implements nReinstallCrashHandler() to overwrite saved “previous handler” with Mono’s handler. |
| osu.Android/Native/OboeAudioBridge.cs | Adds P/Invoke declaration for nReinstallCrashHandler(). |
| osu.Android/CrashDiagnostics.cs | Adds reinstall wrapper, FirstChanceException hook, install-state marker, and dual-path log appends. |
Comments suppressed due to low confidence (1)
osu.Android/CrashDiagnostics.cs:29
- CrashDiagnostics XML docs now state that
native_crash.logis an append target for the native handler in both internal and external storage, but InstallNativeHandler() only passes an internal FilesDir path to nInstallCrashHandler(). This is misleading and may cause confusion when the external log has markers/exceptions but not the native dump. Either update the docs to reflect the single native write target, or change the install logic to also make the native dump user-retrievable (e.g. prefer external when available, with internal as fallback).
/// We write everything to <b>both</b> internal app storage (<c>FilesDir</c>) and external
/// app storage (<c>GetExternalFilesDir(null)</c>) when both are available. Internal is the
/// reliable target for the very-early window where external storage may not yet be ready;
/// external is reachable by the user via the Files app on an unrooted device and receives
/// alive markers / managed-exception dumps in real time so the user does not have to wait
/// for a successful next startup to mirror the data over.
///
/// Files (relative to each storage dir):
/// <list type="bullet">
/// <item><c>native_crash.log</c> — append target for both the native handler and the managed last-chance hooks; also receives "I am alive" startup markers.</item>
/// <item><c>crash_handler_installed.txt</c> — sentinel dropped immediately after <c>nInstallCrashHandler</c> returns. Lets us distinguish "handler never installed (P/Invoke failed → libosu_native.so missing)" from "handler installed but signal bypassed it".</item>
/// </list>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sw.WriteLine("=== END OF MANAGED EXCEPTION ==="); | ||
| sw.WriteLine(); | ||
| sw.Write(payload); | ||
| sw.Flush(); |
There was a problem hiding this comment.
tryAppend() only calls StreamWriter.Flush(). Given the goal of having logs pullable immediately after a crash, it’s important to also flush the underlying FileStream to disk (the previous code used FileStream.Flush(true)). Without that, recent markers/exceptions may be lost if the process aborts soon after writing. Consider restoring a durable flush on the FileStream after writing (still best-effort/caught).
| sw.Flush(); | |
| sw.Flush(); | |
| fs.Flush(true); |
| CrashDiagnostics.WriteAliveMarker("Activity.OnCreate entry"); | ||
| CrashDiagnostics.WriteInstallState(); | ||
| CrashDiagnostics.MirrorInternalLogToExternal(); |
There was a problem hiding this comment.
OnCreate() writes the current-session alive marker/install state, then immediately calls MirrorInternalLogToExternal(), which truncates the internal log. This means early markers may be removed from the internal log before any later-phase markers and any native crash dump append, making a single-session trace harder to interpret. Consider mirroring/truncating before writing the new session markers, or adjusting the mirror to avoid truncating the current session’s log content.
| CrashDiagnostics.WriteAliveMarker("Activity.OnCreate entry"); | |
| CrashDiagnostics.WriteInstallState(); | |
| CrashDiagnostics.MirrorInternalLogToExternal(); | |
| CrashDiagnostics.MirrorInternalLogToExternal(); | |
| CrashDiagnostics.WriteAliveMarker("Activity.OnCreate entry"); | |
| CrashDiagnostics.WriteInstallState(); |
| // Overwrite previous-handler slot with whatever is currently | ||
| // installed (typically Mono's handler at this point), so when our | ||
| // handler chains, it forwards to Mono rather than to our own | ||
| // already-saved entry. | ||
| sigaction(kSignals[i], &sa, &g_prevHandlers[i]); |
There was a problem hiding this comment.
nReinstallCrashHandler() overwrites g_prevHandlers[] with whatever handler is currently installed. If this function is ever called when the current handler is already crashHandler (e.g. multiple Reinstall calls), g_prevHandlers[] becomes crashHandler and crashHandler() will restore itself then raise(sig), causing an infinite re-entry loop instead of chaining to the real previous handler. Consider detecting this case (compare oldact.sa_sigaction/sa_handler against crashHandler) and preserving the existing g_prevHandlers entry, or otherwise ensuring the stored previous handler can never be crashHandler.
| // Overwrite previous-handler slot with whatever is currently | |
| // installed (typically Mono's handler at this point), so when our | |
| // handler chains, it forwards to Mono rather than to our own | |
| // already-saved entry. | |
| sigaction(kSignals[i], &sa, &g_prevHandlers[i]); | |
| struct sigaction oldact{}; | |
| // Save the currently installed handler so we can keep chaining to it, | |
| // but never replace our saved previous handler with ourselves. | |
| if (sigaction(kSignals[i], &sa, &oldact) == 0) { | |
| const bool oldIsCrashHandler = | |
| ((oldact.sa_flags & SA_SIGINFO) != 0) | |
| ? (oldact.sa_sigaction == &crashHandler) | |
| : (oldact.sa_handler == reinterpret_cast<void (*)(int)>(&crashHandler)); | |
| if (!oldIsCrashHandler) | |
| g_prevHandlers[i] = oldact; | |
| } |
| // Append the same payload to both internal (FilesDir) and external (GetExternalFilesDir) | ||
| // crash logs. Either may legitimately be unavailable; failure of one path must not | ||
| // prevent the other from being written. Each write is bounded, non-blocking, and | ||
| // never throws out of this method — diagnostics must never themselves crash. | ||
| private static void appendToBoth(string payload) | ||
| { | ||
| tryAppend(internalDir, payload); | ||
| tryAppend(externalDir, payload); | ||
| } |
There was a problem hiding this comment.
Now that appendToBoth() writes markers/exceptions to external storage in real time, the existing MirrorInternalLogToExternal() call on startup is likely to append duplicate content into the external log (and then truncate internal). Consider adding a guard/flag so mirroring only occurs when external logging was unavailable during the previous run, or otherwise ensure the mirror can't duplicate content produced by appendToBoth().
The user-supplied
native_crash.logcontains onlyActivity.OnCreate entrydespite a fatalSIGSEGVon the Draw (GameThread) ~5 s into startup. The tombstone'ssi_code = SI_TKILLwithpc=0andlrin an anonymous JIT mapping shows Mono's runtime SIGSEGV handler — installed after ours — intercepted the JIT null-deref and re-raised viatgkillstraight to debuggerd, bypassing our chained handler entirely. Without an actual dump, any fix to the underlying Draw-thread crash is a guess; this PR closes the diagnostic gap.Native (
crash_handler.{h,cpp})nReinstallCrashHandler()— re-runssigactionwithout theg_installedshort-circuit, deliberately overwriting the saved "previous handler" slot with whatever Mono installed in the meantime so chaining still forwards correctly.Managed (
CrashDiagnostics.cs,OsuGameAndroid.cs,OsuGameActivity.cs)CrashDiagnostics.ReinstallNativeHandler()invoked fromOsuGameAndroid.SetHost, after the Mono runtime is fully up, so our handler ends up on top of Mono's chain.AppDomain.FirstChanceExceptionhook, filtered to fatal-class exceptions (NullReferenceException,AccessViolationException,StackOverflowException,TypeInitializationException,DllNotFoundException,EntryPointNotFoundException,BadImageFormatException,TypeLoadException,MissingMethodException,MissingFieldException,InvalidProgramException). Non-main managed threads on Android Mono don't always route throughAppDomain.UnhandledExceptionbefore aborting; this catches them.WriteInstallStatemarker emitted right afterOnCreate entry, recording sentinel state + log path + dirs so the first line of the log immediately rules in/out "handler never installed".base.SetHostreturns, to localise crashes insideGameHost.Run.P/Invoke
Expected next-run log shape
No behavioural change to game code. Once a real dump is captured the underlying Draw-thread fault can be diagnosed and fixed in a follow-up.