From 79dfae24648d5dcb5d5841baa966b798bb554fb5 Mon Sep 17 00:00:00 2001 From: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com> Date: Mon, 15 Dec 2025 18:37:27 +0100 Subject: [PATCH] gpui: Fix some memory leaks on macOS platform (#44639) While profiling with instruments, I discovered that some of the strings allocated on the mac platform are never released, and the profiler marks them as leaks image Release Notes: - N/A --------- Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com> Co-authored-by: Anthony Eid --- clippy.toml | 1 + crates/fs/src/fs.rs | 2 ++ crates/gpui/src/platform/mac.rs | 2 ++ .../src/platform/mac/attributed_string.rs | 34 ++++++++++++------- crates/gpui/src/platform/mac/display.rs | 7 ++-- crates/gpui/src/platform/mac/platform.rs | 12 +++---- .../gpui/src/platform/mac/screen_capture.rs | 5 +-- crates/gpui/src/platform/mac/window.rs | 22 ++++++------ 8 files changed, 49 insertions(+), 36 deletions(-) diff --git a/clippy.toml b/clippy.toml index 0ce7a6cd68d4e8210788eb7a67aa06c742cc8274..9dd246074a06c4db7b66eff7a83ef68e3612c378 100644 --- a/clippy.toml +++ b/clippy.toml @@ -14,6 +14,7 @@ disallowed-methods = [ { path = "std::process::Command::stderr", reason = "`smol::process::Command::from()` does not preserve stdio configuration", replacement = "smol::process::Command::stderr" }, { path = "serde_json::from_reader", reason = "Parsing from a buffer is much slower than first reading the buffer into a Vec/String, see https://github.com/serde-rs/json/issues/160#issuecomment-253446892. Use `serde_json::from_slice` instead." }, { path = "serde_json_lenient::from_reader", reason = "Parsing from a buffer is much slower than first reading the buffer into a Vec/String, see https://github.com/serde-rs/json/issues/160#issuecomment-253446892, Use `serde_json_lenient::from_slice` instead." }, + { path = "cocoa::foundation::NSString::alloc", reason = "NSString must be autoreleased to avoid memory leaks. Use `ns_string()` helper instead." }, ] disallowed-types = [ # { path = "std::collections::HashMap", replacement = "collections::HashMap" }, diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index e8357e359696bfcfbc7cfd829f84222c1303402a..e6f69a14593a0246ae8ccb4aa4673f4e1f5a1e8e 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -641,6 +641,8 @@ impl Fs for RealFs { use objc::{class, msg_send, sel, sel_impl}; unsafe { + /// Allow NSString::alloc use here because it sets autorelease + #[allow(clippy::disallowed_methods)] unsafe fn ns_string(string: &str) -> id { unsafe { NSString::alloc(nil).init_str(string).autorelease() } } diff --git a/crates/gpui/src/platform/mac.rs b/crates/gpui/src/platform/mac.rs index 76d636b457517da64cf66988325652ddea56c5d3..aa056846e6bc56e53d95c41a44444dbb89a16237 100644 --- a/crates/gpui/src/platform/mac.rs +++ b/crates/gpui/src/platform/mac.rs @@ -135,6 +135,8 @@ unsafe impl objc::Encode for NSRange { } } +/// Allow NSString::alloc use here because it sets autorelease +#[allow(clippy::disallowed_methods)] unsafe fn ns_string(string: &str) -> id { unsafe { NSString::alloc(nil).init_str(string).autorelease() } } diff --git a/crates/gpui/src/platform/mac/attributed_string.rs b/crates/gpui/src/platform/mac/attributed_string.rs index 5f313ac699d6e1a096c4bcf807fd6c080d0064da..42fe1e5bf7a396a4eaa8ade26977a207d43b49b5 100644 --- a/crates/gpui/src/platform/mac/attributed_string.rs +++ b/crates/gpui/src/platform/mac/attributed_string.rs @@ -50,10 +50,12 @@ impl NSMutableAttributedString for id {} #[cfg(test)] mod tests { + use crate::platform::mac::ns_string; + use super::*; use cocoa::appkit::NSImage; use cocoa::base::nil; - use cocoa::foundation::NSString; + use cocoa::foundation::NSAutoreleasePool; #[test] #[ignore] // This was SIGSEGV-ing on CI but not locally; need to investigate https://github.com/zed-industries/zed/actions/runs/10362363230/job/28684225486?pr=15782#step:4:1348 fn test_nsattributed_string() { @@ -68,26 +70,34 @@ mod tests { impl NSTextAttachment for id {} unsafe { - let image: id = msg_send![class!(NSImage), alloc]; - image.initWithContentsOfFile_(NSString::alloc(nil).init_str("test.jpeg")); + let image: id = { + let img: id = msg_send![class!(NSImage), alloc]; + let img: id = msg_send![img, initWithContentsOfFile: ns_string("test.jpeg")]; + let img: id = msg_send![img, autorelease]; + img + }; let _size = image.size(); - let string = NSString::alloc(nil).init_str("Test String"); - let attr_string = NSMutableAttributedString::alloc(nil).init_attributed_string(string); - let hello_string = NSString::alloc(nil).init_str("Hello World"); - let hello_attr_string = - NSAttributedString::alloc(nil).init_attributed_string(hello_string); + let string = ns_string("Test String"); + let attr_string = NSMutableAttributedString::alloc(nil) + .init_attributed_string(string) + .autorelease(); + let hello_string = ns_string("Hello World"); + let hello_attr_string = NSAttributedString::alloc(nil) + .init_attributed_string(hello_string) + .autorelease(); attr_string.appendAttributedString_(hello_attr_string); - let attachment = NSTextAttachment::alloc(nil); + let attachment: id = msg_send![NSTextAttachment::alloc(nil), autorelease]; let _: () = msg_send![attachment, setImage: image]; let image_attr_string = msg_send![class!(NSAttributedString), attributedStringWithAttachment: attachment]; attr_string.appendAttributedString_(image_attr_string); - let another_string = NSString::alloc(nil).init_str("Another String"); - let another_attr_string = - NSAttributedString::alloc(nil).init_attributed_string(another_string); + let another_string = ns_string("Another String"); + let another_attr_string = NSAttributedString::alloc(nil) + .init_attributed_string(another_string) + .autorelease(); attr_string.appendAttributedString_(another_attr_string); let _len: cocoa::foundation::NSUInteger = msg_send![attr_string, length]; diff --git a/crates/gpui/src/platform/mac/display.rs b/crates/gpui/src/platform/mac/display.rs index fe5aaba8dbb9eab4db8c02f94aea1319c2b7535c..94791620e8a394f67a38c257c95c575398cee0b7 100644 --- a/crates/gpui/src/platform/mac/display.rs +++ b/crates/gpui/src/platform/mac/display.rs @@ -1,9 +1,10 @@ +use super::ns_string; use crate::{Bounds, DisplayId, Pixels, PlatformDisplay, point, px, size}; use anyhow::Result; use cocoa::{ appkit::NSScreen, base::{id, nil}, - foundation::{NSArray, NSDictionary, NSString}, + foundation::{NSArray, NSDictionary}, }; use core_foundation::uuid::{CFUUIDGetUUIDBytes, CFUUIDRef}; use core_graphics::display::{CGDirectDisplayID, CGDisplayBounds, CGGetActiveDisplayList}; @@ -35,7 +36,7 @@ impl MacDisplay { let screens = NSScreen::screens(nil); let screen = cocoa::foundation::NSArray::objectAtIndex(screens, 0); let device_description = NSScreen::deviceDescription(screen); - let screen_number_key: id = NSString::alloc(nil).init_str("NSScreenNumber"); + let screen_number_key: id = ns_string("NSScreenNumber"); let screen_number = device_description.objectForKey_(screen_number_key); let screen_number: CGDirectDisplayID = msg_send![screen_number, unsignedIntegerValue]; Self(screen_number) @@ -150,7 +151,7 @@ impl MacDisplay { unsafe fn get_nsscreen(&self) -> id { let screens = unsafe { NSScreen::screens(nil) }; let count = unsafe { NSArray::count(screens) }; - let screen_number_key: id = unsafe { NSString::alloc(nil).init_str("NSScreenNumber") }; + let screen_number_key: id = unsafe { ns_string("NSScreenNumber") }; for i in 0..count { let screen = unsafe { NSArray::objectAtIndex(screens, i) }; diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index c2363afe270f973513c8ba696bf5d3f99fb92cad..ee67f465e34bd8109246f68b311e225aa8f9fd0a 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/crates/gpui/src/platform/mac/platform.rs @@ -2,7 +2,7 @@ use super::{ BoolExt, MacKeyboardLayout, MacKeyboardMapper, attributed_string::{NSAttributedString, NSMutableAttributedString}, events::key_to_native, - renderer, + ns_string, renderer, }; use crate::{ Action, AnyWindowHandle, BackgroundExecutor, ClipboardEntry, ClipboardItem, ClipboardString, @@ -1061,13 +1061,15 @@ impl Platform for MacPlatform { let attributed_string = { let mut buf = NSMutableAttributedString::alloc(nil) // TODO can we skip this? Or at least part of it? - .init_attributed_string(NSString::alloc(nil).init_str("")); + .init_attributed_string(ns_string("")) + .autorelease(); for entry in item.entries { if let ClipboardEntry::String(ClipboardString { text, metadata: _ }) = entry { let to_append = NSAttributedString::alloc(nil) - .init_attributed_string(NSString::alloc(nil).init_str(&text)); + .init_attributed_string(ns_string(&text)) + .autorelease(); buf.appendAttributedString_(to_append); } @@ -1543,10 +1545,6 @@ extern "C" fn handle_dock_menu(this: &mut Object, _: Sel, _: id) -> id { } } -unsafe fn ns_string(string: &str) -> id { - unsafe { NSString::alloc(nil).init_str(string).autorelease() } -} - unsafe fn ns_url_to_path(url: id) -> Result { let path: *mut c_char = msg_send![url, fileSystemRepresentation]; anyhow::ensure!(!path.is_null(), "url is not a file path: {}", unsafe { diff --git a/crates/gpui/src/platform/mac/screen_capture.rs b/crates/gpui/src/platform/mac/screen_capture.rs index 4d4ffa6896520e465dfeb7b1ccc06e1149f9e25d..2f2c1eae335c8bcb366879661534c46dacfd47b4 100644 --- a/crates/gpui/src/platform/mac/screen_capture.rs +++ b/crates/gpui/src/platform/mac/screen_capture.rs @@ -1,3 +1,4 @@ +use super::ns_string; use crate::{ DevicePixels, ForegroundExecutor, SharedString, SourceMetadata, platform::{ScreenCaptureFrame, ScreenCaptureSource, ScreenCaptureStream}, @@ -7,7 +8,7 @@ use anyhow::{Result, anyhow}; use block::ConcreteBlock; use cocoa::{ base::{YES, id, nil}, - foundation::{NSArray, NSString}, + foundation::NSArray, }; use collections::HashMap; use core_foundation::base::TCFType; @@ -195,7 +196,7 @@ unsafe fn screen_id_to_human_label() -> HashMap { let screens: id = msg_send![class!(NSScreen), screens]; let count: usize = msg_send![screens, count]; let mut map = HashMap::default(); - let screen_number_key = unsafe { NSString::alloc(nil).init_str("NSScreenNumber") }; + let screen_number_key = unsafe { ns_string("NSScreenNumber") }; for i in 0..count { let screen: id = msg_send![screens, objectAtIndex: i]; let device_desc: id = msg_send![screen, deviceDescription]; diff --git a/crates/gpui/src/platform/mac/window.rs b/crates/gpui/src/platform/mac/window.rs index 53207fb77d16f2e1956f6914889b29ae3ea7bb35..19ad1777570da9494148e01161e156748cd9bcfc 100644 --- a/crates/gpui/src/platform/mac/window.rs +++ b/crates/gpui/src/platform/mac/window.rs @@ -785,7 +785,7 @@ impl MacWindow { native_window.setAcceptsMouseMovedEvents_(YES); if let Some(tabbing_identifier) = tabbing_identifier { - let tabbing_id = NSString::alloc(nil).init_str(tabbing_identifier.as_str()); + let tabbing_id = ns_string(tabbing_identifier.as_str()); let _: () = msg_send![native_window, setTabbingIdentifier: tabbing_id]; } else { let _: () = msg_send![native_window, setTabbingIdentifier:nil]; @@ -908,8 +908,8 @@ impl MacWindow { pub fn get_user_tabbing_preference() -> Option { unsafe { let defaults: id = NSUserDefaults::standardUserDefaults(); - let domain = NSString::alloc(nil).init_str("NSGlobalDomain"); - let key = NSString::alloc(nil).init_str("AppleWindowTabbingMode"); + let domain = ns_string("NSGlobalDomain"); + let key = ns_string("AppleWindowTabbingMode"); let dict: id = msg_send![defaults, persistentDomainForName: domain]; let value: id = if !dict.is_null() { @@ -1037,7 +1037,7 @@ impl PlatformWindow for MacWindow { } if let Some(tabbing_identifier) = tabbing_identifier { - let tabbing_id = NSString::alloc(nil).init_str(tabbing_identifier.as_str()); + let tabbing_id = ns_string(tabbing_identifier.as_str()); let _: () = msg_send![native_window, setTabbingIdentifier: tabbing_id]; } else { let _: () = msg_send![native_window, setTabbingIdentifier:nil]; @@ -1063,10 +1063,8 @@ impl PlatformWindow for MacWindow { return None; } let device_description: id = msg_send![screen, deviceDescription]; - let screen_number: id = NSDictionary::valueForKey_( - device_description, - NSString::alloc(nil).init_str("NSScreenNumber"), - ); + let screen_number: id = + NSDictionary::valueForKey_(device_description, ns_string("NSScreenNumber")); let screen_number: u32 = msg_send![screen_number, unsignedIntValue]; @@ -1509,8 +1507,8 @@ impl PlatformWindow for MacWindow { .spawn(async move { unsafe { let defaults: id = NSUserDefaults::standardUserDefaults(); - let domain = NSString::alloc(nil).init_str("NSGlobalDomain"); - let key = NSString::alloc(nil).init_str("AppleActionOnDoubleClick"); + let domain = ns_string("NSGlobalDomain"); + let key = ns_string("AppleActionOnDoubleClick"); let dict: id = msg_send![defaults, persistentDomainForName: domain]; let action: id = if !dict.is_null() { @@ -2512,7 +2510,7 @@ where unsafe fn display_id_for_screen(screen: id) -> CGDirectDisplayID { unsafe { let device_description = NSScreen::deviceDescription(screen); - let screen_number_key: id = NSString::alloc(nil).init_str("NSScreenNumber"); + let screen_number_key: id = ns_string("NSScreenNumber"); let screen_number = device_description.objectForKey_(screen_number_key); let screen_number: NSUInteger = msg_send![screen_number, unsignedIntegerValue]; screen_number as CGDirectDisplayID @@ -2558,7 +2556,7 @@ unsafe fn remove_layer_background(layer: id) { // `description` reflects its name and some parameters. Currently `NSVisualEffectView` // uses a `CAFilter` named "colorSaturate". If one day they switch to `CIFilter`, the // `description` will still contain "Saturat" ("... inputSaturation = ..."). - let test_string: id = NSString::alloc(nil).init_str("Saturat").autorelease(); + let test_string: id = ns_string("Saturat"); let count = NSArray::count(filters); for i in 0..count { let description: id = msg_send![filters.objectAtIndex(i), description];