Migrate macOS ObjC FFI to objc2, fixing the menu-bar leak (#99)#131
Conversation
enumerate_hidpp_devices() built a fresh HidBackend on every ~2 s inventory tick. On macOS that backend wraps an IOHIDManager which it schedules and, on drop, cancels — so each tick spun an IOHIDManager up and tore it down, needless churn that kept the process busy and its heap dirty around the clock. Hold one long-lived backend in a LazyLock (the usage async-hid intends); it also keeps the device set warm between polls. Refs #99.
The status-item/tray code modelled every AppKit object as a raw, hand-retained `id` over cocoa/objc 0.x. nsstring() returned a +1 NSString that was never released and ran on the 2 s tray refresh — issue #99's tens of thousands of leaked CFStrings. Rebuild the surface on objc2: Retained<T> owns each object and frees it on Drop, so the leak can't be written. NSString::from_str replaces nsstring() (and the autorelease-pool workaround is gone); the OpenLogiMenuTarget click target is now a define_class! subclass holding the channel in a typed ivar; NSMenu/NSMenuItem are MainThreadOnly (!Send), so the tray's retained state moves from OnceLock statics into a main-thread thread_local. permissions.rs's CBCentralManager lookup and the hook's NSWorkspace frontmost-app read also move to objc2, dropping cocoa/objc from both crates' direct dependencies (they remain only transitively via gpui). The CGEventTap stays on core-graphics: objc2-core-graphics does expose the tap API, but the Accessibility-revoke freeze-hazard run-loop machinery is load-bearing and out of scope here. Closes #99.
Scoped CLAUDE.md for platform/: the objc2 ownership model (Retained<T>, no manual retain/release, no nsstring()), the thread-affinity rules (MainThreadOnly vs AnyThread, MainThreadMarker, the tray thread_local), the remaining unsafe surface, why CGEventTap stays on core-graphics, and the gpui-pin / Metal-env build caveats. Captures the issue-#99 lesson so the leak class isn't reintroduced.
Greptile SummaryThis PR migrates the macOS Objective-C FFI from
Confidence Score: 5/5Safe to merge — the migration correctly eliminates the CFString leak and IOHIDManager churn, and the new objc2 type discipline makes the fixed class of bugs structurally unrepresentable. The objc2 migration is mechanically sound: every AppKit object is now owned by a Retained that releases on Drop, the define_class! MenuTarget wires the event channel through typed ivars rather than a global OnceLock, and the thread_local TrayState makes the MainThreadOnly constraint a type-system guarantee rather than a doc comment. The LazyLock fix is the correct async-hid usage. No correctness issues found in the changed code paths. No files require special attention — all changed files were reviewed and the logic is consistent throughout. Important Files Changed
Sequence DiagramsequenceDiagram
participant GPUI as GPUI main thread
participant TL as TRAY thread_local
participant MT as MenuTarget (define_class!)
participant AppKit as AppKit (NSMenu click)
participant Chan as mpsc channel
participant Drain as drain task
GPUI->>TL: install(tx) via TRAY.with_borrow_mut
TL->>MT: MenuTarget::new(mtm, tx.clone())
Note over MT: alloc -> set_ivars(tx) -> super-init
TL->>TL: "slot = Some(TrayState)"
AppKit->>MT: openOpenLogi: / quitOpenLogi:
MT->>Chan: ivars().tx.send(event)
Chan->>Drain: "recv -> cx.open() / cx.quit()"
GPUI->>TL: set_device_lines(lines)
TL->>TL: "setTitle on Retained<NSMenuItem> (no leak)"
GPUI->>TL: uninstall()
TL->>TL: "slot.take() -> TrayState drops -> all Retained<T> release"
Reviews (2): Last reviewed commit: "refactor(macos): document shared-backend..." | Re-trigger Greptile |
Addresses the Greptile review on #131 (both non-blocking): - transport.rs: document why the shared HID_BACKEND is safe under concurrent callers (the 2s watcher + open_route_writer) — async-hid declares the backend Send+Sync, enumerate only reads an IOHIDManagerCopyDevices snapshot, and one shared long-lived IOHIDManager is hidapi's model too. - tray.rs: debug_assert_main_thread() on the tray mutators. They are already memory-safe off-main (the !Send TrayState only exists in the main thread's TLS, so they no-op), but a future off-main caller is now a loud debug-build failure instead of a silent no-op. Free in release.
|
Addressed both non-blocking questions in 30287ce:
|
Why
Issue #99 reports memory climbing to 1 GB+. There were two distinct things behind it:
status_item.rs::nsstring()returned a+1-ownedNSString(NSString::alloc(nil).init_str) that was never released, and it ran on every 2 s tray refresh viaset_title. That is exactly the ~26k leakedCFStrings the reporter'sleaksrun found. The root cause isn't one missingrelease— it's the representation:cocoa/objc0.x model every object as a raw, hand-retainedid, so retain/release is an unenforced runtime discipline.enumerate_hidpp_devices()rebuilt a wholeHidBackend(a macOSIOHIDManagerit schedules and, on drop, cancels) on every ~2 s inventory tick — needless create/teardown that kept the process busy and its heap dirty around the clock.What this does
Migrate the macOS Objective-C FFI from
cocoa/objc0.x toobjc2(already in the tree via gpui/async-hid), so the leak class can't be written:Retained<T>owns each AppKit object and releases it once onDrop.nsstring()and the autorelease-pool workaround are gone — every string isNSString::from_str(s)used as a borrowed temporary. A+1-with-no-owner is unrepresentable.OpenLogiMenuTargetclick target is now adefine_class!subclass with the event channel in a typed#[ivars](noClassDecl/add_method, noOnceLock<MENU_TX>read in the callbacks).NSMenu/NSMenuItemareMainThreadOnly(!Send), so the tray's retained state moves fromOnceLockstatics into a main-threadthread_local— "main-thread only" is enforced by the type system, not a doc comment.permissions.rs(CBCentralManager.authorization) and the hook'sfrontmost_bundle_id(NSWorkspace, which isAnyThreadso it's sound on the watcher thread) also move toobjc2.cocoa/objc0.x leave both crates' direct dependencies (they remain inCargo.lockonly transitively via gpui).Reuse one HID backend in a
LazyLockinstead of rebuilding it per poll — the usage async-hid intends; it also keeps the device set warm between polls.Plus a scoped
crates/openlogi-gui/src/platform/CLAUDE.mddocumenting the conventions.What deliberately stays
The CGEventTap stays on
core-graphics/CF.objc2-core-graphicsdoes expose the tap API, but the Accessibility-revoke freeze-hazard run-loop machinery is load-bearing — out of scope here, byte-for-byte unchanged. Only theNSWorkspaceread in that file moved.Verification
cargo fmt --check, full-workspacecargo clippy --all-targets -- -D warnings, and all tests pass (hook+hid 26, gui 11).zed/gpui-component git pins inCargo.lockare unchanged — the lock diff is only the newobjc2-*crates.io packages.computermouse.fillstatus item renders; the menu builds correctly (AX dump: device rowMX Master 4 · NN%, separator,Open OpenLogi,Quit OpenLogi, spare rows hidden); and clicking Quit OpenLogi exits the app — confirming thedefine_class!action callback → ivar channel →cx.quit()chain fires end to end.Closes #99.