Fix exception memory management

Author: madsmtm

crates/objc-sys/build.rs

@@ -210,9 +210,7 @@ fn main() {
     // - ...
     //
     // TODO: -fobjc-weak ?
-    let mut cc_args = format!(
-        "-fobjc-arc -fobjc-arc-exceptions -fobjc-exceptions -fobjc-runtime={clang_runtime}"
-    );
+    let mut cc_args = format!("-fobjc-exceptions -fobjc-runtime={clang_runtime}");
 
     if let Runtime::ObjFW(_) = &runtime {
         // Add compability headers to make `#include <objc/objc.h>` work.
@@ -221,8 +219,6 @@ fn main() {
         cc_args.push_str(compat_headers.to_str().unwrap());
     }
 
-    println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS
-
     if let Runtime::ObjFW(_) = &runtime {
         // Link to libobjfw-rt
         println!("cargo:rustc-link-lib=dylib=objfw-rt");
@@ -250,10 +246,18 @@ fn main() {
         let mut builder = cc::Build::new();
         builder.file("extern/exception.m");
 
+        // Compile with exceptions enabled and with the correct runtime, but
+        // _without ARC_!
         for flag in cc_args.split(' ') {
             builder.flag(flag);
         }
 
         builder.compile("librust_objc_sys_0_3_try_catch_exception.a");
     }
+
+    // Add this to the `CC` args _after_ we've omitted it when compiling
+    // `extern/exception.m`.
+    cc_args.push_str(" -fobjc-arc -fobjc-arc-exceptions");
+
+    println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS
 }

crates/objc-sys/extern/exception.m

@@ -12,14 +12,11 @@
 unsigned char rust_objc_sys_0_3_try_catch_exception(void (*f)(void *), void *context, id *error) {
     @try {
         f(context);
-        if (error) {
-            *error = (id)0; // nil
-        }
         return 0;
     } @catch (id exception) {
-        if (error) {
-            *error = objc_retain(exception);
-        }
+        // The exception is retained while inside the `@catch` block, but is
+        // not guaranteed to be so outside of it; so hence we must do it here!
+        *error = objc_retain(exception);
         return 1;
     }
 }

crates/objc2/CHANGELOG.md

@@ -68,6 +68,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 * Fixed using autorelease pools on 32bit macOS and older macOS versions.
+* Fixed memory leaks in and improved performance of `exception::catch`.
 
 ### Removed
 * **BREAKING**: Removed `rc::SliceId`, since it is trivially implementable

crates/objc2/src/exception.rs

@@ -237,6 +237,9 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Id<Exception>
         // SAFETY:
         // The exception is always a valid object or NULL.
         //
+        // Since we do a retain inside `extern/exception.m`, the object has
+        // +1 retain count.
+        //
         // Code throwing an exception know that they don't hold sole access to
         // that object any more, so even if the type was originally mutable,
         // it is okay to create a new `Id` to it here.

crates/tests/src/exception.rs

@@ -22,29 +22,39 @@ fn throw_catch_raise_catch() {
     let reason = NSString::from_str("def");
 
     let exc = NSException::new(&name, Some(&reason), None).unwrap();
-    // TODO: Investigate why this is required on GNUStep!
-    let _exc2 = exc.clone();
 
-    assert_retain_count(&exc, 2);
-
-    // TODO: Investigate this!
-    let extra_retain = usize::from(cfg!(all(
-        feature = "apple",
-        target_os = "macos",
-        target_arch = "x86"
-    )));
+    assert_retain_count(&exc, 1);
 
     let exc = autoreleasepool(|_| {
         let exc = NSException::into_exception(exc);
         let res = unsafe { catch(|| throw(exc)) };
         let exc = res.unwrap_err().unwrap();
         let exc = NSException::from_exception(exc).unwrap();
 
-        assert_retain_count(&exc, 3 + extra_retain);
+        assert_retain_count(&exc, 1);
+        exc
+    });
+
+    assert_retain_count(&exc, 1);
+
+    let exc = autoreleasepool(|_| {
+        let inner = || {
+            autoreleasepool(|pool| {
+                let exc = Id::autorelease(exc, pool);
+                unsafe { exc.raise() }
+            })
+        };
+
+        let res = unsafe { catch(inner) };
+        let exc = NSException::from_exception(res.unwrap_err().unwrap()).unwrap();
+
+        // Undesired: The inner pool _should_ have been drained on unwind, but
+        // it isn't, see `rc::Pool::drain`.
+        assert_retain_count(&exc, 2);
         exc
     });
 
-    assert_retain_count(&exc, 2 + extra_retain);
+    assert_retain_count(&exc, 1);
 
     assert_eq!(exc.name(), name);
     assert_eq!(exc.reason().unwrap(), reason);
@@ -85,28 +95,26 @@ fn raise_catch() {
     let exc = NSException::new(&name, Some(&reason), None).unwrap();
     assert_retain_count(&exc, 1);
 
-    let res = autoreleasepool(|_| {
-        let res = unsafe {
-            catch(|| {
-                if exc.name() == name {
-                    exc.raise();
-                } else {
-                    4
-                }
-            })
+    let exc = autoreleasepool(|pool| {
+        let exc = Id::autorelease(exc, pool);
+        let inner = || {
+            if exc.name() == name {
+                unsafe { exc.raise() };
+            } else {
+                42
+            }
         };
-        assert_retain_count(&exc, 3);
+        let res = unsafe { catch(inner) }.unwrap_err().unwrap();
+        assert_retain_count(&exc, 2);
         res
     });
 
-    assert_retain_count(&exc, 2);
-
-    let obj = res.unwrap_err().unwrap();
+    assert_retain_count(&exc, 1);
 
-    assert_eq!(format!("{obj}"), "def");
+    assert_eq!(format!("{exc}"), "def");
     assert_eq!(
-        format!("{obj:?}"),
-        format!("exception <NSException: {:p}> 'abc' reason:def", &*obj)
+        format!("{exc:?}"),
+        format!("exception <NSException: {:p}> 'abc' reason:def", &*exc)
     );
 }