Use Handle for passing foreign objects to Rust

Consolidated the various handle maps into a single implementation for
each language.  This handle map works basically the same as all the
others, but it's API is based on the `HandleAlloc` trait.  Handles have
a couple properties:

* All foreign handles are odd, which allows us to distinguish between
  Rust and foreign handles.
* For handles store a map ID that can detect when a handle is used with
  the wrong map.

Made all languages always use the handle maps for passing objects.  No
more trying to leak pointers from to foreign objects.

Started updating the ForeignExecutor code to use handles, but this is
still a WIP while the ForeignExecutor type is in it's limbo state.

Author: bendk

CHANGELOG.md

@@ -19,7 +19,11 @@
 - The `rust_future_continuation_callback_set` FFI function was removed.  `rust_future_poll` now
   inputs the callback pointer.  External bindings authors will need to update their code.
 - The object handle FFI has changed.  External bindings generators will need to update their code to
-  use the new handle system.
+  use the new handle system:
+    * A single `FfiType::Handle` is used for all object handles.
+    * `FfiType::Handle` is always a 64-bit int.
+    * Foreign handles must always set the lowest bit of that int.
+- The `NoPointer` singleton was renamed to `NoHandle`
 
 ### What's new?
 

fixtures/coverall/tests/bindings/test_coverall.kts

