Fix unwinding while using writeback parameters

Author: madsmtm

crates/objc2/CHANGELOG.md

@@ -235,6 +235,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   nullable argument.
 * Fixed handling of methods that return NULL errors. This affected for example
   `-[MTLBinaryArchive serializeToURL:error:]`.
+* Fixed unwinding while using writeback / error parameters.
 
 
 ## 0.5.2 - 2024-05-21

crates/objc2/src/__macro_helpers/convert.rs

@@ -13,26 +13,34 @@ mod argument_private {
 /// Objective-C `BOOL`, where it would otherwise not be allowed (since they
 /// are not ABI compatible).
 ///
-/// This is also done specially for `&mut Retained<_>`-like arguments, to allow
-/// using those as "out" parameters.
+/// This is also done specially for `&mut Retained<_>`-like arguments, to
+/// allow using those as "out" / pass-by-writeback parameters.
 pub trait ConvertArgument: argument_private::Sealed {
     /// The inner type that this can be converted to and from.
     #[doc(hidden)]
     type __Inner: EncodeArgument;
 
     /// A helper type for out parameters.
+    ///
+    /// When dropped, this will process any necessary change to the
+    /// parameters.
     #[doc(hidden)]
-    type __StoredBeforeMessage: Sized;
+    type __WritebackOnDrop: Sized;
 
     #[doc(hidden)]
     fn __from_defined_param(inner: Self::__Inner) -> Self;
 
+    /// # Safety
+    ///
+    /// The `__WritebackOnDrop` return type must not be leaked, and the
+    /// `__Inner` pointer must not be used after the `__WritebackOnDrop` has
+    /// dropped.
+    ///
+    /// NOTE: The standard way to ensure such a thing is with closures, but
+    /// using those would interact poorly with backtraces of the message send,
+    /// so we're forced to ensure this out of band.
     #[doc(hidden)]
-    fn __into_argument(self) -> (Self::__Inner, Self::__StoredBeforeMessage);
-
-    #[doc(hidden)]
-    #[inline]
-    unsafe fn __process_after_message_send(_stored: Self::__StoredBeforeMessage) {}
+    unsafe fn __into_argument(self) -> (Self::__Inner, Self::__WritebackOnDrop);
 }
 
 // Implemented in writeback.rs
@@ -45,15 +53,15 @@ impl<T: EncodeArgument> argument_private::Sealed for T {}
 impl<T: EncodeArgument> ConvertArgument for T {
     type __Inner = Self;
 
-    type __StoredBeforeMessage = ();
+    type __WritebackOnDrop = ();
 
     #[inline]
     fn __from_defined_param(inner: Self::__Inner) -> Self {
         inner
     }
 
     #[inline]
-    fn __into_argument(self) -> (Self::__Inner, Self::__StoredBeforeMessage) {
+    unsafe fn __into_argument(self) -> (Self::__Inner, Self::__WritebackOnDrop) {
         (self, ())
     }
 }
@@ -62,15 +70,15 @@ impl argument_private::Sealed for bool {}
 impl ConvertArgument for bool {
     type __Inner = Bool;
 
-    type __StoredBeforeMessage = ();
+    type __WritebackOnDrop = ();
 
     #[inline]
     fn __from_defined_param(inner: Self::__Inner) -> Self {
         inner.as_bool()
     }
 
     #[inline]
-    fn __into_argument(self) -> (Self::__Inner, Self::__StoredBeforeMessage) {
+    unsafe fn __into_argument(self) -> (Self::__Inner, Self::__WritebackOnDrop) {
         (Bool::new(self), ())
     }
 }
@@ -127,13 +135,10 @@ pub trait ConvertArguments {
     type __Inner: EncodeArguments;
 
     #[doc(hidden)]
-    type __StoredBeforeMessage: Sized;
-
-    #[doc(hidden)]
-    fn __into_arguments(self) -> (Self::__Inner, Self::__StoredBeforeMessage);
+    type __WritebackOnDrop: Sized;
 
     #[doc(hidden)]
-    unsafe fn __process_after_message_send(_stored: Self::__StoredBeforeMessage);
+    unsafe fn __into_arguments(self) -> (Self::__Inner, Self::__WritebackOnDrop);
 }
 
 pub trait TupleExtender<T> {
@@ -148,22 +153,16 @@ macro_rules! args_impl {
         impl<$($t: ConvertArgument),*> ConvertArguments for ($($t,)*) {
             type __Inner = ($($t::__Inner,)*);
 
-            type __StoredBeforeMessage = ($($t::__StoredBeforeMessage,)*);
+            type __WritebackOnDrop = ($($t::__WritebackOnDrop,)*);
 
             #[inline]
-            fn __into_arguments(self) -> (Self::__Inner, Self::__StoredBeforeMessage) {
+            unsafe fn __into_arguments(self) -> (Self::__Inner, Self::__WritebackOnDrop) {
                 let ($($a,)*) = self;
-                $(let $a = ConvertArgument::__into_argument($a);)*
+                // SAFETY: Upheld by caller
+                $(let $a = unsafe { ConvertArgument::__into_argument($a) };)*
 
                 (($($a.0,)*), ($($a.1,)*))
             }
-
-            #[inline]
-            unsafe fn __process_after_message_send(($($a,)*): Self::__StoredBeforeMessage) {
-                $(
-                    unsafe { <$t as ConvertArgument>::__process_after_message_send($a) };
-                )*
-            }
         }
 
         impl<$($t,)* T> TupleExtender<T> for ($($t,)*) {
@@ -296,7 +295,7 @@ mod tests {
             TypeId::of::<i32>()
         );
         assert_eq!(<i32 as ConvertArgument>::__from_defined_param(42), 42);
-        assert_eq!(ConvertArgument::__into_argument(42i32).0, 42);
+        assert_eq!(unsafe { ConvertArgument::__into_argument(42i32).0 }, 42);
     }
 
     #[test]
@@ -306,7 +305,7 @@ mod tests {
             TypeId::of::<i8>()
         );
         assert_eq!(<i8 as ConvertArgument>::__from_defined_param(-3), -3);
-        assert_eq!(ConvertArgument::__into_argument(-3i32).0, -3);
+        assert_eq!(unsafe { ConvertArgument::__into_argument(-3i32).0 }, -3);
     }
 
     #[test]
@@ -316,8 +315,8 @@ mod tests {
         assert!(!<bool as ConvertReturn>::__from_return(Bool::NO));
         assert!(<bool as ConvertReturn>::__from_return(Bool::YES));
 
-        assert!(!ConvertArgument::__into_argument(false).0.as_bool());
-        assert!(ConvertArgument::__into_argument(true).0.as_bool());
+        assert!(!unsafe { ConvertArgument::__into_argument(false).0 }.as_bool());
+        assert!(unsafe { ConvertArgument::__into_argument(true).0 }.as_bool());
         assert!(!ConvertReturn::__into_defined_return(false).as_bool());
         assert!(ConvertReturn::__into_defined_return(true).as_bool());
 

crates/objc2/src/__macro_helpers/msg_send.rs

@@ -21,21 +21,17 @@ pub trait MsgSend: Sized {
         A: ConvertArguments,
         R: ConvertReturn,
     {
-        let (args, stored) = A::__into_arguments(args);
+        // SAFETY: The writeback helper is not leaked (it is dropped at the
+        // end of this scope).
+        //
+        // TODO: If we want `objc_retainAutoreleasedReturnValue` to work, we
+        // must not do any work before it has been run; so somehow, we should
+        // do that _before_ the helper is dropped!
+        let (args, _helper) = unsafe { A::__into_arguments(args) };
 
-        // SAFETY: Upheld by caller
+        // SAFETY: Upheld by caller.
         let result = unsafe { MessageReceiver::send_message(self.into_raw_receiver(), sel, args) };
 
-        // TODO: If we want `objc_retainAutoreleasedReturnValue` to
-        // work, we must not do any work before it has been run; so
-        // somehow, we should do that _before_ this call!
-        //
-        // SAFETY: The argument was passed to the message sending
-        // function, and the stored values are only processed this
-        // once. See `src/__macro_helpers/writeback.rs` for
-        // details.
-        unsafe { A::__process_after_message_send(stored) };
-
         R::__from_return(result)
     }
 
@@ -46,16 +42,15 @@ pub trait MsgSend: Sized {
         A: ConvertArguments,
         R: ConvertReturn,
     {
-        let (args, stored) = A::__into_arguments(args);
+        // SAFETY: The writeback helper is not leaked (it is dropped at the
+        // end of this scope).
+        let (args, _helper) = unsafe { A::__into_arguments(args) };
 
-        // SAFETY: Upheld by caller
+        // SAFETY: Upheld by caller.
         let result = unsafe {
             MessageReceiver::send_super_message(self.into_raw_receiver(), superclass, sel, args)
         };
 
-        // SAFETY: Same as in send_message above.
-        unsafe { A::__process_after_message_send(stored) };
-
         R::__from_return(result)
     }