fix(macos): release leaked window delegate Box and ObjC class on window close

[ManthanNimodiya]

Summary

setup() in apps/desktop/src-tauri/src/platform/macos/delegates.rs (used for traffic-light positioning on Editor, ScreenshotEditor, Settings, Upgrade and ModeSelect windows) leaked on every call:

1. A boxed WindowState (holding a live Window handle) was never freed.
2. A brand new Objective-C delegate class with a randomized name was registered every call and never disposed, along with its alloc'd instance.

Since closing these windows destroys and later rebuilds the underlying webview window, every open/close cycle permanently leaked a window handle plus a new ObjC class. This code has been in place since Nov 2024.

Changes

• Register the delegate class once (OnceLock) and reuse it for every window; only alloc a new instance per window.
• windowWillClose: now drops the boxed window state and releases the delegate instance's alloc reference.

Test plan

• cargo dev build compiles cleanly
• Dev app launches and runs without crashes

Greptile Summary

This PR fixes two memory leaks that accumulated on every open/close cycle of the traffic-light delegate windows: Box<WindowState<R>> was never freed because Box::into_raw was never balanced, and a new Objective-C class (with a randomized name) was registered on every setup() call and never disposed.

• The delegate class is now registered exactly once via std::sync::OnceLock and reused for every window, eliminating the per-call ObjC class leak.
• windowWillClose: now drops the boxed WindowState, nulls the ivar, restores the previous delegate, and calls release to balance the new allocation — all with careful ordering to avoid use-after-free on late callbacks.
• with_window_state now guards against the null app_box left behind after close, addressing the previously-flagged SIGSEGV path for enqueued post-close delegate callbacks.

Confidence Score: 5/5

The change correctly fixes two long-standing leaks and handles the cleanup ordering carefully; the one open question (class registration failure) affects only an edge-case startup path and leaves the rest of the delegate logic intact.

The core leak fixes are well-structured: the OnceLock class-registration correctly avoids duplicate registration across setup() calls, the windowWillClose: cleanup sequence (drop box → null ivar → restore delegate → release) is ordered to prevent dangling-reference callbacks, and the null guard in with_window_state closes the previously-flagged crash path. The only concern is a robustness gap in the OnceLock initializer: a panic there would poison the lock for the process lifetime, but this requires an ObjC class name collision that is very unlikely in practice and does not affect the correctness of the leak fix itself.

No files require special attention beyond the single changed file.

Important Files Changed

Filename

Overview

apps/desktop/src-tauri/src/platform/macos/delegates.rs

Fixes two memory leaks: Box<WindowState> is now properly dropped in windowWillClose:, and the delegate ObjC class is registered once via OnceLock instead of per-call. The app_box null guard addresses the previously-flagged SIGSEGV path for late callbacks. One robustness gap remains: if ClassDecl::new returns None the expect panic inside OnceLock::get_or_init poisons the lock, permanently breaking all subsequent setup() calls for the process lifetime.

Comments Outside Diff (1)

1. apps/desktop/src-tauri/src/platform/macos/delegates.rs, line 87-96 (link)

   with_window_state dereferences app_box without a null check

   on_window_will_close sets app_box to null (line 132) and then calls release, potentially freeing the delegate. If any subsequent delegate message (e.g. a windowDidResize: notification enqueued on the main thread before the window fully closed) fires after windowWillClose: returns, with_window_state will cast a null pointer to &mut WindowState<R> — a SIGSEGV that suppress_delegate_panic's catch_unwind will not catch. Adding a null guard here would prevent the crash without changing normal-path behavior.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/desktop/src-tauri/src/platform/macos/delegates.rs
   Line: 87-96
   
   Comment:
   **`with_window_state` dereferences `app_box` without a null check**
   
   `on_window_will_close` sets `app_box` to `null` (line 132) and then calls `release`, potentially freeing the delegate. If any subsequent delegate message (e.g. a `windowDidResize:` notification enqueued on the main thread before the window fully closed) fires after `windowWillClose:` returns, `with_window_state` will cast a null pointer to `&mut WindowState<R>` — a SIGSEGV that `suppress_delegate_panic`'s `catch_unwind` will not catch. Adding a null guard here would prevent the crash without changing normal-path behavior.
   
   How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (3): Last reviewed commit: "fix(macos): cargo fmt and make windowWil..." | Re-trigger Greptile}

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[tembo]

Nit: consider using new (or alloc + init) here. alloc without init is unusual for NSObject subclasses and can be a footgun.

Suggested change

	let delegate: id = msg_send![delegate_class, alloc];
	let delegate: id = msg_send![delegate_class, new];

[tembo]

One thought: since you release the delegate instance here, it may be safer to restore the previous delegate (or set nil) first, just in case the window emits any late delegate callbacks during teardown.

