Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How do you not cause memory leaks? #117

Closed
selfconfigio opened this issue Oct 25, 2022 · 10 comments
Closed

How do you not cause memory leaks? #117

selfconfigio opened this issue Oct 25, 2022 · 10 comments

Comments

@selfconfigio
Copy link

selfconfigio commented Oct 25, 2022

I'm using the objc crate to get the icon/image of a macos .app file.

But my following snippet leaks memory. I am fairly certain it is this snippet because if I replace the snippet with a String of b64 of the same image, no memory leak occurs.

Does this snippet use the objc crate in a wrong way? I suspect I'm not seeing something obvious in the docs.
How can I prevent this snippet from causing memory leaks?

fn get_icon_for_app_file(path: &str) -> String {
  // Get a class
  let ns_workspace_class = objc::class!(NSWorkspace); // NSObject
  // Allocate an instance
  let ns_workspace_obj = unsafe {
      let obj: *mut Object = objc::msg_send![ns_workspace_class, alloc];
      let obj: *mut Object = objc::msg_send![obj, init];
      StrongPtr::new(obj)
  };
  let path = unsafe { NSString::alloc(nil).init_str(path) };

  let ns_image: Id<Object> = unsafe {
      let img = msg_send!(*ns_workspace_obj, iconForFile:path);
      img
  };
  // let _: () = unsafe { msg_send![*ns_workspace_obj, release] };
  // drop(ns_workspace_obj);

  let foundation_ns_data: Id<Object> = unsafe {
      msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90)
  };

  let ns_bitmap_image_rep: Id<Object> = unsafe {
      msg_send!(class!(NSBitmapImageRep), imageRepWithData: ns_data)
  };

  let foundation_data: Id<Object> = unsafe {
      msg_send!(ns_bitmap_image_rep, representationUsingType:3 properties:nil)
  };

  let b64 = unsafe {
      msg_send!(data, base64EncodedStringWithOptions:nil)
  };

  let c_buf: *const c_char = unsafe {
      // GITHUB COMMENT EDIT: renamed from about_item_prefix
      let init_string = NSString::alloc(nil).init_str("");
      // GITHUB COMMENT EDIT: renamed from about_item_title
      let b64_string = init_string.stringByAppendingString_(b64);
      let c_pointer: *const libc::c_char = b64_string.UTF8String();
      c_pointer
  };

  let c_str: &CStr = unsafe { CStr::from_ptr(c_buf) };
  let str_slice: &str = c_str.to_str().unwrap();

  let mut img = String::from("data:image/jpeg;base64,");
  img.push_str(str_slice);

  println!("Final image data is: {}", img);
  Ok(img)
}

I've tried using various expressions of let _: () = unsafe { msg_send![some_object, release] }; to release memory, but I get segmentation faults and rarely bus faults (for context this snippet is run in multithreaded env, perhaps contributing). Should the snippet be using release, and wrapping this snippet in something like a mutex so it can only be run by 1 thread at a time?

@madsmtm
Copy link

madsmtm commented Oct 25, 2022

Returning an Id directly from msg_send! is not valid, instead you need to use Id::from_ptr or Id::from_retained_ptr (depending on the selector, see the ARC documentation on method families).

Also, it's quite confusing that you're using both StrongPtr and Id; pick one, and stay with it.

To answer your specific question, I think you're missing a release af b64 and about_item_title.

Shameless plug: In objc2, I've created the msg_send_id! macro specifically to make dealing with this problem easier.

@selfconfigio
Copy link
Author

Thanks so much for your help @madsmtm

Returning an Id directly from msg_send! is not valid, instead you need to use Id::from_ptr or Id::from_retained_ptr (depending on the selector, see the ARC documentation on method families).

Do you mean most of the msg_send! e.g. msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90) should be wrapped with Id::from_ptr(msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90)) since their definitions have init methods, and not new methods?

think you're missing a release at b64 and about_item_title.
How should these be released exactly?

let b64: Id<Object> = unsafe {
    // Id::from_retained_ptr(msg_send!(data, base64EncodedStringWithOptions:nil))
    Id::from_ptr(msg_send!(data, base64EncodedStringWithOptions:nil))
};

let c_buf: *const c_char = unsafe {
    // GITHUB COMMENT EDIT: renamed from about_item_prefix
    let init_string = NSString::alloc(nil).init_str("");
    // GITHUB COMMENT EDIT: renamed from about_item_title
    let b64_string = init_string.stringByAppendingString_(b64);
    let c_pointer: *const libc::c_char = b64_string.UTF8String();
    // Is this correct release?
    std::mem::drop(b64_string); 
    c_pointer
};
// Is this correct release?
let _: () = unsafe { msg_send!(b64, release) };

// https://github.com/madsmtm/objc2/blob/master/objc2/examples/nspasteboard.rs
// Maybe something like unsafe { ManuallyDrop::new([Id::new(cls).unwrap()]) }

let c_str: &CStr = unsafe { CStr::from_ptr(c_buf) };
let str_slice: &str = c_str.to_str().unwrap();

@selfconfigio
Copy link
Author

selfconfigio commented Oct 26, 2022

I pinpointed this is the first expression (the others following it also likely memory leak as well) causes a memory leak, but I wasn't able to figure how to stop it from occurring

let ns_image: Id<Object> = unsafe {
    let img = msg_send!(*ns_workspace_obj, iconForFile:path);
    img
};

Tried following with no success
1)

