diff --git a/CHANGELOG.md b/CHANGELOG.md index 87dd969..70c0e03 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Unreleased + +- Clear pending Java exceptions on failed USB JNI operations before detaching + native threads, preventing an Android crash when reconnecting an unplugged dongle. + ## [3.0.7](https://github.com/s00d/tauri-plugin-serialplugin/compare/v3.0.6...v3.0.7) (2026-09-11) diff --git a/crates/android-usb-serial/CHANGELOG.md b/crates/android-usb-serial/CHANGELOG.md index 0f0637f..0b829e9 100644 --- a/crates/android-usb-serial/CHANGELOG.md +++ b/crates/android-usb-serial/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +- Return `Disconnected` for I/O and control operations on closed serial handles, + preventing stale adapter clones from panicking after USB detach. +- Add regression coverage for stale clones, closed-handle operations, and reopening. + ## 0.1.1 - Track CDC ACM interrupt-IN notifications and expose DCD, DSR, and ring state diff --git a/crates/android-usb-serial/Cargo.toml b/crates/android-usb-serial/Cargo.toml index 5fcd290..0590714 100644 --- a/crates/android-usb-serial/Cargo.toml +++ b/crates/android-usb-serial/Cargo.toml @@ -64,3 +64,8 @@ base64 = "0.23" name = "golden_record" path = "src/bin/golden_record.rs" required-features = ["fake-transport"] + +[[test]] +name = "disconnect_test" +path = "tests/disconnect_test.rs" +required-features = ["fake-transport", "serialport-compat"] diff --git a/crates/android-usb-serial/src/port.rs b/crates/android-usb-serial/src/port.rs index 175df8b..ed343a3 100644 --- a/crates/android-usb-serial/src/port.rs +++ b/crates/android-usb-serial/src/port.rs @@ -5,7 +5,7 @@ use crate::config::{FlowControl, LineConfig, PurgeKind}; use crate::drivers::{Driver, ModemStatus}; -use crate::error::Result; +use crate::error::{Result, UsbSerialError}; use crate::reader::SerialReader; use crate::transport::SharedTransport; @@ -27,39 +27,57 @@ impl SerialPortHandle { } } + // Adapter clones share this handle under a mutex. A detach can close it + // before a queued operation acquires that mutex; never enter a torn-down driver. + fn ensure_open(&self) -> Result<()> { + if self.closed { + Err(UsbSerialError::Disconnected) + } else { + Ok(()) + } + } + /// Bulk OUT write. Opens OUT only — IN belongs to the optional [`Self::start_reader`]. pub fn write(&mut self, data: &[u8]) -> Result { + self.ensure_open()?; self.driver.write(data) } /// Blocking/synchronous bulk IN read through the driver (not the background reader). pub fn read(&mut self, buf: &mut [u8]) -> Result { + self.ensure_open()?; self.driver.read(buf) } /// Baud / framing line coding. pub fn set_line_config(&mut self, cfg: LineConfig) -> Result<()> { + self.ensure_open()?; self.driver.set_line_config(cfg) } pub fn set_flow_control(&mut self, flow: FlowControl) -> Result<()> { + self.ensure_open()?; self.driver.set_flow_control(flow) } pub fn set_dtr(&mut self, value: bool) -> Result<()> { + self.ensure_open()?; self.driver.set_dtr(value) } pub fn set_rts(&mut self, value: bool) -> Result<()> { + self.ensure_open()?; self.driver.set_rts(value) } pub fn set_break(&mut self, enabled: bool) -> Result<()> { + self.ensure_open()?; self.driver.set_break(enabled) } /// Clear RX and/or TX driver buffers (host-side purge). pub fn purge(&mut self, kind: PurgeKind) -> Result<()> { + self.ensure_open()?; self.driver.purge(kind) } @@ -70,6 +88,7 @@ impl SerialPortHandle { /// Latched modem status lines (CTS/DSR/RI/CD), when the chip reports them. pub fn modem_status(&mut self) -> Result { + self.ensure_open()?; self.driver.modem_status() } @@ -87,6 +106,7 @@ impl SerialPortHandle { /// /// Call **after** [`Self::set_line_config`] and DTR/RTS on weak OTG / CH340 adapters. pub fn start_reader(&mut self) -> Result<()> { + self.ensure_open()?; if self.reader.is_some() { return Ok(()); } @@ -97,6 +117,7 @@ impl SerialPortHandle { /// Non-blocking read from the background reader if running; else [`Self::read`]. pub fn try_read(&mut self, buf: &mut [u8]) -> Result { + self.ensure_open()?; if let Some(reader) = &mut self.reader { return reader.try_read(buf); } diff --git a/crates/android-usb-serial/tests/disconnect_test.rs b/crates/android-usb-serial/tests/disconnect_test.rs new file mode 100644 index 0000000..cd51e65 --- /dev/null +++ b/crates/android-usb-serial/tests/disconnect_test.rs @@ -0,0 +1,68 @@ +use android_usb_serial::config::{FlowControl, LineConfig, PurgeKind}; +use android_usb_serial::error::UsbSerialError; +use android_usb_serial::fake::FakeTransport; +use android_usb_serial::open_port; +use android_usb_serial::serialport_compat::SerialPortAdapter; +use serialport::SerialPort; +use std::io::{Read, Write}; +use std::sync::Arc; + +#[test] +fn stale_clone_write_after_detach_returns_error() { + let handle = open_port(Arc::new(FakeTransport::cdc_iad()), 0).unwrap(); + let adapter = + SerialPortAdapter::new(handle, "usb#0", LineConfig::default(), FlowControl::None).unwrap(); + adapter.start_reader().unwrap(); + let mut pending_writer = adapter.clone(); + adapter.shutdown(); + assert!(pending_writer.write(b"1").is_err()); + assert!(pending_writer.read(&mut [0; 8]).is_err()); + assert!(pending_writer.write_data_terminal_ready(true).is_err()); + adapter.shutdown(); +} + +#[test] +fn closed_handle_rejects_io_and_controls() { + let mut handle = open_port(Arc::new(FakeTransport::cdc_iad()), 0).unwrap(); + handle.close(); + assert_eq!(handle.write(b"1"), Err(UsbSerialError::Disconnected)); + assert_eq!(handle.read(&mut [0; 8]), Err(UsbSerialError::Disconnected)); + assert_eq!( + handle.try_read(&mut [0; 8]), + Err(UsbSerialError::Disconnected) + ); + assert_eq!(handle.start_reader(), Err(UsbSerialError::Disconnected)); + assert_eq!( + handle.set_line_config(LineConfig::default()), + Err(UsbSerialError::Disconnected) + ); + assert_eq!( + handle.set_flow_control(FlowControl::None), + Err(UsbSerialError::Disconnected) + ); + assert_eq!(handle.set_dtr(true), Err(UsbSerialError::Disconnected)); + assert_eq!(handle.set_rts(true), Err(UsbSerialError::Disconnected)); + assert_eq!(handle.set_break(true), Err(UsbSerialError::Disconnected)); + assert_eq!( + handle.purge(PurgeKind::Both), + Err(UsbSerialError::Disconnected) + ); + assert_eq!( + handle.clear(PurgeKind::Both), + Err(UsbSerialError::Disconnected) + ); + assert!(matches!( + handle.modem_status(), + Err(UsbSerialError::Disconnected) + )); + handle.close(); +} + +#[test] +fn fresh_handle_can_write_after_previous_session_closes() { + let fake = FakeTransport::cdc_iad(); + let mut old = open_port(Arc::new(fake.clone()), 0).unwrap(); + old.close(); + let mut fresh = open_port(Arc::new(fake), 0).unwrap(); + assert_eq!(fresh.write(b"1").unwrap(), 1); +} diff --git a/src/android/fd_bridge.rs b/src/android/fd_bridge.rs index e99ea8b..7685fbd 100644 --- a/src/android/fd_bridge.rs +++ b/src/android/fd_bridge.rs @@ -1,34 +1,34 @@ //! JNI bridge: Kotlin UsbFdBridge → Rust fd API. -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use crate::error::Error; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use crate::jni_ready::jni_not_ready_message; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use jni::errors::Error as JniError; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use jni::objects::{GlobalRef, JObject, JString, JValue}; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use jni::{JNIEnv, JavaVM}; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] use std::sync::OnceLock; -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] static JVM: OnceLock = OnceLock::new(); -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] struct FdJniCache { class: GlobalRef, } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] static CACHE: OnceLock = OnceLock::new(); /// Last `new_global_ref` failure from [`init_class`], if any. -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] static CLASS_INIT_ERROR: OnceLock = OnceLock::new(); -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] fn not_ready() -> Error { Error::new(jni_not_ready_message( JVM.get().is_some(), @@ -37,14 +37,14 @@ fn not_ready() -> Error { )) } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] impl From for Error { fn from(err: JniError) -> Self { Error::new(err.to_string()) } } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] fn with_env(f: F) -> Result where F: FnOnce(&mut JNIEnv) -> Result, @@ -54,13 +54,16 @@ where .attach_current_thread() .map_err(|e| Error::new(format!("JNI attach failed: {e}")))?; env.with_local_frame(32, |env| { - let out = f(env)?; + // JNI errors can leave a Java exception pending. Clear/map it even when + // the operation failed, before the attached native thread is detached. + // Otherwise an expected USB unplug becomes an uncaught Java exception. + let out = f(env); map_exception(env, "USB fd operation failed")?; - Ok(out) + out }) } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] fn map_exception(env: &mut JNIEnv, fallback: &str) -> Result<(), Error> { if !env .exception_check() @@ -68,11 +71,10 @@ fn map_exception(env: &mut JNIEnv, fallback: &str) -> Result<(), Error> { { return Ok(()); } - let msg: String = env - .exception_occurred() - .ok() + let exception = env.exception_occurred().ok(); + env.exception_clear().map_err(Error::from)?; + let msg: String = exception .and_then(|exc| { - let _ = env.exception_clear(); let jmsg = env .call_method(&exc, "getMessage", "()Ljava/lang/String;", &[]) .ok() @@ -83,16 +85,19 @@ fn map_exception(env: &mut JNIEnv, fallback: &str) -> Result<(), Error> { }) }) .unwrap_or_else(|| fallback.into()); + // Exception message retrieval itself can throw. Never let a secondary + // exception escape this boundary either. + env.exception_clear().map_err(Error::from)?; Err(Error::new(msg)) } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] fn cache(_env: &mut JNIEnv) -> Result<&'static FdJniCache, Error> { CACHE.get().ok_or_else(not_ready) } /// Called from `UsbNative.nativeInit` on a Java thread, where the app class loader is in scope. -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] pub fn init_class(env: &mut JNIEnv, class: &JObject) { if CACHE.get().is_some() { return; @@ -109,12 +114,12 @@ pub fn init_class(env: &mut JNIEnv, class: &JObject) { } } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] pub fn init_java_vm(vm: JavaVM) { let _ = JVM.set(vm); } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] pub fn call_enumerate_json() -> Result { with_env(|env| { let cache = cache(env)?; @@ -128,7 +133,7 @@ pub fn call_enumerate_json() -> Result { }) } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] pub fn call_open_device_fd(device_name: &str) -> Result { with_env(|env| { let cache = cache(env)?; @@ -143,7 +148,7 @@ pub fn call_open_device_fd(device_name: &str) -> Result { }) } -#[cfg(target_os = "android")] +#[cfg(any(target_os = "android", test))] pub fn call_close_device_fd(device_name: &str) -> Result<(), Error> { with_env(|env| { let cache = cache(env)?; @@ -157,3 +162,47 @@ pub fn call_close_device_fd(device_name: &str) -> Result<(), Error> { Ok(()) }) } + +#[cfg(all(test, not(any(target_os = "android", target_os = "ios"))))] +mod tests { + use super::*; + + #[test] + fn failed_java_call_is_cleared_before_returning_to_caller() { + let args = jni::InitArgsBuilder::new().build().unwrap(); + init_java_vm(JavaVM::new(args).unwrap()); + // Keep the thread attached so we can inspect the exception state after + // with_env returns, before thread teardown could deliver it to Java. + let env = JVM.get().unwrap().attach_current_thread().unwrap(); + let result: Result<(), Error> = with_env(|env| { + let invalid = env.new_string("not-a-number")?; + env.call_static_method( + "java/lang/Integer", + "parseInt", + "(Ljava/lang/String;)I", + &[JValue::Object(&JObject::from(invalid))], + )?; + Ok(()) + }); + let pending = env.exception_check().unwrap(); + // Also clear on test failure so the failing regression cannot kill the JVM. + env.exception_clear().unwrap(); + assert!(!pending, "failed JNI call leaked a pending Java exception"); + assert!(result.unwrap_err().to_string().contains("not-a-number")); + assert_eq!(with_env(|_| Ok(42)).unwrap(), 42); + assert_eq!( + with_env::<(), _>(|_| Err(Error::new("native failure"))) + .unwrap_err() + .to_string(), + "native failure" + ); + // A Throwable with a null message must still be cleared. + let result: Result<(), Error> = with_env(|env| { + let exception = env.new_object("java/io/IOException", "()V", &[])?; + env.throw(jni::objects::JThrowable::from(exception))?; + Err(Error::new("JNI failure")) + }); + assert!(result.is_err()); + assert!(!env.exception_check().unwrap()); + } +} diff --git a/tests/README.md b/tests/README.md index 4e64d98..045757d 100644 --- a/tests/README.md +++ b/tests/README.md @@ -1,5 +1,10 @@ # Tests +The desktop JNI exception regression requires a JDK (`JAVA_HOME`): +`cargo test --manifest-path tests/jni-bridge/Cargo.toml`. It compiles the actual +JNI bridge without desktop Tauri/WebView dependencies, invokes a throwing Java method and +checks that the USB bridge returns the error with no pending Java exception. + ## Trust matrix | Layer | What it proves | Where | CI job | diff --git a/tests/jni-bridge/.gitignore b/tests/jni-bridge/.gitignore new file mode 100644 index 0000000..e9e2199 --- /dev/null +++ b/tests/jni-bridge/.gitignore @@ -0,0 +1,2 @@ +/target/ +/Cargo.lock diff --git a/tests/jni-bridge/Cargo.toml b/tests/jni-bridge/Cargo.toml new file mode 100644 index 0000000..a799fd1 --- /dev/null +++ b/tests/jni-bridge/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "jni-bridge-regression" +version = "0.0.0" +edition = "2021" +publish = false + +# Run the actual JNI boundary against a JVM without linking desktop Tauri/WebView. +[workspace] + +[dev-dependencies] +jni = { version = "0.21", features = ["invocation"] } +serde = "1.0" +serialport = { version = "4.10", default-features = false } diff --git a/tests/jni-bridge/src/lib.rs b/tests/jni-bridge/src/lib.rs new file mode 100644 index 0000000..87ff973 --- /dev/null +++ b/tests/jni-bridge/src/lib.rs @@ -0,0 +1,14 @@ +#![cfg(test)] +#![allow(dead_code)] + +#[path = "../../../src/error.rs"] +mod error; +#[path = "../../../src/android/fd_bridge.rs"] +mod fd_bridge; +#[path = "../../../src/jni_ready.rs"] +mod jni_ready; + +#[macro_export] +macro_rules! log_error { + ($($arg:tt)*) => { eprintln!($($arg)*); }; +}