Suggested change

	// NSWindow does not retain its delegate, so the `alloc` reference taken
	// when this delegate was created is the only owning reference. Release
	// it now that the window is closing.
	let this_id = this as *const Object as id;
	let _: () = msg_send![this_id, release];
	// NSWindow does not retain its delegate, so the `alloc` reference taken
	// when this delegate was created is the only owning reference. Release
	// it now that the window is closing.
	let window: id = *this.get_ivar("window");
	let _: () = msg_send![window, setDelegate: super_del];
	let this_id = this as *const Object as id;
	let _: () = msg_send![this_id, release];

[tembo]

Heads up: since get_or_register_delegate_class is generic, the static CLASS is per-monomorphization, but the ObjC class name is always CapWindowDelegate. If setup() is ever called with a different R, this will try to re-register the same class name and panic.

Probably fine if there’s only ever one runtime type in practice, but if not, you may want a stable per-R class name (e.g., hash type_name::<R>()) or otherwise enforce a single R.

[greptile-apps]

Missing init after alloc

ObjC convention requires alloc to be followed by init before any instance methods (including ivar access) are used. For a plain NSObject subclass init is a no-op and alloc zeroes memory, so this works today, but it is technically undefined per the ObjC spec and may break with stricter runtimes or future changes.

Suggested change

	let delegate_class = get_or_register_delegate_class::<R>();
	let delegate: id = msg_send![delegate_class, alloc];
	(*delegate).set_ivar("window", ns_win_id);
	let delegate_class = get_or_register_delegate_class::<R>();
	let delegate: id = {
	let alloc: id = msg_send![delegate_class, alloc];
	msg_send![alloc, init]
	};
	(*delegate).set_ivar("window", ns_win_id);

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/platform/macos/delegates.rs
Line: 394-396

Comment:
**Missing `init` after `alloc`**

ObjC convention requires `alloc` to be followed by `init` before any instance methods (including ivar access) are used. For a plain `NSObject` subclass `init` is a no-op and `alloc` zeroes memory, so this works today, but it is technically undefined per the ObjC spec and may break with stricter runtimes or future changes.

