Strip HTML notes without leaking mach ports - #953
Open
jamtur01 wants to merge 6 commits into
Open
Conversation
htmlTagsStrippedForMeetingLinks and String.htmlTagsStripped both used
NSAttributedString(html:) to turn HTML notes into plain text. That
initializer spins up TextKit's nsattributedstringagent XPC service and
leaks ~2 mach ports every call that never come back. Link detection runs
it for every event with HTML notes on every calendar refresh, so over a
day the port table fills up and the kernel kills the app with EXC_GUARD
("allocating too many mach ports" — I caught one at 305,834 ports). I
think this is what's behind leits#872, and probably leits#716/leits#721 too.
Replaced it with HTMLPlainText, a pure-Foundation strip: block tags
become newlines, everything else is dropped, and entities are decoded
including decimal and hex numeric forms. Lives in MeetingLinkDetector so
both the logic target and the app share it. No XPC, nothing to leak.
Measured it to be sure — a little harness reading mach_port_names shows
NSAttributedString(html:) climbing ~2 ports/call and never releasing,
while the regex strip stays flat at zero across hundreds of calls. That
became the regression test: it hammers the strip 400 times and fails if
the port table grows, so a future NSAttributedString(html:) can't sneak
back in. Also added tests for & URL reconnection, entity decoding,
and block-tag newlines.
The first cut of HTMLPlainText only knew a handful of named entities, so NSAttributedString(html:)'s full decoding got narrowed — a note written with named accents like café or München would show the raw entity instead of café / München. Filled out the table with the Latin-1 accented letters (upper and lower) and the common symbols (euro, pound, frac12, times, …). Numeric entities were already total, so this closes the named-side gap; anything still outside the table is left as written, which the doc comment now spells out. Also swapped mach_task_self_ for task_self_trap() in the port-leak test — the raw global tripped -strict-concurrency=complete on CI (shared mutable var); the trap returns the same task port as a plain function. Added a test for the accented names.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMeetingBar replaces ChangesHTML plain-text conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StringExtensions
participant MeetingLinkDetector
participant HTMLPlainText
StringExtensions->>HTMLPlainText: convert HTML fragments
MeetingLinkDetector->>HTMLPlainText: convert detected HTML
HTMLPlainText-->>StringExtensions: normalized plain text
HTMLPlainText-->>MeetingLinkDetector: normalized plain text
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MeetingBar/Meetings/MeetingLinkDetector.swift (1)
317-328: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider precompiling the regexes used in
from(_:).Each call recompiles four regex patterns via
replacingOccurrences(options: .regularExpression). Given the stated call frequency ("every event with HTML notes on every calendar refresh"), hoisting these to precompiledstatic letNSRegularExpressioninstances would cut redundant compilation overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeetingBar/Meetings/MeetingLinkDetector.swift` around lines 317 - 328, Precompile the four regular-expression patterns used by MeetingLinkDetector.from(_:) as static let NSRegularExpression instances, then reuse them for HTML tag replacement, whitespace cleanup, and newline collapsing instead of invoking replacingOccurrences with .regularExpression on each call. Preserve the current replacement order and output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MeetingBar/Meetings/MeetingLinkDetector.swift`:
- Around line 317-328: Update MeetingLinkDetector.from(_:) to remove complete
script and style blocks, including their contents, before the generic HTML tag
stripping step. Preserve the existing visible-text conversions, entity decoding,
whitespace normalization, and trimming for all non-script/style content.
- Around line 330-349: Update decodeEntities(in:) so the semicolon lookup is
limited to the maximum entity length from the current ampersand, rather than
scanning the entire remaining string. Preserve the existing 12-character
validation, decode(entity:) behavior, and fallback character-by-character
handling while ensuring malformed input is processed linearly.
---
Nitpick comments:
In `@MeetingBar/Meetings/MeetingLinkDetector.swift`:
- Around line 317-328: Precompile the four regular-expression patterns used by
MeetingLinkDetector.from(_:) as static let NSRegularExpression instances, then
reuse them for HTML tag replacement, whitespace cleanup, and newline collapsing
instead of invoking replacingOccurrences with .regularExpression on each call.
Preserve the current replacement order and output behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 233738a7-6e8f-45fb-b457-4479572cc38b
📒 Files selected for processing (3)
MeetingBar/Extensions/String.swiftMeetingBar/Meetings/MeetingLinkDetector.swiftMeetingBarLogicTests/MeetingLinkDetectorTests.swift
from(_:) recompiled five regex patterns on every call, and it runs per event with HTML notes on every calendar refresh. Hoisted them to static let NSRegularExpression compiled once and reused via a small helper. Same patterns, order and output — just no per-call compilation. Closes the last CodeRabbit note on leits#953.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MeetingBarLogicTests/MeetingLinkDetectorTests.swift (1)
613-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeallocate memory returned by
mach_port_names.The
mach_port_nameskernel call allocates out-of-line memory for thenamesandtypesarrays. Since this memory is not automatically freed by Swift, it leaves behind a small memory leak each time the test runs. While negligible in a test process, it is best practice to clean it up—especially within a test designed to prevent resource exhaustion.Consider using
vm_deallocatein adeferblock to free the allocated arrays.♻️ Proposed refactor
private func machPortCount() -> Int { var names: mach_port_name_array_t? var namesCount: mach_msg_type_number_t = 0 var types: mach_port_type_array_t? var typesCount: mach_msg_type_number_t = 0 - guard mach_port_names(task_self_trap(), &names, &namesCount, &types, &typesCount) == KERN_SUCCESS + let task = task_self_trap() + guard mach_port_names(task, &names, &namesCount, &types, &typesCount) == KERN_SUCCESS else { return -1 } + + defer { + if let names = names { + vm_deallocate(task, vm_address_t(bitPattern: names), vm_size_t(namesCount) * vm_size_t(MemoryLayout<mach_port_name_t>.stride)) + } + if let types = types { + vm_deallocate(task, vm_address_t(bitPattern: types), vm_size_t(typesCount) * vm_size_t(MemoryLayout<mach_port_type_t>.stride)) + } + } return Int(namesCount) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeetingBarLogicTests/MeetingLinkDetectorTests.swift` around lines 613 - 621, Update machPortCount() to deallocate the memory returned in names and types by mach_port_names using vm_deallocate in a defer block. Ensure cleanup runs after both successful and failed calls while preserving the existing -1 failure result and count return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@MeetingBarLogicTests/MeetingLinkDetectorTests.swift`:
- Around line 613-621: Update machPortCount() to deallocate the memory returned
in names and types by mach_port_names using vm_deallocate in a defer block.
Ensure cleanup runs after both successful and failed calls while preserving the
existing -1 failure result and count return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64aa5e42-216c-486d-a1c9-cd6d66b794a5
📒 Files selected for processing (3)
MeetingBar/Meetings/MeetingLinkDetector.swiftMeetingBarLogicTests/MeetingLinkDetectorTests.swiftMeetingBarTests/HelpersTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- MeetingBar/Meetings/MeetingLinkDetector.swift
mach_port_names hands back out-of-line memory for its names/types arrays that Swift won't reclaim, so the leak-detection helper was itself leaking a little each call. Deallocate both in a defer. Reading the count happens first, so the measurement is unchanged. Addresses the last CodeRabbit nit on leits#953.
Contributor
Author
|
From what I can see, the test failure is an unrelated flaky test. |
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
1 participant
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.
Status
READY
Description
Fixes a mach-port leak that gets MeetingBar force-killed after long uptime.
htmlTagsStrippedForMeetingLinks(link detection) andString.htmlTagsStripped()(notes display) both turned HTML notes into plain text withNSAttributedString(html:). That initializer spins up TextKit'scom.apple.textkit.nsattributedstringagentXPC service and leaks ~2 mach ports per call that are never reclaimed. Link detection runs it for every event with HTML notes on every calendar refresh, so the process port table grows unbounded until the kernel kills the app withEXC_GUARD— "allocating too many mach ports" (I caught one at 305,834 ports; unified-log shows thensattributedstringagentconnection flood right after each EventKitCADEventPredicate).The HTML strip dates back to #144 and still exists in the 5.0 codebase, so the 4.11 rewrite didn't remove it.
Replaced both call sites with
HTMLPlainText, a pure-Foundation strip (inMeetingLinkDetector.swiftso the logic target and app share it): block-level tags become newlines, other tags are removed, and entities are decoded — all numeric forms ({/{) plus a named table covering the five XML names, common symbols, and the full Latin-1 accented-letter set (café, München). No XPC, nothing to leak. Named entities outside the table are left as written (documented in the code); numeric coverage is total.Provenance:
d646f98— replaceNSAttributedString(html:)withHTMLPlainText; add the regression + behavioral tests.a35af58— flesh out the named-entity table (the first cut narrowed the old full decoding); also swapmach_task_self_→task_self_trap()in the port test so it builds under-strict-concurrency=complete.Likely the cause behind #872 (same
nsattributedstringagentsignature), and probably #716 / #721 / #888.No breaking changes to external interfaces. One behavior note: the notes-display path now decodes numeric + Latin-1/common named entities rather than the full HTML named set; anything rarer shows as written instead of decoded.
Steps to Test or Reproduce
NSAttributedString(html:)in the strip, add a calendar event whose notes contain HTML (Google/Outlook invites do) and let it refresh over time.log stream --predicate 'process == "MeetingBar"'shows repeatedactivating connection … com.apple.textkit.nsattributedstringagentright after each EventKitCADEventPredicate.mach_port_nameson the task, or the harness intestHTMLStripDoesNotLeakMachPorts) across many strip calls — it climbs ~2 ports/call and never drops. Left running, the port table fills and the kernel kills the app withEXC_GUARD/ port-space exhaustion (seen at 305,834 ports).nsattributedstringagentconnections in the log, and the mach-port count stays flat across calls.Summary by CodeRabbit
Bug Fixes
script/style, converts block breaks to new lines, and normalizes whitespace for readability.Tests