fix(android): contain panics at the JNI boundary instead of aborting the process
Ten `extern "system"` callbacks are entered by the JVM on arbitrary threads. A panic unwinding out of one crosses the FFI boundary, which Rust answers by aborting: the app vanishes with no Java exception, no attributable stack trace, and no crash report the user can send. For callbacks that fire four times a second during playback that is the worst available failure mode. It was reachable. `nativeOnPositionUpdate` built a fallback Tokio runtime with `Runtime::new().unwrap()` on threads that have none, and `Runtime::new()` fails under exactly the fd exhaustion and thread-spawn refusal Android subjects a media app to. That now logs and drops the report — losing one progress report is recoverable, losing the app is not. Every callback body is wrapped in `jni_guard`, which catches the unwind and logs it. It is a backstop, not a licence to panic: a contained panic still leaves whatever it interrupted half-done. The guard lives in `player::jni_guard` rather than `player::android` because that module is `cfg(target_os = "android")` and so never compiles on the host — which is why its 1575 lines had no tests at all. A tripwire test asserts every entry point wraps its body, so an eleventh callback cannot reintroduce the defect; it reads the source, since exercising the real boundary needs a JVM. Verified with `cargo check --target aarch64-linux-android`.
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
//! Panic containment for the Android JNI boundary.
|
||||
//!
|
||||
//! Compiled on every platform, unlike `player::android` itself, so the guard and
|
||||
//! the tripwire that enforces its use are unit-tested on the host — the same
|
||||
//! reason `RESUME_BACKOFF_STEP_SECS` lives outside the `cfg(android)` block.
|
||||
|
||||
/// Run the body of a JNI callback with any panic contained.
|
||||
///
|
||||
/// Every `extern "system"` function in this file is called by the JVM on an
|
||||
/// arbitrary thread. A panic that unwinds out of one crosses the FFI boundary,
|
||||
/// which Rust answers by **aborting the process** — the app vanishes with no
|
||||
/// Java exception, no stack trace attributable to it, and no crash report the
|
||||
/// user can send. That is the worst possible failure mode for the callbacks
|
||||
/// that fire four times a second during playback.
|
||||
///
|
||||
/// The panics are real, not theoretical: this file builds a fallback Tokio
|
||||
/// runtime on threads that have none, and `Runtime::new()` fails under the fd
|
||||
/// exhaustion and thread-spawn refusal an Android device puts a media app
|
||||
/// through. Losing one position report is recoverable; losing the process is
|
||||
/// not.
|
||||
///
|
||||
/// A contained panic still leaves whatever it interrupted half-done, so this is
|
||||
/// a backstop, not a licence to panic. `utils::lock` already keeps a poisoned
|
||||
/// mutex from cascading; this keeps the FFI boundary from turning any remaining
|
||||
/// panic into a process kill.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052
|
||||
///
|
||||
/// Only *called* from `player::android`, which is `cfg(target_os = "android")`,
|
||||
/// so it is dead code on every other target — the same reason
|
||||
/// `RESUME_BACKOFF_STEP_SECS` carries this attribute. It is still compiled and
|
||||
/// tested here on purpose.
|
||||
#[cfg_attr(not(target_os = "android"), allow(dead_code))]
|
||||
pub(crate) fn jni_guard<F: FnOnce()>(name: &str, body: F) {
|
||||
// AssertUnwindSafe: the shared state behind these callbacks is already
|
||||
// reached through poison-tolerant locks, so a panic cannot hand out a
|
||||
// guard observing a torn value.
|
||||
if std::panic::catch_unwind(std::panic::AssertUnwindSafe(body)).is_err() {
|
||||
// The panic hook has already logged the payload and location.
|
||||
log::error!("[JNI] Panic in {name} was contained; the callback was dropped");
|
||||
}
|
||||
}
|
||||
|
||||
// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[cfg(test)]
|
||||
mod jni_guard_tests {
|
||||
use super::*;
|
||||
|
||||
/// The guard must swallow a panic rather than let it reach the JVM.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn a_panicking_callback_body_does_not_escape_the_guard() {
|
||||
let hook = std::panic::take_hook();
|
||||
std::panic::set_hook(Box::new(|_| {}));
|
||||
jni_guard("test_callback", || panic!("ExoPlayer callback blew up"));
|
||||
std::panic::set_hook(hook);
|
||||
// Reaching here at all is the assertion: without the guard the panic
|
||||
// would unwind out of the `extern "system"` fn and abort the process.
|
||||
}
|
||||
|
||||
/// The guard must not disturb a callback that behaves.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn a_normal_callback_body_still_runs() {
|
||||
let mut ran = false;
|
||||
jni_guard("test_callback", || ran = true);
|
||||
assert!(ran);
|
||||
}
|
||||
|
||||
/// **Tripwire.** Every JNI entry point must wrap its body in `jni_guard`.
|
||||
///
|
||||
/// A panic crossing the `extern "system"` boundary aborts the process, so a
|
||||
/// twelfth callback added without the guard reintroduces the whole defect.
|
||||
/// Checked against the source because the real boundary needs a JVM to
|
||||
/// exercise — the same tripwire idiom as `check:boundary`.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn every_jni_entry_point_wraps_its_body_in_the_guard() {
|
||||
let src = include_str!("android/mod.rs");
|
||||
let mut unguarded = Vec::new();
|
||||
|
||||
let mut lines = src.lines().enumerate().peekable();
|
||||
while let Some((_, line)) = lines.next() {
|
||||
if !line.starts_with("pub extern \"system\" fn ") {
|
||||
continue;
|
||||
}
|
||||
let name = line
|
||||
.trim_start_matches("pub extern \"system\" fn ")
|
||||
.trim_end_matches('(')
|
||||
.to_string();
|
||||
|
||||
// Walk to the end of the parameter list, then look at the first
|
||||
// statement of the body.
|
||||
let mut body_start = None;
|
||||
for (n, l) in lines.by_ref() {
|
||||
if l.trim_end().ends_with(") {") || l.trim() == ") {" {
|
||||
body_start = Some(n);
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(body_start.is_some(), "could not find the body of {name}");
|
||||
|
||||
match lines.peek() {
|
||||
Some((_, first)) if first.trim_start().starts_with("jni_guard(") => {}
|
||||
other => unguarded.push(format!(
|
||||
"{name} (body starts with {:?})",
|
||||
other.map(|(_, l)| l.trim()).unwrap_or("<eof>")
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
assert!(
|
||||
unguarded.is_empty(),
|
||||
"JNI entry points whose body is not wrapped in jni_guard — a panic in \
|
||||
one of these aborts the process:\n {}",
|
||||
unguarded.join("\n ")
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user