```suggestion
        let delegate_class = get_or_register_delegate_class::<R>();
        let delegate: id = {
            let alloc: id = msg_send![delegate_class, alloc];
            msg_send![alloc, init]
        };
        (*delegate).set_ivar("window", ns_win_id);
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

Shared static CLASS captures function pointers for only the first R monomorphization

In Rust, static items inside generic functions are shared across all monomorphizations — there is exactly one CLASS for the entire program regardless of R. get_or_init will only ever run the closure once, registering ObjC method implementations for whichever R type first calls setup. Any subsequent setup::<DifferentR>() reuses that class silently, with the wrong monomorphized function pointers. In a real Tauri application R is always tauri::Wry, so this causes no observable issue today, but the pattern will silently misbehave if the type ever changes.

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/platform/macos/delegates.rs
Line: 350-386

Comment:
**Shared `static CLASS` captures function pointers for only the first `R` monomorphization**

In Rust, `static` items inside generic functions are shared across all monomorphizations — there is exactly one `CLASS` for the entire program regardless of `R`. `get_or_init` will only ever run the closure once, registering ObjC method implementations for whichever `R` type first calls `setup`. Any subsequent `setup::<DifferentR>()` reuses that class silently, with the wrong monomorphized function pointers. In a real Tauri application `R` is always `tauri::Wry`, so this causes no observable issue today, but the pattern will silently misbehave if the type ever changes.

How can I resolve this? If you propose a fix, please make it concise.

[ManthanNimodiya]

@greptileai please re-review

[tembo]

Now that app_box can be set to null, any late delegate callback that still hits this object (e.g. an event queued before setDelegate: takes effect) will cause with_window_state to deref a null pointer and SIGSEGV.

Might be worth adding a null guard in with_window_state (or otherwise ensuring no callbacks can run after windowWillClose:).

[ManthanNimodiya]

@tembo please re-review

[tembo]

@tembo please re-review

re-reviewed pr #1913. all of my earlier feedback is now addressed:

• alloc → new (proper objc init)
• windowWillClose: restores the previous delegate before releasing self — ref counting checks out (new gives +1, nswindow doesn't retain the delegate, single release balances it, no leak/over-release)
• with_window_state now null-guards app_box, so late resize/fullscreen callbacks after close are ignored instead of derefing null
• the shared generic static CLASS caveat is documented and fine in practice (R is always tauri::Wry)

the leak fix itself is correct for the normal path.

one thing to fix before merge: the Format (Cargo) ci check is failing — the long decl.add_method(...) lines aren't cargo fmt-clean. a quick cargo fmt clears it.

minor non-blocking note: the drop/release cleanup sits inside the suppress_delegate_panic closure, so if the forwarded super_del windowWillClose: ever panicked, cleanup would be skipped and you'd fall back to the original leak (not a crash). acceptable as-is.

once ci is green it's good to go from my side. left this as a review comment on the pr.

[tembo]

Minor robustness nit: right now all of the cleanup lives inside the same suppress_delegate_panic closure as the forwarded super_del call. If msg_send![super_del, windowWillClose: …] ever panics, we’ll skip dropping app_box / restoring the delegate / releasing self, which brings back the leak in the error path.

Suggested change

	extern "C" fn on_window_will_close<R: Runtime>(this: &Object, _cmd: Sel, notification: id) {
	suppress_delegate_panic("windowWillClose:", (), || unsafe {
	let super_del: id = *this.get_ivar("super_delegate");
	let _: () = msg_send![super_del, windowWillClose: notification];
	// Drop the boxed `WindowState<R>` (and the `Window<R>` handle it holds)
	// that was leaked via `Box::into_raw` when this delegate was created.
	let app_box: *mut c_void = *this.get_ivar("app_box");
	if !app_box.is_null() {
	drop(Box::from_raw(app_box as *mut WindowState<R>));
	let this_mut = this as *const Object as *mut Object;
	(*this_mut).set_ivar("app_box", std::ptr::null_mut::<c_void>());
	}
	// Restore the previous delegate before releasing this one, so any
	// further delegate callbacks during teardown don't hit a freed object.
	let window: id = *this.get_ivar("window");
	let _: () = msg_send![window, setDelegate: super_del];
	// NSWindow does not retain its delegate, so the reference taken when
	// this delegate was created (`new`) is the only owning one. Release
	// it now that the window is closing.
	let this_id = this as *const Object as id;
	let _: () = msg_send![this_id, release];
	});
	}
	extern "C" fn on_window_will_close<R: Runtime>(this: &Object, _cmd: Sel, notification: id) {
	let super_del: id = unsafe { *this.get_ivar("super_delegate") };
	// Forward to the previous delegate, but don't let it prevent cleanup.
	suppress_delegate_panic("windowWillClose:", (), || unsafe {
	let _: () = msg_send![super_del, windowWillClose: notification];
	});
	suppress_delegate_panic("windowWillClose:cleanup", (), || unsafe {
	// Drop the boxed `WindowState<R>` (and the `Window<R>` handle it holds)
	// that was leaked via `Box::into_raw` when this delegate was created.
	let app_box: *mut c_void = *this.get_ivar("app_box");
	if !app_box.is_null() {
	drop(Box::from_raw(app_box as *mut WindowState<R>));
	let this_mut = this as *const Object as *mut Object;
	(*this_mut).set_ivar("app_box", std::ptr::null_mut::<c_void>());
	}
	// Restore the previous delegate before releasing this one, so any
	// further delegate callbacks during teardown don't hit a freed object.
	let window: id = *this.get_ivar("window");
	let _: () = msg_send![window, setDelegate: super_del];
	// NSWindow does not retain its delegate, so the reference taken when
	// this delegate was created (`new`) is the only owning one. Release
	// it now that the window is closing.
	let this_id = this as *const Object as id;
	let _: () = msg_send![this_id, release];
	});
	}

[tembo]

Re-review ✅

Thanks for the updates — all of my earlier feedback has been addressed:

• alloc → new: the instance is now properly allocated + initialized.
• Delegate restore before release: windowWillClose: now restores the previous delegate (setDelegate: super_del) before release-ing self, so teardown callbacks land on a live object. Reference accounting is correct — new gives a +1, NSWindow doesn't retain its delegate, and the single release balances it (no leak, no over-release).
• Null deref after close: with_window_state now early-returns when app_box is null, so any late windowDidResize:/fullscreen callback after windowWillClose: is safely ignored instead of dereferencing null. Combined with the delegate swap this is solid belt-and-suspenders.
• Shared static CLASS / generic R: documented clearly. Since setup is only ever called with R = tauri::Wry, the single process-wide class is fine in practice.

The leak fix itself looks correct for the normal path.

One thing to fix before merge: the Format (Cargo) CI check is failing — the long decl.add_method(...) lines in get_or_register_delegate_class aren't cargo fmt-clean. A quick cargo fmt should clear it.

Minor (non-blocking) note for the record: the drop/release cleanup lives inside the suppress_delegate_panic closure, so if the forwarded super_del windowWillClose: ever panicked, cleanup would be skipped and you'd fall back to the original leak behavior (not a crash). Acceptable as-is given the forwarding call doesn't realistically panic.

Once CI is green this is good to go from my side.

[ManthanNimodiya]

@greptileai please re-review

[tembo]

Small robustness tweak: instead of panicking when the class is already registered (e.g. in unusual init / multi-runtime scenarios), consider reusing the existing class.

Suggested change

	.expect("CapWindowDelegate class already registered");
	if let Some(existing) = Class::get("CapWindowDelegate") {
	return existing;
	}
	let mut decl = ClassDecl::new("CapWindowDelegate", class!(NSObject))
	.expect("failed to register CapWindowDelegate");

[tembo]

Minor safety tweak: consider nulling the app_box ivar before dropping the Box, so any synchronous re-entrancy during drop(WindowState) won’t observe a dangling pointer.

Suggested change

	(*this_mut).set_ivar("app_box", std::ptr::null_mut::<c_void>());
	let app_box: *mut c_void = *this.get_ivar("app_box");
	if !app_box.is_null() {
	let this_mut = this as *const Object as *mut Object;
	(*this_mut).set_ivar("app_box", std::ptr::null_mut::<c_void>());
	drop(Box::from_raw(app_box as *mut WindowState<R>));
	}

[tembo]

Small robustness tweak: ClassDecl::new can still return None if the class gets registered between the Class::get check and this call. Falling back to Class::get avoids a panic here.

Suggested change

	.expect("failed to register CapWindowDelegate");
	let mut decl = match ClassDecl::new("CapWindowDelegate", class!(NSObject)) {
	Some(decl) => decl,
	None => {
	return Class::get("CapWindowDelegate")
	.expect("CapWindowDelegate should exist if already registered");
	}
	};

[ManthanNimodiya]

@tembo please re-review

[tembo]

@tembo please re-review

re-reviewed pr #1913 at commit 315dfe8 — #1913

looks good, posted an approving re-review. all my earlier feedback is now addressed:

• box leak: reclaimed in windowwillclose: via box::from_raw, with the app_box ivar nulled before the drop
• per-call objc class leak: class registered once via oncelock and reused (randomized-name registration gone)
• alloc without init: now uses new
• late-callback null deref: with_window_state guards against null app_box
• teardown ordering: prior delegate restored before release, so the object is deregistered from appkit's observers before being freed; new (+1) balanced by exactly one release
• panic isolation: super_delegate forward and cleanup are in separate suppress_delegate_panic closures so a panic in the forward can't skip cleanup
• registration robustness: class::get short-circuit + classdecl::new none fallback avoid panicking if already registered

the two remaining open threads are about the process-wide static class inside the generic fn. it's documented in-code and a non-issue in practice — the only caller (add_traffic_lights in windows.rs) calls setup exclusively with r = tauri::wry, on the main thread. no change needed.

the second file (memory-leak-detector.rs) is a pure cargo fmt change to unblock format ci, no behavior change.

nothing blocking. one heads-up: the macos/windows clippy checks were still pending at review time, worth a glance once they finish.

[tembo]

Re-review — looks good ✅

Re-reviewed at 315dfe8. All previously raised feedback is now addressed:

• Box<WindowState> leak — windowWillClose: now reclaims it via Box::from_raw, nulling the app_box ivar before the drop to avoid a dangling pointer on synchronous re-entrancy.
• Per-call ObjC class leak — the class is registered exactly once via OnceLock and reused; the randomized-name registration is gone.
• alloc without init — now uses new (alloc+init).
• Late-callback null deref — with_window_state guards against a null app_box.
• Teardown ordering — the prior delegate is restored (setDelegate: super_del) before release, so AppKit deregisters this object from its notification observers before it can be deallocated. new (+1) is balanced by exactly one release, so no leak and no over-release.
• Panic isolation — the super_delegate forward and the cleanup are in separate suppress_delegate_panic closures, so a panic in the forward can't skip cleanup (which would reintroduce the leak).
• Registration robustness — Class::get short-circuit plus a ClassDecl::new → None fallback avoid panicking if the class already exists.

Two open threads remain about the process-wide static CLASS inside the generic fn capturing the first monomorphization's method pointers. This is documented in-code and is a non-issue in practice: the only caller (add_traffic_lights in windows.rs) invokes setup exclusively with R = tauri::Wry, on the main thread. No change needed unless a second runtime type is ever introduced.

The second file (memory-leak-detector.rs) is a pure cargo fmt change to unblock the Format CI — no behavior change.

Nothing blocking from my side. Note the macOS/Windows Clippy checks were still pending at review time — worth a glance once they finish.

[richiemcilroy]

/tip $30

[algora-pbc]

Please visit Algora to complete your tip via Stripe.

[ManthanNimodiya]

hey @richiemcilroy, looks like the tips are still pending on algora's side, this one and the one from #1865 both seem to need you to click through, please have a look when you get chance
thank you 🙏🏻