Slabs and handles

CHANGELOG.md

@@ -14,10 +14,18 @@
 
 [All changes in [[UnreleasedUniFFIVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.25.0...HEAD).
 
+### What's Fixed
+- Fixed potential use-after-free error when lowering object handles (#1797)
+
 ### What's changed?
 
 - 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 slab/handle system.
+- Trait interfaces defined in UDL need to be wrapped with `#[uniffi::trait_interface]`.
+- Trait interfaces performance has been improved.  If a trait interface handle is passed across the
+  FFI multiple times, UniFFI will only wrap the object once rather than each time it's passed.
 
 ## v0.25.0 (backend crates: v0.25.0) - (_2023-10-18_)
 

docs/manual/src/udl/interfaces.md

@@ -87,9 +87,10 @@ interface Button {
 
 ```
 
-With the following Rust implementation:
+The Rust implementation needs to be wrapped in `#[uniffi::trait_interface]`:
 
 ```rust
+#[uniffi::trait_interface]
 pub trait Button: Send + Sync {
     fn name(&self) -> String;
 }

examples/callbacks/src/lib.rs

@@ -21,6 +21,7 @@ impl From<uniffi::UnexpectedUniFFICallbackError> for TelephoneError {
 }
 
 // SIM cards.
+#[uniffi::trait_interface]
 pub trait SimCard: Send + Sync {
     fn name(&self) -> String;
 }
@@ -39,6 +40,7 @@ fn get_sim_cards() -> Vec<Arc<dyn SimCard>> {
 }
 
 // The call-answer, callback interface.
+#[uniffi::trait_interface]
 pub trait CallAnswerer {
     fn answer(&self) -> Result<String, TelephoneError>;
 }

examples/sprites/tests/bindings/test_sprites.kts

@@ -16,7 +16,7 @@ s.destroy()
 try {
     s.moveBy(Vector(0.0, 0.0))
     assert(false) { "Should not be able to call anything after `destroy`" }
-} catch(e: IllegalStateException) {
+} catch(e: InternalException) {
     assert(true)
 }
 

examples/traits/src/lib.rs

@@ -13,6 +13,7 @@ fn press(button: Arc<dyn Button>) -> Arc<dyn Button> {
     button
 }
 
+#[uniffi::trait_interface]
 pub trait Button: Send + Sync {
     fn name(&self) -> String;
 }

fixtures/coverall/src/traits.rs

@@ -10,6 +10,7 @@ pub fn get_traits() -> Vec<Arc<dyn NodeTrait>> {
     vec![Arc::new(Trait1::default()), Arc::new(Trait2::default())]
 }
 
+#[uniffi::trait_interface]
 pub trait NodeTrait: Send + Sync + std::fmt::Debug {
     fn name(&self) -> String;
 
@@ -35,6 +36,7 @@ pub fn ancestor_names(node: Arc<dyn NodeTrait>) -> Vec<String> {
 /// Test trait
 ///
 /// The goal here is to test all possible arg, return, and error types.
+#[uniffi::trait_interface]
 pub trait Getters: Send + Sync {
     fn get_bool(&self, v: bool, arg2: bool) -> bool;
     fn get_string(&self, v: String, arg2: bool) -> Result<String, CoverallError>;

fixtures/coverall/tests/bindings/test_coverall.kts

@@ -344,12 +344,10 @@ getTraits().let { traits ->
     assert(traits[1].name() == "node-2")
     assert(traits[1].strongCount() == 2UL)
 
-    // Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-    // Swift impl before passing it to `setParent()`
     traits[0].setParent(traits[1])
     assert(ancestorNames(traits[0]) == listOf("node-2"))
     assert(ancestorNames(traits[1]).isEmpty())
-    assert(traits[1].strongCount() == 2UL)
+    assert(traits[1].strongCount() == 3UL)
     assert(traits[0].getParent()!!.name() == "node-2")
 
     val ktNode = KotlinNode()
@@ -358,6 +356,10 @@ getTraits().let { traits ->
     assert(ancestorNames(traits[1]) == listOf("node-kt"))
     assert(ancestorNames(ktNode) == listOf<String>())
 
+    // If we get the node back from Rust, we should get the original object, not an object that's
+    // been wrapped again.
+    assert(traits[1].getParent() === ktNode)
+
     traits[1].setParent(null)
     ktNode.setParent(traits[0])
     assert(ancestorNames(ktNode) == listOf("node-1", "node-2"))

fixtures/coverall/tests/bindings/test_coverall.py

@@ -332,14 +332,14 @@ def strong_count(self):
 
 class TraitsTest(unittest.TestCase):
     # Test traits implemented in Rust
-    # def test_rust_getters(self):
-    #     test_getters(None)
-    #     self.check_getters_from_python(make_rust_getters())
+    def test_rust_getters(self):
+        test_getters(make_rust_getters())
+        self.check_getters_from_python(make_rust_getters())
 
-    # Test traits implemented in Rust
+    # Test traits implemented in Python
     def test_python_getters(self):
         test_getters(PyGetters())
-        #self.check_getters_from_python(PyGetters())
+        self.check_getters_from_python(PyGetters())
 
     def check_getters_from_python(self, getters):
         self.assertEqual(getters.get_bool(True, True), False);
@@ -370,7 +370,11 @@ def check_getters_from_python(self, getters):
         with self.assertRaises(ComplexError.UnknownError):
             getters.get_option("unknown-error", True)
 
-        with self.assertRaises(InternalError):
+        # If the trait is implmented in Rust, we should see an `InternalError`.
+
+        # If it's implemented in Python, we see a `RuntimeError` since it's a direct call with no
+        # UniFFI wrapping.
+        with self.assertRaises((InternalError, RuntimeError)):
             getters.get_string("unexpected-error", True)
 
     def test_path(self):
@@ -386,9 +390,7 @@ def test_path(self):
 
         # Let's try connecting them together
         traits[0].set_parent(traits[1])
-        # Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-        # python impl before passing it to `set_parent()`
-        self.assertEqual(traits[1].strong_count(), 2)
+        self.assertEqual(traits[1].strong_count(), 3)
         self.assertEqual(ancestor_names(traits[0]), ["node-2"])
         self.assertEqual(ancestor_names(traits[1]), [])
         self.assertEqual(traits[0].get_parent().name(), "node-2")
@@ -401,6 +403,10 @@ def test_path(self):
         self.assertEqual(ancestor_names(traits[1]), ["node-py"])
         self.assertEqual(ancestor_names(py_node), [])
 
+        # If we get the node back from Rust, we should get the original object, not an object that's
+        # been wrapped again.
+        self.assertIs(traits[1].get_parent(), py_node)
+
         # Rotating things.
         # The ancestry chain now goes py_node -> traits[0] -> traits[1]
         traits[1].set_parent(None)

fixtures/coverall/tests/bindings/test_coverall.swift

@@ -384,12 +384,10 @@ do {
     assert(traits[1].name() == "node-2")
     assert(traits[1].strongCount() == 2)
 
-    // Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-    // Swift impl before passing it to `set_parent()`
     traits[0].setParent(parent: traits[1])
     assert(ancestorNames(node: traits[0]) == ["node-2"])
     assert(ancestorNames(node: traits[1]) == [])
-    assert(traits[1].strongCount() == 2)
+    assert(traits[1].strongCount() == 3)
     assert(traits[0].getParent()!.name() == "node-2")
 
     // Throw in a Swift implementation of the trait
@@ -400,6 +398,10 @@ do {
     assert(ancestorNames(node: traits[1]) == ["node-swift"])
     assert(ancestorNames(node: swiftNode) == [])
 
+    // If we get the node back from Rust, we should get the original object, not an object that's
+    // been wrapped again.
+    assert(traits[1].getParent() === swiftNode)
+
     // Rotating things.
     // The ancestry chain now goes swiftNode -> traits[0] -> traits[1]
     traits[1].setParent(parent: nil)