@@ -446,11 +446,11 @@ Coveralls("test_bytes").use { coveralls ->
 
 // Test fakes using open classes
 
-class FakePatch(private val color: Color): Patch(NoPointer) {
+class FakePatch(private val color: Color): Patch(NoHandle) {
     override fun `getColor`(): Color = color
 }
 
-class FakeCoveralls(private val name: String) : Coveralls(NoPointer) {
+class FakeCoveralls(private val name: String) : Coveralls(NoHandle) {
     private val repairs = mutableListOf<Repair>()
 
     override fun `addPatch`(patch: Patch) {

fixtures/foreign-executor/tests/bindings/test_foreign_executor.kts

@@ -37,11 +37,3 @@ runBlocking {
     assert(result.delayMs <= 200U)
     tester.close()
 }
-
-// Test that we cleanup when dropping a ForeignExecutor handles
-assert(FfiConverterForeignExecutor.handleCount() == 0)
-val tester = ForeignExecutorTester(coroutineScope)
-val tester2 = ForeignExecutorTester.newFromSequence(listOf(coroutineScope))
-tester.close()
-tester2.close()
-assert(FfiConverterForeignExecutor.handleCount() == 0)

fixtures/uitests/tests/ui/interface_trait_not_sync_and_send.stderr

@@ -1,3 +1,31 @@
+error[E0277]: `(dyn Trait + 'static)` cannot be shared between threads safely
+ --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
+  |
+  | #[::uniffi::export_for_udl]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn Trait + 'static)` cannot be shared between threads safely
+  |
+  = help: the trait `Sync` is not implemented for `(dyn Trait + 'static)`
+note: required by a bound in `HandleAlloc`
+ --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+  |
+  | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+  |                                          ^^^^ required by this bound in `HandleAlloc`
+  = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `(dyn Trait + 'static)` cannot be sent between threads safely
+ --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
+  |
+  | #[::uniffi::export_for_udl]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn Trait + 'static)` cannot be sent between threads safely
+  |
+  = help: the trait `Send` is not implemented for `(dyn Trait + 'static)`
+note: required by a bound in `HandleAlloc`
+ --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+  |
+  | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+  |                                   ^^^^ required by this bound in `HandleAlloc`
+  = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0277]: `(dyn Trait + 'static)` cannot be shared between threads safely
  --> $OUT_DIR[uniffi_uitests]/trait.uniffi.rs
   |
@@ -26,6 +54,34 @@ note: required by a bound in `FfiConverterArc`
   |                                       ^^^^ required by this bound in `FfiConverterArc`
   = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
+  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+   |
+11 | #[uniffi::export]
+   | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
+   |
+   = help: the trait `Sync` is not implemented for `(dyn ProcMacroTrait + 'static)`
+note: required by a bound in `HandleAlloc`
+  --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+   |
+   | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+   |                                          ^^^^ required by this bound in `HandleAlloc`
+   = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
+  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+   |
+11 | #[uniffi::export]
+   | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
+   |
+   = help: the trait `Send` is not implemented for `(dyn ProcMacroTrait + 'static)`
+note: required by a bound in `HandleAlloc`
+  --> $WORKSPACE/uniffi_core/src/ffi_converter_traits.rs
+   |
+   | pub unsafe trait HandleAlloc<UT>: Send + Sync {
+   |                                   ^^^^ required by this bound in `HandleAlloc`
+   = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
   --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
    |

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs

@@ -333,12 +333,10 @@ impl KotlinCodeOracle {
             }
             FfiType::ForeignBytes => "ForeignBytes.ByValue".to_string(),
             FfiType::ForeignCallback => "ForeignCallback".to_string(),
-            FfiType::ForeignExecutorHandle => "USize".to_string(),
             FfiType::ForeignExecutorCallback => "UniFfiForeignExecutorCallback".to_string(),
             FfiType::RustFutureContinuationCallback => {
                 "UniFffiRustFutureContinuationCallbackType".to_string()
             }
-            FfiType::RustFutureContinuationData => "USize".to_string(),
         }
     }
 

uniffi_bindgen/src/bindings/kotlin/templates/Async.kt

@@ -3,18 +3,18 @@
 internal const val UNIFFI_RUST_FUTURE_POLL_READY = 0.toShort()
 internal const val UNIFFI_RUST_FUTURE_POLL_MAYBE_READY = 1.toShort()
 
-internal val uniffiContinuationHandleMap = UniFfiHandleMap<CancellableContinuation<Short>>()
+internal val uniffiContinuationHandleMap = UniffiHandleMap<CancellableContinuation<Short>>()
 
 // FFI type for Rust future continuations
 internal object uniffiRustFutureContinuationCallback: UniFffiRustFutureContinuationCallbackType {
-    override fun callback(continuationHandle: USize, pollResult: Short) {
-        uniffiContinuationHandleMap.remove(continuationHandle)?.resume(pollResult)
+    override fun callback(continuationHandle: UniffiHandle, pollResult: Short) {
+        uniffiContinuationHandleMap.consumeHandle(continuationHandle).resume(pollResult)
     }
 }
 
 internal suspend fun<T, F, E: Exception> uniffiRustCallAsync(
     rustFuture: UniffiHandle,
-    pollFunc: (UniffiHandle, UniFffiRustFutureContinuationCallbackType, USize) -> Unit,
+    pollFunc: (UniffiHandle, UniFffiRustFutureContinuationCallbackType, UniffiHandle) -> Unit,
     completeFunc: (UniffiHandle, RustCallStatus) -> F,
     freeFunc: (UniffiHandle) -> Unit,
     liftFunc: (F) -> T,
@@ -26,7 +26,7 @@ internal suspend fun<T, F, E: Exception> uniffiRustCallAsync(
                 pollFunc(
                     rustFuture,
                     uniffiRustFutureContinuationCallback,
-                    uniffiContinuationHandleMap.insert(continuation)
+                    uniffiContinuationHandleMap.newHandle(continuation)
                 )
             }
         } while (pollResult != UNIFFI_RUST_FUTURE_POLL_READY);

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceImpl.kt

@@ -3,11 +3,11 @@
 // Implement the foreign callback handler for {{ interface_name }}
 internal class {{ callback_handler_class }} : ForeignCallback {
     @Suppress("TooGenericExceptionCaught")
-    override fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int {
+    override fun invoke(handle: UniffiHandle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int {
         val cb = {{ ffi_converter_name }}.handleMap.get(handle)
         return when (method) {
             IDX_CALLBACK_FREE -> {
-                {{ ffi_converter_name }}.handleMap.remove(handle)
+                {{ ffi_converter_name }}.handleMap.consumeHandle(handle)
 
                 // Successful return
                 // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt

@@ -1,39 +1,8 @@
 {{- self.add_import("java.util.concurrent.atomic.AtomicLong") }}
 {{- self.add_import("java.util.concurrent.locks.ReentrantLock") }}
-{{- self.add_import("kotlin.concurrent.withLock") }}
-
-internal typealias Handle = Long
-internal class ConcurrentHandleMap<T>(
-    private val leftMap: MutableMap<Handle, T> = mutableMapOf(),
-) {
-    private val lock = java.util.concurrent.locks.ReentrantLock()
-    private val currentHandle = AtomicLong(0L)
-    private val stride = 1L
-
-    fun insert(obj: T): Handle =
-        lock.withLock {
-            currentHandle.getAndAdd(stride)
-                .also { handle ->
-                    leftMap[handle] = obj
-                }
-            }
-
-    fun get(handle: Handle) = lock.withLock {
-        leftMap[handle] ?: throw InternalException("No callback in handlemap; this is a Uniffi bug")
-    }
-
-    fun delete(handle: Handle) {
-        this.remove(handle)
-    }
-
-    fun remove(handle: Handle): T? =
-        lock.withLock {
-            leftMap.remove(handle)
-        }
-}
 
 interface ForeignCallback : com.sun.jna.Callback {
-    public fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int
+    public fun invoke(handle: UniffiHandle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int
 }
 
 // Magic number for the Rust proxy to call using the same mechanism as every other method,
@@ -44,20 +13,16 @@ internal const val UNIFFI_CALLBACK_SUCCESS = 0
 internal const val UNIFFI_CALLBACK_ERROR = 1
 internal const val UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2
 
-public abstract class FfiConverterCallbackInterface<CallbackInterface>: FfiConverter<CallbackInterface, Handle> {
-    internal val handleMap = ConcurrentHandleMap<CallbackInterface>()
-
-    internal fun drop(handle: Handle) {
-        handleMap.remove(handle)
-    }
+public abstract class FfiConverterCallbackInterface<CallbackInterface>: FfiConverter<CallbackInterface, UniffiHandle> {
+    internal val handleMap = UniffiHandleMap<CallbackInterface>()
 
-    override fun lift(value: Handle): CallbackInterface {
+    override fun lift(value: UniffiHandle): CallbackInterface {
         return handleMap.get(value)
     }
 
     override fun read(buf: ByteBuffer) = lift(buf.getLong())
 
-    override fun lower(value: CallbackInterface) = handleMap.insert(value)
+    override fun lower(value: CallbackInterface) = handleMap.newHandle(value)
 
     override fun allocationSize(value: CallbackInterface) = 8
 

uniffi_bindgen/src/bindings/kotlin/templates/ForeignExecutorTemplate.kt

@@ -16,7 +16,7 @@ internal interface UniFfiRustTaskCallback : com.sun.jna.Callback {
 }
 
 internal object UniFfiForeignExecutorCallback : com.sun.jna.Callback {
-    fun callback(handle: USize, delayMs: Int, rustTask: UniFfiRustTaskCallback?, rustTaskData: Pointer?) : Byte {
+    fun callback(handle: UniffiHandle, delayMs: Int, rustTask: UniFfiRustTaskCallback?, rustTaskData: Pointer?) : Byte {
         if (rustTask == null) {
             FfiConverterForeignExecutor.drop(handle)
             return UNIFFI_FOREIGN_EXECUTOR_CALLBACK_SUCCESS
@@ -42,11 +42,11 @@ internal object UniFfiForeignExecutorCallback : com.sun.jna.Callback {
     }
 }
 
-public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, USize> {
-    internal val handleMap = UniFfiHandleMap<CoroutineScope>()
+public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, UniffiHandle> {
+    internal val handleMap = UniffiHandleMap<CoroutineScope>()
 
-    internal fun drop(handle: USize) {
-        handleMap.remove(handle)
+    internal fun drop(handle: UniffiHandle) {
+        handleMap.consumeHandle(handle)
     }
 
     internal fun register(lib: _UniFFILib) {
@@ -58,26 +58,21 @@ public object FfiConverterForeignExecutor: FfiConverter<CoroutineScope, USize> {
         {% endmatch %}
     }
 
-    // Number of live handles, exposed so we can test the memory management
-    public fun handleCount() : Int {
-        return handleMap.size
-    }
-
-    override fun allocationSize(value: CoroutineScope) = USize.size
+    override fun allocationSize(value: CoroutineScope) = 8
 
-    override fun lift(value: USize): CoroutineScope {
-        return handleMap.get(value) ?: throw RuntimeException("unknown handle in FfiConverterForeignExecutor.lift")
+    override fun lift(value: UniffiHandle): CoroutineScope {
+        return handleMap.get(value)
     }
 
     override fun read(buf: ByteBuffer): CoroutineScope {
-        return lift(USize.readFromBuffer(buf))
+        return lift(buf.getLong())
     }
 
-    override fun lower(value: CoroutineScope): USize {
-        return handleMap.insert(value)
+    override fun lower(value: CoroutineScope): UniffiHandle {
+        return handleMap.newHandle(value)
     }
 
     override fun write(value: CoroutineScope, buf: ByteBuffer) {
-        lower(value).writeToBuffer(buf)
+        buf.putLong(lower(value))
     }
 }

uniffi_bindgen/src/bindings/kotlin/templates/HandleMap.kt

@@ -0,0 +1,53 @@
+internal class UniffiHandleMap<T> {
+    private val lock = ReentrantReadWriteLock()
+    private var mapId: Long = UniffiHandleMap.nextMapId()
+    private val map: MutableMap<Long, T> = mutableMapOf()
+    // Note: Foreign handles are always odd
+    private var keyCounter = 1L
+
+    private fun nextKey(): Long = keyCounter.also {
+        keyCounter = (keyCounter + 2L).and(0xFFFF_FFFF_FFFFL)
+    }
+
+    private fun makeHandle(key: Long): UniffiHandle = key.or(mapId)
+
+    private fun key(handle: UniffiHandle): Long {
+        if (handle.and(0x7FFF_0000_0000_0000L) != mapId) {
+            throw InternalException("Handle map ID mismatch")
+        }
+        return handle.and(0xFFFF_FFFF_FFFFL)
+    }
+ 
+    fun newHandle(obj: T): UniffiHandle = lock.writeLock().withLock {
+        val key = nextKey()
+        map[key] = obj
+        makeHandle(key)
+    }
+ 
+    fun get(handle: UniffiHandle) = lock.readLock().withLock {
+        map[key(handle)] ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+    }
+ 
+    fun cloneHandle(handle: UniffiHandle): UniffiHandle = lock.writeLock().withLock {
+        val obj = map[key(handle)] ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+        val clone = nextKey()
+        map[clone] = obj
+        makeHandle(clone)
+    }
+ 
+    fun consumeHandle(handle: UniffiHandle): T = lock.writeLock().withLock {
+        map.remove(key(handle)) ?: throw InternalException("Missing key in handlemap: was the handle used after being freed?")
+    }
+
+    companion object {
+        // Generate map IDs that are likely to be unique
+        private var mapIdCounter: Long = {{ ci.namespace_hash() }}.and(0x7FFF)
+
+        // Map ID, shifted into the top 16 bits
+        internal fun nextMapId(): Long = mapIdCounter.shl(48).also {
+            // On Kotlin, map ids are only 15 bits to get around signed/unsigned issues
+            mapIdCounter = ((mapIdCounter + 1).and(0x7FFF))
+        }
+    }
+}
+