let foundation_ns_data: Id<Object> = unsafe {
      Id::from_ptr(msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90))
};
let foundation_ns_data: Id<Object> = unsafe {
      Id::from_retained_ptr(msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90))
};
let foundation_ns_data: StrongPtr = unsafe {
      StrongPtr::new(msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90))
};

With the following attempts on all 3 variants to stop the memory leak, no success
a)

std::mem::drop(foundation_ns_data);

b)

 // let _: () = unsafe { msg_send!(, release) };

c)

let autorelease_pool: *mut Object = msg_send![class!(NSAutoreleasePool), new];

let foundation_ns_data: Id<Object> = unsafe {
      Id::from_ptr(msg_send!(ns_image, TIFFRepresentationUsingCompression:6 factor:90))
};

let _: () = msg_send![autorelease_pool, release];

@madsmtm
Copy link

madsmtm commented Oct 26, 2022

I think the last code you wrote, option c), should be correct: -[NSImage TIFFRepresentationUsingCompression:factor:] returns an autoreleased instance, which Id::from_ptr retains, and when dropped, releases again.

Since you've wrapped everything in an autorelease pool, the object's retain count should drop fully to zero once the autorelease pool drains.

So just to be clear, I think your code should look something like:

objc::rc::autoreleasepool(|| {
    let ns_workspace_obj = unsafe {
        let obj: *mut Object = msg_send![class!(NSWorkspace), alloc];
        let obj: *mut Object = msg_send![obj, init];
        Id::from_retained_ptr(obj) // `init` returns a retained instance
    };

    let path: Id<Object> = unsafe {
        Id::from_retained_ptr(NSString::alloc(nil).init_str(path)) // `init` returns a retained instance
    };
    let ns_image: Id<Object> = unsafe {
        Id::from_ptr(msg_send![ns_workspace_obj, iconForFile: &*path]) // `iconForFile` returns an autoreleased instance
    };

    let foundation_ns_data: Id<Object> = unsafe {
        // `TIFFRepresentationUsingCompression:factor:` returns an autoreleased instance
        Id::from_ptr(msg_send![ns_image, TIFFRepresentationUsingCompression: 6usize factor: 90.0f32])
    };

    // ...
})

(Here I've also fixed things like using the correct types in TIFFRepresentationUsingCompression:factor:)

@selfconfigio
Copy link
Author

selfconfigio commented Oct 26, 2022

Wow @madsmtm afaics this seems to completely fix the leak.
Thanks so much for spelling it out with the code sample. Seeing a code sample/example like this makes it so clear, especially for a novice to Rust and ObjC/MacOS/iOS APIs, this code sample definitely feels like the easiest/most basic way to accomplish use cases.

I think the last code you wrote, option c), should be correct

I had not been wrapping the code in autoreleasepool

Without this sample I had honestly resorted to a new cursed option 4) where I created a rust bin/executable to wrap the snippet in, which was freeing memory each time the executable finishes/ends 😅

@madsmtm
Copy link

madsmtm commented Oct 26, 2022

No problem!

As a teaser, in madsmtm/objc2#264 I'm building a new crate which has automatically generated bindings, and automatic memory management, allowing you to just do:

use objc2::rc::autoreleasepool;
use icrate::Foundation::NSString;
use icrate::AppKit::NSWorkspace;

autoreleasepool(|| {
    let ns_workspace_obj = unsafe { NSWorkspace::new() };
    let path = NSString::from_str(path);
    let ns_image = unsafe { ns_workspace_obj.iconForFile(&path) };
    let foundation_ns_data = unsafe { ns_image.TIFFRepresentationUsingCompression_factor(6, 90.0) };
    // ...
})

@complexspaces
Copy link

Hey there, @madsmtm. I've been reading through this issue over the last couple days and had a question if you don't mind.

Are there headers somewhere documenting that specific methods return objects that are autoreleased (and hence require some autorelease pool), is this something you determined via a side-channel, or are you working off a broad interpretation of the autorelease documentation which notes that all detached or POSIX threads must handle their own pools if interacting with Cocoa APIs?

@madsmtm
Copy link

madsmtm commented Aug 3, 2023

It's all documented in https://clang.llvm.org/docs/AutomaticReferenceCounting.html.

In short, whether something returns an autoreleased object vs. an owned object depends on the method name.

I can elaborate if you want, but I suspect that link was what you were missing ;)

@complexspaces
Copy link

I went through a mostly-complete read of that document (skimmed over some seemingly-unrelated parts) but I didn't see anything obvious about method names and release semantics. The only thing that looked like it may be it was Related result types. Is that what you were referencing as well? Either way, I would appreciate the elaboration if the rest of my analysis is off.

  • outside of ARC, the instance methods retain and autorelease

If the formal result type of such a method is id or protocol-qualified id, or a type equal to the declaring class or a superclass, then it is said to have a related result type. In this case, when invoked in an explicit message send, it is assumed to return a type related to the type of the receiver:

Assuming this is the right part, does this mean that since Rust code is not participating in ARC most msg_send! calls that return an object are autoreleased? That sounds a bit off, but I'm not confident in my reading of this either.

@SSheldon
Copy link
Owner

SSheldon commented Aug 4, 2023

He's referring to the section on method families: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-method-families

Methods in the alloc, copy, mutableCopy, and new families... implicitly return a retained object

aka those methods return objects with a +1 retain count, meaning you need not retain them, and you are responsible for releasing them. All other methods return objects with a +0 retain count (potentially meaning they have been autoreleased), so if you want to ensure they stick around, you need to retain them yourself (and therefore also release them when you are done with them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants