Handles #1823

bendk · 2023-11-01T00:07:09Z

This PR contains the parts of #1808 that I think are most beneficial and least controversial:

Consolidate the handle code. Use one FfiType variant for all handles. Implement one system
in Rust and one system in each foreign language rather than multiple systems. This makes it
easier to add new object/handle types.
Handles should be clonable. This is needed to fix Object.lower() can result in use-after-free errors #1797.
Rust and foreign handles should be distinguishable. This will make the trait interface implementation more efficient.

This PR sets things up to fix #1797, but doesn't actually fix it. That's going to take some more code and I want to do that in a separate PR.

It also doesn't implement the trait interface changes, because when I did that I hit #1797. In the traits example, we pass a trait interface into Rust, then Rust immediately returns it. This is what happens Rust returns it:

It copies the handle in lower()
It drops the struct, invoking the IDX_CALLBACK_FREE method.
It then returns the handle back to the foreign language, which is use-after-free.

tarikeshaq

Apologies this fell through my review queue, I'll redirect to @mhammond as he's been more recently involved and I'd most likely not provide the best review without playing catchup

arg0d · 2023-11-08T12:26:04Z

uniffi_core/src/ffi/handle.rs

+///   leaked pointer will be aligned).
+///
+/// Foreign handles for internal bindings languages look like this:
+///   * Bits 0-48 are the map key.  Keys are always odd, so the lowest bit is always set.


Nice one, I was thinking it would be MSB to check range [0; u64::max/2), but checking LSB seems less error prone. Also LSB is friendly with map identifier bits.

`Handle` is a u64 newtype that I hope to use to represent objects that get passed across the FFI. `HandleAlloc` is an ffi trait that converts between `Arc<>` and `Handle`.

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.

Added the `BlockingTaskQueue` type BlockingTaskQueue allows a Rust closure to be scheduled on a foreign thread where blocking operations are okay. The closure runs inside the parent future, which is nice because it allows the closure to reference its outside scope. On the foreign side, a `BlockingTaskQueue` is a native type that runs a task in some sort of thread queue (`DispatchQueue`, `CoroutineContext`, `futures.Executor`, etc.). Updated some of the foreign code so that the handle-maps used for callback interfaces can also be used for this. It's quite messy right now, but mozilla#1823 should clean things up. Renamed `Handle` to `UniffiHandle` on Kotlin, since `Handle` can easily conflict with user names. In fact it did on the `custom_types` fixture, the only reason that this worked before was that that fixture didn't use callback interfaces. Added new tests for this in the futures fixtures. Updated the tests to check that handles are being released properly.

I'm trying to get mozilla#1823 merged now that 0.26.0 is out, but I'll break it up a bit to make it easier. The first step is consolidating the foreign handle map code. Foreign languages define 1 handle map and use it for all object types. I tried to simplify the handle map implementations, the had a lot of features that we weren't using at all and seemed to be only half-implemneted to me, like the right map and counter map. Handles are always `u64` values. This allows us to remove some code like the `USize` handling on Kotlin and the `FfiType::RustFutureContinuationData` variant.

I'm trying to get mozilla#1823 merged now that 0.26.0 is out, but I'll break it up a bit to make it easier. The first step is consolidating the foreign handle map code. Foreign now languages define 1 handle map and use it for all object types. I tried to simplify the handle map implementations, the had a lot of features that we weren't using at all and seemed to be only half-implemneted to me, like the right map and counter map. Handles are always `u64` values. This allows us to remove some code like the `USize` handling on Kotlin and the `FfiType::RustFutureContinuationData` variant.

I'm trying to get mozilla#1823 merged now that 0.26.0 is out, but I'll break it up a bit to make it easier. The first step is consolidating the foreign handle map code. Foreign now languages define 1 handle map and use it for all object types. I tried to simplify the handle map implementations, the had a lot of features that we weren't using at all and seemed to be only half-implemented to me, like the right map and counter map. Handles are always `u64` values. This allows us to remove some code like the `USize` handling on Kotlin and the `FfiType::RustFutureContinuationData` variant.

I'm trying to get #1823 merged now that 0.26.0 is out, but I'll break it up a bit to make it easier. The first step is consolidating the foreign handle map code. Foreign now languages define 1 handle map and use it for all object types. I tried to simplify the handle map implementations, the had a lot of features that we weren't using at all and seemed to be only half-implemented to me, like the right map and counter map. Handles are always `u64` values. This allows us to remove some code like the `USize` handling on Kotlin and the `FfiType::RustFutureContinuationData` variant.

FfiType::Handle is a opaque 64-bit handle that's used to pass objects across the FFI. This PR changes the rustfuture code to use it and also renames the type used to represent callback interfaces from `u64` to handle. The plan is to use it for all object types, including interfaces and trait interfaces, but do that in future PRs. Accompianing that code is the `HandleAlloc` trait, which has a general system for allocating/using/freeing handles for objects. It's essentially the same system(s) we're currently using, but now the code is all in one place. On the bindings side, switched the code to using the `u64` type rather than having a `UniffiHandle` type alias. I don't think the type alias is very useful since handles are only used internally. Also, I remember running into Switch issues with the type alias and multiple crates. Most of this code was copied from the handles PR (mozilla#1823).

FfiType::Handle is a opaque 64-bit handle that's used to pass objects across the FFI. This PR changes the rustfuture code to use it and also renames the type used to represent callback interfaces from `u64` to handle. The plan is to use it for all object types, including interfaces and trait interfaces, but that will be done in future PRs. Accompianing that code is the `HandleAlloc` trait, which has a general system for allocating/using/freeing handles for objects. It's essentially the same system(s) we're currently using, but now the code is all in one place. On the bindings side, switched the code to using the `u64` type rather than having a `UniffiHandle` type alias. I don't think the type alias is very useful since handles are only used internally. Also, I remember running into Switch issues with the type alias and multiple crates. Most of this code was copied from the handles PR (mozilla#1823).

FfiType::Handle is a opaque 64-bit handle that's used to pass objects across the FFI. This PR changes the rustfuture code to use it and also renames the type used to represent callback interfaces from `u64` to handle. The plan is to use it for all object types, including interfaces and trait interfaces, but that will be done in future PRs. Accompianing that code is the `HandleAlloc` trait, which has a general system for allocating/using/freeing handles for objects. It's essentially the same system(s) we're currently using, but now the code is all in one place. On the bindings side, switched the code to using the `u64` type rather than having a `UniffiHandle` type alias. I don't think the type alias is very useful since handles are only used internally. Also, I remember running into Swift issues with the type alias and multiple crates. Most of this code was copied from the handles PR (mozilla#1823).

FfiType::Handle is a opaque 64-bit handle that's used to pass objects across the FFI. This PR changes the rustfuture code to use it and also renames the type used to represent callback interfaces from `u64` to handle. The plan is to use it for all object types, including interfaces and trait interfaces, but that will be done in future PRs. Accompianing that code is the `HandleAlloc` trait, which has a general system for allocating/using/freeing handles for objects. It's essentially the same system(s) we're currently using, but now the code is all in one place. On the bindings side, switched the code to using the `u64` type rather than having a `UniffiHandle` type alias. I don't think the type alias is very useful since handles are only used internally. Also, I remember running into Swift issues with the type alias and multiple crates. Most of this code was copied from the handles PR (#1823).

Added the `BlockingTaskQueue` type BlockingTaskQueue allows a Rust closure to be scheduled on a foreign thread where blocking operations are okay. The closure runs inside the parent future, which is nice because it allows the closure to reference its outside scope. On the foreign side, a `BlockingTaskQueue` is a native type that runs a task in some sort of thread queue (`DispatchQueue`, `CoroutineContext`, `futures.Executor`, etc.). Updated some of the foreign code so that the handle-maps used for callback interfaces can also be used for this. It's quite messy right now, but mozilla#1823 should clean things up. Renamed `Handle` to `UniffiHandle` on Kotlin, since `Handle` can easily conflict with user names. In fact it did on the `custom_types` fixture, the only reason that this worked before was that that fixture didn't use callback interfaces. Added new tests for this in the futures fixtures. Updated the tests to check that handles are being released properly.

bendk requested a review from a team as a code owner November 1, 2023 00:07

bendk requested review from tarikeshaq and removed request for a team November 1, 2023 00:07

bendk mentioned this pull request Nov 1, 2023

Handles part 2 #1826

Closed

bendk force-pushed the handles branch 4 times, most recently from 9c5ad98 to 56300c3 Compare November 2, 2023 17:38

tarikeshaq reviewed Nov 7, 2023

View reviewed changes

tarikeshaq requested a review from mhammond November 7, 2023 16:31

arg0d reviewed Nov 8, 2023

View reviewed changes

bendk mentioned this pull request Nov 9, 2023

Async blocking task queue #1837

Open

bendk force-pushed the handles branch 2 times, most recently from 6a7ea36 to 00d58c2 Compare November 9, 2023 18:11

bendk force-pushed the handles branch 3 times, most recently from bc5e827 to 3de393c Compare November 21, 2023 01:12

bendk mentioned this pull request Nov 22, 2023

Adding ADR-0009 #1861

Open

bendk added 3 commits November 22, 2023 17:03

Adding Handle and HandleAlloc

49e6d77

`Handle` is a u64 newtype that I hope to use to represent objects that get passed across the FFI. `HandleAlloc` is an ffi trait that converts between `Arc<>` and `Handle`.

Adding FfiType::Handle and use it for Rust objects

b06a1f8

bendk force-pushed the handles branch from 3de393c to cfe6e01 Compare November 22, 2023 22:05

bendk mentioned this pull request Dec 21, 2023

0.26.0 Release #1911

Closed

2 tasks

bendk mentioned this pull request Jan 29, 2024

Consolidate/Simplify the foreign bindings handlemap code #1969

Merged

bendk mentioned this pull request Jan 30, 2024

Adding FfiType::Handle and use it for Rust futures #1972

Merged

bendk mentioned this pull request Apr 19, 2024

Added ffi-buffer scaffolding functions #2039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handles #1823

Handles #1823

bendk commented Nov 1, 2023

tarikeshaq left a comment

arg0d Nov 8, 2023

Handles #1823

Are you sure you want to change the base?

Handles #1823

Conversation

bendk commented Nov 1, 2023

tarikeshaq left a comment

Choose a reason for hiding this comment

arg0d Nov 8, 2023

Choose a reason for hiding this comment