fix mac os memory leak

[Lurk]

TLDR;
I did not find an easy way to clean up NSArray, but there is a better option to get NSString from pasteboard - stringForType.

Next up. Even if send release to NSString, there are leftovers after conversion NSString to Rust string. Right now, conversion happens in INSString.as_str(), which internally uses NSString.UTF8String. UTF8String, according to Apple docs, is a pointer lifetime of which is less or equal to lifetime NSString itself. This is not true if the clipboard contains characters outside the ASCII range. That's why nsstring_to_rust_string function.

I tried to describe the full process in a blogpost

[chrisduerr]

Please be more specific on where the memory leak is, how to reproduce it and why your changes are necessary to fix it.

[Lurk]

@chrisduerr Woah, that was fast responce :)

To reproduce memory leak on macOS

use copypasta::{ClipboardContext, ClipboardProvider};

fn main() {
    let mut ctx: ClipboardContext = ClipboardContext::new().unwrap();
    for _ in 0..20_000 {
        match ctx.get_contents() {
            Ok(str) => println!("{}", str.chars().count()),
            Err(_e) => println!("{:?}", _e),
        }
    }
}

cargo instruments -t Allocation

will produce a nice graph

The leak occurs in at least two places.
First, and I am not sure about the actual reason here, NSArray of NSString we got here does not get released. If we send release to NSArray nothing happens but, if we send the release to NSString we got here

string_array[0]

memory leak drops ~3x

Since we use NSArray to get the first NSString out of it, we can use NSPasteboard::stringForType which will return NSString.

Now we can send release directly to NSString we are getting from NSPastebord.

now If we run

cargo instruments -t Allocation

again, we will see this

Which is a huge 3x win but still.

Next leak, again I am not sure that I identified the source precisely, is that we use INSString.to_str() which internally uses NSString.UTF8String. Apple doc says

This C string is a pointer to a structure inside the string object, which may have a lifetime shorter than the string object and will certainly not have a longer lifetime. Therefore, you should copy the C string if it needs to be stored outside of the memory context in which you use this property.

And most of the time it is true, I mean this C string gets cleaned up, except when clipboard contains characters outside the ASCII range. In that case it lives forever.

Solution I come up with:

fn nsstring_to_rust_string(nsstring: *mut NSString) -> Result<String> {
    unsafe {
        let string_size: usize =
            msg_send![nsstring, lengthOfBytesUsingEncoding: NSUTF8StringEncoding];
        //we need +1 because getCString will return null terminated string
        let char_ptr = libc::malloc(string_size + 1);
        let res: bool = msg_send![nsstring, getCString:char_ptr  maxLength:string_size + 1 encoding:NSUTF8StringEncoding];
        if res {
            let c_string = CStr::from_ptr(char_ptr as *const i8);
            libc::free(char_ptr);
            Ok(c_string.to_string_lossy().into_owned())
        } else {
            libc::free(char_ptr);
            Err("Casting from NSString to Rust string has failed".into())
        }
    }
}

Now allocations is showing

I will gladly provide further explanations and/or fixes if needed.

[chrisduerr]

These seem like more fundamental issues with the objc crates. I find it difficult to believe that your solution is the 'correct' approach here. I try to stay away from macOS as much as possible, but surely it's possible to use these types without leaking.

I'd maybe try and look at some other usage of the macos crates in winit maybe. But manual allocation with libc and malloc should surely be unnecessary.

[Lurk]

Yeah, I discovered that leak in my first Rust project. Imagine how surprised I was :)

Anyway. I do not insist on this solution and will gladly use any other working solution. But every other clipboard crate I found in one way or another uses Avraham Weinstock implementation. And I looked hard, spent almost a month of evenings on it.

[chrisduerr]

Well if you're willing to look to further look into this, feel free. But the current solution does not seem appropriate to me.

[Lurk]

I am still trying to fix this because it annoys me as hell :)

Progress so far.

Found a way to get rid of libc.
Created repo with the reproducible example, fix, and benchmark (https://github.com/Lurk/nsstring_leak).
Created the issue in rust-objc-foundation (SSheldon/rust-objc-foundation#15)

I was kind of hoping that after the macOS update leak will go away, but no luck.

[Lurk]

Good news, everyone.

in the last commit the fix reduced to this:

    fn get_contents(&mut self) -> Result<String> {
        let nsstring: *mut NSString =
            unsafe { msg_send![self.pasteboard, stringForType: NSPasteboardTypeString] };
        if nsstring.is_null() {
            Err("pasteboard#stringForType returned null".into())
        } else {
            let nsstring: Id<NSString> = unsafe { Id::from_retained_ptr(nsstring) };
            Ok(autoreleasepool(|| nsstring.as_str().to_owned()))
        }
    }

• pasteboard allows to get string from clipboard so there is no need to get array

 let nsstring: *mut NSString = unsafe { msg_send![self.pasteboard, stringForType: NSPasteboardTypeString] };

• before we can wrap NSString we got from pasteboard to Id, we need to check if it is not null https://github.com/SSheldon/rust-objc-id/blob/master/src/id.rs#L47
• We need to wrap NSString to Id so it will be automatically released

let nsstring: Id<NSString> = unsafe { Id::from_retained_ptr(nsstring) };

• last but not least we need to wrap nsstring:as_str with autoreleasepool. The reason for that is described here: Make autoreleasepool take the pool as a parameter SSheldon/rust-objc#103 (comment)

Ok(autoreleasepool(|| nsstring.as_str().to_owned()))

I updated the tests and benchmarks here https://github.com/Lurk/nsstring_leak nothing is leaking and performance-wise it is fine.

[chrisduerr]

Shouldn't be any whitespace here.

[Lurk]

Should I also remove whitespace from line 22?

[Lurk]

removed

[chrisduerr]

No you shouldn't.

[chrisduerr]

The whitespace before crate imports was intentional. Same as the whitespace after std. This way the alphabetic ordering doesn't get in the way of the logical ordering of relevance.

Please revert the removal of whitespace before the crate::common import.

[Lurk]

Done

[chrisduerr]

Thanks for the PR.