Remove leaks from macOS clipboard getters

This additioanlly refactors `image()` to be simpler by taking advantage of the fact that we only want one image. Additionally, this codepath avoids the system-level caching of image data that can help reduce peak usage. We have applied an autorelease pool to the Obj-C code as well, as there appear to be a lot of autoreleased values used by AppKit.framework Finally, it applies a similar refactoring to `text()` for simplicity and removal of memory leaks due to incorrect object retaining assumptions. Co-authored-by: taojiacheng <[email protected]>
1Password · Aug 24, 2023 · 921698e · 921698e
1 parent f409d08
commit 921698e
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 66 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -11,7 +11,7 @@ edition = "2018"
 
 [features]
 default = ["image-data"]
-image-data = ["core-graphics", "image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"]
+image-data = ["core-graphics", "once_cell", "image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"]
 wayland-data-control = ["wl-clipboard-rs"]
 
 [dependencies]
@@ -34,7 +34,7 @@ log = "0.4"
 objc = "0.2"
 objc_id = "0.1"
 objc-foundation = "0.1"
-once_cell = "1"
+once_cell = { version = "1", optional = true }
 core-graphics = { version = "0.22", optional = true }
 image = { version = "0.24", optional = true, default-features = false, features = ["tiff"] }
 

diff --git a/src/platform/osx.rs b/src/platform/osx.rs
@@ -20,22 +20,25 @@ use core_graphics::{
 };
 use objc::{
 	msg_send,
+	rc::autoreleasepool,
 	runtime::{Class, Object},
 	sel, sel_impl,
 };
-use objc_foundation::{INSArray, INSObject, INSString, NSArray, NSDictionary, NSObject, NSString};
+use objc_foundation::{INSArray, INSFastEnumeration, INSString, NSArray, NSObject, NSString};
 use objc_id::{Id, Owned};
+#[cfg(feature = "image-data")]
 use once_cell::sync::Lazy;
-use std::borrow::Cow;
+use std::{borrow::Cow, ptr::NonNull};
 
 // Required to bring NSPasteboard into the path of the class-resolver
 #[link(name = "AppKit", kind = "framework")]
 extern "C" {
 	static NSPasteboardTypeHTML: *const Object;
 	static NSPasteboardTypeString: *const Object;
+	#[cfg(feature = "image-data")]
+	static NSPasteboardTypeTIFF: *const Object;
 }
 
-static NSSTRING_CLASS: Lazy<&Class> = Lazy::new(|| Class::get("NSString").unwrap());
 #[cfg(feature = "image-data")]
 static NSIMAGE_CLASS: Lazy<&Class> = Lazy::new(|| Class::get("NSImage").unwrap());
 
