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 long-standing memory leaks in the macOS window delegate: a new randomized ObjC class was registered on every setup call (now replaced with a OnceLock singleton named CapWindowDelegate), and the Box-wrapped WindowState<R> and delegate instance were never freed on window close (now explicitly dropped and released in windowWillClose:).

• The OnceLock-based class registration eliminates permanent per-window ObjC class metadata growth across open/close cycles.
• on_window_will_close now drops app_box, restores the prior delegate, and calls release on self; the cleanup ordering (super-delegate call first, then release last) is correct for the normal path.

Confidence Score: 4/5

The fix is correct for the common path; the remaining concern is that cleanup in windowWillClose: is not panic-safe and would leave the leak in place if the forwarded super-delegate call ever panics.

The OnceLock class registration and the explicit drop/release in windowWillClose: are sound for the normal execution path. The one open gap is that all three cleanup steps live inside the suppress_delegate_panic closure, so a panic in the super_del call skips them entirely, preserving the original leak in the error path.

apps/desktop/src-tauri/src/platform/macos/delegates.rs — specifically the cleanup ordering inside on_window_will_close

Important Files Changed

Filename	Overview
apps/desktop/src-tauri/src/platform/macos/delegates.rs	Fixes per-window ObjC class leak (OnceLock singleton) and WindowState/delegate leak (explicit drop + release in windowWillClose:); cleanup in the close callback is not panic-safe, leaving the leak path open on super-delegate panics

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 (2): Last reviewed commit: "fix(macos): use alloc+new and restore pr..." | 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.