From c895e92c26cb79b1a4ace48b29a1faabbaf05b50 Mon Sep 17 00:00:00 2001 From: Artem Pylypchuk Date: Thu, 7 May 2026 10:44:28 +0300 Subject: [PATCH] fix: manually drop room if it's last in Arc --- bindings/matrix-sdk-ffi/Cargo.toml | 2 + bindings/matrix-sdk-ffi/src/client.rs | 47 ++++++++++++++++++++++ bindings/matrix-sdk-ffi/src/room/mod.rs | 52 ++++++++++++++++++++++++- bindings/matrix-sdk-ffi/src/search.rs | 2 +- bindings/matrix-sdk-ffi/src/widget.rs | 2 +- 5 files changed, 101 insertions(+), 4 deletions(-) diff --git a/bindings/matrix-sdk-ffi/Cargo.toml b/bindings/matrix-sdk-ffi/Cargo.toml index 56418bc8b..62b3d1db9 100644 --- a/bindings/matrix-sdk-ffi/Cargo.toml +++ b/bindings/matrix-sdk-ffi/Cargo.toml @@ -133,8 +133,10 @@ once_cell = "1.21.4" jvm-getter = "0.1.0" [dev-dependencies] +matrix-sdk = { workspace = true, features = ["testing"] } similar-asserts.workspace = true tempfile.workspace = true +tokio = { workspace = true, features = ["rt-multi-thread", "macros", "time"] } [build-dependencies] uniffi = { workspace = true, features = ["build"] } diff --git a/bindings/matrix-sdk-ffi/src/client.rs b/bindings/matrix-sdk-ffi/src/client.rs index 21568b64c..07617f3ec 100644 --- a/bindings/matrix-sdk-ffi/src/client.rs +++ b/bindings/matrix-sdk-ffi/src/client.rs @@ -3367,4 +3367,51 @@ mod tests { assert_eq!(token.matrix_server_name, "example.com"); assert_eq!(token.expires_in_seconds, 3_600); } + + /// Dropping an FFI [`Client`] on a non-tokio thread must not panic. + /// + /// Regression test: `Client.inner` is wrapped in [`AsyncRuntimeDropped`] + /// precisely because dropping a sqlite-backed [`MatrixClient`] outside a + /// tokio runtime context causes `SyncWrapper::drop` + /// to call `tokio::task::spawn_blocking`, which panics and triggers + /// SIGABRT. This test guards against accidentally removing that wrapper. + #[cfg(not(target_family = "wasm"))] + #[tokio::test] + async fn client_drop_on_non_tokio_thread_does_not_panic() { + use std::time::Duration; + + use matrix_sdk::{Client, config::RequestConfig}; + use matrix_sdk_common::cross_process_lock::CrossProcessLockConfig; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + + // SingleProcess lock avoids the session-delegate requirement in + // `Client::new`, keeping the test self-contained. + let sdk_client = Client::builder() + .homeserver_url("https://example.com") + .request_config(RequestConfig::new().disable_retry()) + .sqlite_store(dir.path(), None) + .cross_process_store_config(CrossProcessLockConfig::SingleProcess) + .build() + .await + .unwrap(); + + // Allow background init tasks to complete; after this the only strong + // reference to `ClientInner` is the local `sdk_client` variable. + tokio::time::sleep(Duration::from_secs(1)).await; + + let ffi_client = super::Client::new(sdk_client.clone(), None, None).await.unwrap(); + + // Dropping `sdk_client` leaves `ffi_client` as the sole Arc holder of + // `ClientInner` — mirroring what happens when the last JS reference to + // a Client is garbage-collected on the Hermes JS thread. + drop(sdk_client); + + // Simulate Hermes GC on the JS thread (a non-tokio thread). + // Without `AsyncRuntimeDropped` wrapping `Client.inner` this SIGABRT. + std::thread::spawn(move || drop(ffi_client)) + .join() + .expect("Client::drop panicked on a non-tokio thread"); + } } diff --git a/bindings/matrix-sdk-ffi/src/room/mod.rs b/bindings/matrix-sdk-ffi/src/room/mod.rs index ed6b456d5..ad88e73b9 100644 --- a/bindings/matrix-sdk-ffi/src/room/mod.rs +++ b/bindings/matrix-sdk-ffi/src/room/mod.rs @@ -98,13 +98,13 @@ impl From for Membership { #[derive(uniffi::Object)] pub struct Room { - pub(super) inner: SdkRoom, + pub(super) inner: AsyncRuntimeDropped, utd_hook_manager: Option>, } impl Room { pub(crate) fn new(inner: SdkRoom, utd_hook_manager: Option>) -> Self { - Room { inner, utd_hook_manager } + Room { inner: AsyncRuntimeDropped::new(inner), utd_hook_manager } } } @@ -1981,3 +1981,51 @@ impl TryFrom for RoomSendQueueUpdate { }) } } + +#[cfg(all(test, not(target_family = "wasm")))] +mod tests { + use std::time::Duration; + + use matrix_sdk::{ruma::room_id, test_utils::mocks::MatrixMockServer}; + use tempfile::tempdir; + + use super::*; + + /// Dropping an FFI [`Room`] on a non-tokio thread must not panic. + /// + /// Regression test: when `Room.inner` was `SdkRoom` (not wrapped in + /// [`AsyncRuntimeDropped`]), garbage-collection of an orphaned `Room` on + /// the Hermes JS thread caused `SyncWrapper::drop` + /// to call `tokio::task::spawn_blocking` from a thread with no tokio + /// runtime context. Rust turns a panic inside `Drop` into SIGABRT. + /// + /// The fix wraps `inner` in [`AsyncRuntimeDropped`], which enters the + /// global tokio runtime context before running `SdkRoom`'s destructor. + #[tokio::test] + async fn room_drop_on_non_tokio_thread_does_not_panic() { + let server = MatrixMockServer::new().await; + let dir = tempdir().unwrap(); + + let client = + server.client_builder().on_builder(|b| b.sqlite_store(dir.path(), None)).build().await; + + // Allow background init tasks to complete; after this the only strong + // reference to `ClientInner` is the local `client` variable. + tokio::time::sleep(Duration::from_secs(1)).await; + + let room_id = room_id!("!test:example.com"); + let sdk_room = server.sync_joined_room(&client, room_id).await; + let ffi_room = Room::new(sdk_room, None); + + // Dropping `client` makes `ffi_room` the sole Arc holder of + // `ClientInner`, mirroring the situation where the FFI client has + // already been released and GC finalises the last room JS object. + drop(client); + + // Simulate Hermes GC on the JS thread (a non-tokio thread). + // Without the `AsyncRuntimeDropped` wrapper this causes a SIGABRT. + std::thread::spawn(move || drop(ffi_room)) + .join() + .expect("Room::drop panicked on a non-tokio thread"); + } +} diff --git a/bindings/matrix-sdk-ffi/src/search.rs b/bindings/matrix-sdk-ffi/src/search.rs index e65b54c52..60045321c 100644 --- a/bindings/matrix-sdk-ffi/src/search.rs +++ b/bindings/matrix-sdk-ffi/src/search.rs @@ -54,7 +54,7 @@ impl Room { /// a time. pub fn search_messages(&self, query: String, num_results_per_batch: u32) -> RoomSearchIterator { RoomSearchIterator { - sdk_room: self.inner.clone(), + sdk_room: (*self.inner).clone(), inner: Mutex::new(self.inner.search_messages(query, num_results_per_batch as usize)), } } diff --git a/bindings/matrix-sdk-ffi/src/widget.rs b/bindings/matrix-sdk-ffi/src/widget.rs index 04a80acce..bbdd8f7ac 100644 --- a/bindings/matrix-sdk-ffi/src/widget.rs +++ b/bindings/matrix-sdk-ffi/src/widget.rs @@ -55,7 +55,7 @@ impl WidgetDriver { }; let capabilities_provider = CapabilitiesProviderWrap(capabilities_provider.into()); - if let Err(()) = driver.run(room.inner.clone(), capabilities_provider).await { + if let Err(()) = driver.run((*room.inner).clone(), capabilities_provider).await { // TODO } }