@@ -181,73 +184,72 @@ impl<'clipboard> Get<'clipboard> {
 	}
 
 	pub(crate) fn text(self) -> Result<String, Error> {
-		let string_class = object_class(&NSSTRING_CLASS);
-		let classes: Id<NSArray<NSObject, Owned>> = NSArray::from_vec(vec![string_class]);
-		let options: Id<NSDictionary<NSObject, NSObject>> = NSDictionary::new();
-
-		let string_array: Id<NSArray<NSString>> = unsafe {
-			let obj: *mut NSArray<NSString> =
-				msg_send![self.pasteboard, readObjectsForClasses:&*classes options:&*options];
-
-			if obj.is_null() {
-				return Err(Error::ContentNotAvailable);
-			} else {
-				Id::from_ptr(obj)
+		// XXX: There does not appear to be an alternative for obtaining text without the need for
+		// autorelease behavior.
+		autoreleasepool(|| {
+			// XXX: We explicitly use `pasteboardItems` and not `stringForType` since the latter will concat
+			// multiple strings, if present, into one and return it instead of reading just the first which is `arboard`'s
+			// historical behavior.
+			let contents: Option<NonNull<NSArray<NSObject>>> =
+				unsafe { msg_send![self.pasteboard, pasteboardItems] };
+
+			let contents = contents.map(|c| unsafe { c.as_ref() }).ok_or_else(|| {
+				Error::Unknown { description: String::from("NSPasteboard#pasteboardItems errored") }
+			})?;
+
+			for item in contents.enumerator() {
+				let maybe_str: Option<NonNull<NSString>> =
+					unsafe { msg_send![item, stringForType:NSPasteboardTypeString] };
+
+				match maybe_str {
+					Some(string) => {
+						let string: Id<NSString, Owned> = unsafe { Id::from_ptr(string.as_ptr()) };
+						return Ok(string.as_str().to_owned());
+					}
+					None => continue,
+				}
 			}
-		};
 
-		string_array
-			.first_object()
-			.map(|obj| obj.as_str().to_owned())
-			.ok_or(Error::ContentNotAvailable)
+			Err(Error::ContentNotAvailable)
+		})
 	}
 
 	#[cfg(feature = "image-data")]
 	pub(crate) fn image(self) -> Result<ImageData<'static>, Error> {
+		use objc_foundation::NSData;
 		use std::io::Cursor;
 
-		let image_class: Id<NSObject> = object_class(&NSIMAGE_CLASS);
-		let classes = vec![image_class];
-		let classes: Id<NSArray<NSObject, Owned>> = NSArray::from_vec(classes);
-		let options: Id<NSDictionary<NSObject, NSObject>> = NSDictionary::new();
-
-		let contents: Id<NSArray<NSObject>> = unsafe {
-			let obj: *mut NSArray<NSObject> =
-				msg_send![self.pasteboard, readObjectsForClasses:&*classes options:&*options];
+		// XXX: There does not appear to be an alternative for obtaining images without the need for
+		// autorelease behavior.
+		let image = autoreleasepool(|| {
+			let obj: Option<NonNull<NSData>> =
+				unsafe { msg_send![self.pasteboard, dataForType: NSPasteboardTypeTIFF] };
 
-			if obj.is_null() {
-				return Err(Error::ContentNotAvailable);
+			let image_data: Id<NSData> = if let Some(obj) = obj {
+				unsafe { Id::from_ptr(obj.as_ptr()) }
 			} else {
-				Id::from_ptr(obj)
-			}
-		};
+				return Err(Error::ContentNotAvailable);
+			};
 
-		let obj = match contents.first_object() {
-			Some(obj) if obj.is_kind_of(&NSIMAGE_CLASS) => obj,
-			Some(_) | None => return Err(Error::ContentNotAvailable),
-		};
+			let data = unsafe {
+				let len: usize = msg_send![&*image_data, length];
+				let bytes: *const u8 = msg_send![&*image_data, bytes];
 
-		let tiff: &NSArray<NSObject> = unsafe { msg_send![obj, TIFFRepresentation] };
-		let data = unsafe {
-			let len: usize = msg_send![tiff, length];
-			let bytes: *const u8 = msg_send![tiff, bytes];
+				Cursor::new(std::slice::from_raw_parts(bytes, len))
+			};
 
-			Cursor::new(std::slice::from_raw_parts(bytes, len))
-		};
-		let reader = image::io::Reader::with_format(data, image::ImageFormat::Tiff);
-		match reader.decode() {
-			Ok(img) => {
-				let rgba = img.into_rgba8();
-				let (width, height) = rgba.dimensions();
-
-				Ok(ImageData {
-					width: width as usize,
-					height: height as usize,
-					bytes: rgba.into_raw().into(),
-				})
-			}
-			Err(_) => Err(Error::ConversionFailure),
-		}
+			let reader = image::io::Reader::with_format(data, image::ImageFormat::Tiff);
+			reader.decode().map_err(|_| Error::ConversionFailure)
+		})?;
+
+		let rgba = image.into_rgba8();
+		let (width, height) = rgba.dimensions();
+
+		Ok(ImageData {
+			width: width as usize,
+			height: height as usize,
+			bytes: rgba.into_raw().into(),
+		})
 	}
 }
 
@@ -345,10 +347,3 @@ impl<'clipboard> Clear<'clipboard> {
 		Ok(())
 	}
 }
-
-/// Convenience function to get an Objective-C object from a
-/// specific class.
-fn object_class(class: &'static Class) -> Id<NSObject> {
-	// SAFETY: `Class` is a valid object and `Id` will not mutate it
-	unsafe { Id::from_ptr(class as *const Class as *mut NSObject) }
-}