fix: manually drop room if it's last in Arc

Author: articice

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"] }

bindings/matrix-sdk-ffi/src/client.rs

@@ -3281,4 +3281,52 @@ 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<rusqlite::Connection>::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;
+        use matrix_sdk::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");
+    }
 }

bindings/matrix-sdk-ffi/src/room/mod.rs

@@ -98,13 +98,13 @@ impl From<RoomState> for Membership {
 
 #[derive(uniffi::Object)]
 pub struct Room {
-    pub(super) inner: SdkRoom,
+    pub(super) inner: AsyncRuntimeDropped<SdkRoom>,
     utd_hook_manager: Option<Arc<UtdHookManager>>,
 }
 
 impl Room {
     pub(crate) fn new(inner: SdkRoom, utd_hook_manager: Option<Arc<UtdHookManager>>) -> Self {
-        Room { inner, utd_hook_manager }
+        Room { inner: AsyncRuntimeDropped::new(inner), utd_hook_manager }
     }
 }
 
@@ -1981,3 +1981,51 @@ impl TryFrom<SdkRoomSendQueueUpdate> 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<rusqlite::Connection>::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");
+    }
+}

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
         }
     }