Invalidate the event tap CFMachPort in ActiveEventMonitor.stop() - #1801
Open
MrMage wants to merge 1 commit into
Open
Invalidate the event tap CFMachPort in ActiveEventMonitor.stop()#1801MrMage wants to merge 1 commit into
MrMage wants to merge 1 commit into
Conversation
CoreGraphics holds internal references to the CFMachPort returned by CGEventTapCreate, so releasing the last client reference never deallocates it: without an explicit CFMachPortInvalidate, the window server keeps the (disabled) event tap registration until the process exits. Every stop()/start() cycle (snapping toggles, settings changes, and notably the tapDisabledByTimeout recovery in the tap callback) leaked one registration; accumulated stale registrations degrade system-wide input latency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request addresses a resource leak in ActiveEventMonitor teardown by explicitly invalidating the underlying CFMachPort created by CGEvent.tapCreate, ensuring the WindowServer event-tap registration is actually removed when monitoring stops.
Changes:
- Add an explicit
CFMachPortInvalidate(tap)call inActiveEventMonitor.stop()after disabling the event tap. - Document why invalidation is required to fully release the WindowServer-side registration and prevent accumulation across stop/start cycles.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
|
Thanks for contributing. I'll test it out in the next day or so and make sure there aren't any unexpected side effects.. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a resource leak in
ActiveEventMonitor: itsstop()disables the CGEventTap and drops the reference, but never callsCFMachPortInvalidate. CoreGraphics holds internal references to theCFMachPortreturned byCGEventTapCreate(observed retain count 4 right after creation), so releasing the client reference never deallocates it — the window server keeps the (disabled) event-tap registration until the process exits. Eachstop()/start()cycle therefore leaks one stale registration, and accumulated stale registrations measurably degrade system-wide input latency (the window server's per-event tap-table processing scales with table size).The same bug class was recently root-caused across several projects: fixes have been submitted to Karabiner-Elements (pqrs-org/Karabiner-Elements#4505 — includes a video of the symptom and standalone repro scripts), Mos (Caldis/Mos#999 — with a red/green regression test of exactly this fix pattern), pynput (moses-palmer/pynput#684), and deskflow (deskflow/deskflow#10010); Easydict found and fixed the identical leak independently (tisfeng/Easydict#1170, maintainer-confirmed and shipped in their 2.20.0). Long-lived tap-heavy projects (Hammerspoon, LinearMouse) all invalidate in their teardown paths.
This analysis and fix were prepared with AI assistance (Claude); the mechanism was verified on a real machine (macOS 26.5, Apple Silicon) and the change was human-reviewed.
Scope in Rectangle
ActiveEventMonitoris only instantiated in two non-default configurations —SnappingManagerwhen Mission-Control dragging is user-disabled, andGreenButtonManagerwhen the green-button override is enabled — which is presumably why no lag reports have surfaced in this tracker yet. But when either is active, registrations leak on:stopEventMonitor()/toggleListening()recreate the monitor), andtapDisabledByTimeout/tapDisabledByUserInputrecovery, since the tap callback handles those withstop(); start()(EventMonitor.swift) — this one can fire repeatedly under system load, silently.Mechanism verification (standalone)
On macOS 26.5: tearing a tap down with
tapEnable(false)+ run-loop-source removal + release leaves the mach receive right alive and theCGGetEventTapListentry present indefinitely; addingCFMachPortInvalidatereaps the entry immediately. Repeating the no-invalidate teardown in a loop grows the process's tap-table entry count by exactly 1 per cycle. Synthetically holding 1,000 such disabled registrations makes the whole Mac's input noticeably laggy within seconds; releasing them restores it. Repro scripts (taplist.swiftto enumerate the table,bloatgen.swiftto demonstrate impact) are in the Karabiner PR linked above.The fix
One line (+ comment) in
ActiveEventMonitor.stop():CFMachPortInvalidate(tap)aftertapEnable(false).stop()is the sole teardown path (start()always creates a fresh tap, anddeinitcallsstop()), so this covers all cases, including the timeout-recovery cycle. No behavior change on the event path — the tap is already disabled at that point.Validation
xcodebuild -project Rectangle.xcodeproj -scheme Rectangle build, unsigned).CGGetEventTapList— Rectangle's disabled-entry count should stay at 0 instead of growing by 1 per cycle.