Fix crash, UB, and correctness defects found in full-codebase review - #10
Open
OleksandrAWS wants to merge 2 commits into
Open
Fix crash, UB, and correctness defects found in full-codebase review#10OleksandrAWS wants to merge 2 commits into
OleksandrAWS wants to merge 2 commits into
Conversation
Settings previously had no way to view or manage the correction blacklist beyond the "Enable/Disable in Current App" menu-bar item, which only affected the frontmost app. - Add an "Excluded Apps" list to SettingsView backed by AppFilter, with icons/names resolved via NSWorkspace and remove via a minus button (multi-select supported). - Add app via a menu: pick from currently running apps (a sheet listing NSWorkspace.shared.runningApplications, excluding apps already excluded and SwitchFix itself), or browse the filesystem via NSOpenPanel (defaults to /Applications, but any folder works). - Resize the Settings window to fit the new section.
Crash / undefined behavior:
- TextCorrector: snapshot pasteboard item data into fresh NSPasteboardItems
before clearContents(); items read from a pasteboard are invalidated by
clearing and writing them back throws an ObjC exception (clipboard was
also permanently lost on selection corrections).
- KeyCodeMapping, KeyboardMonitor, InputSourceManager: keep the
UCKeyboardLayout pointer inside withUnsafeBytes; returning it out of the
closure and calling UCKeyTranslate afterwards is undefined behavior.
- BloomFilter: clamp bitCount/hashCount to >= 1 and pad undersized bit
arrays (public inits could previously trap on modulo-by-zero or index
out of bounds).
Correctness:
- TextCorrector: hop TISSelectInputSource calls in apply()/undo() to the
main thread (TIS APIs are main-thread-only; these ran on the correction
queue), and refuse to build events for empty replacement text (a
zero-length unicode key event types a literal "a" in many apps).
- KeyboardMonitor: classify forward delete (117) and F17-F19 (64/79/80)
as navigation/function keys; forward delete previously entered the word
buffer as U+F728, corrupting correction delete counts.
- KeyboardMonitor: fire the CapsLock revert hotkey only when the
alpha-shift bit actually flips, so one press cannot trigger revert and
then fall through to converting the current selection.
- PreferencesManager: distinguish key code 0 ("A") from "unset" via
object(forKey:); recording a hotkey on the letter A silently rebound it
to the default (Space / CapsLock).
- ScriptAnalyzer, LayoutDetector: fix the Latin range in mixed-script
detection (0x41-0x7A included "[ \ ] ^ _ `"; now A-Z / a-z).
- InputEngine/InputStateMachine: after dropping an event with a stale
capture context, mark the buffer invalid until the next boundary so a
partial word can't be "corrected" with a wrong delete count.
- Dictionary: normalize words to NFC in compile_dictionary.swift and in
DictionaryLoader lookups; bloom hashing and partition lookup operate on
raw UTF-8 bytes, so NFD input previously produced false negatives.
- WordValidator: accept irregular English contractions ("can't", "won't",
"shan't", ...) whose base does not survive suffix stripping.
Resource management / UI:
- SettingsView: stop the hotkey recorder's NSEvent monitor in deinit
(closing Settings mid-recording leaked a monitor that swallowed
keystrokes app-wide) and pass through non-CapsLock flagsChanged events;
use a non-blocking NSOpenPanel.begin instead of runModal(); prune stale
list selections on reload.
- AppFilter: guard state with a lock (the singleton is reachable from
multiple threads) and persist user deltas instead of the frozen
effective set, so future default-blacklist additions reach existing
users; legacy key is migrated and kept in sync.
- SettingsWindowController: remove the willClose observer for dead
windows (registrations accumulated per open/close cycle).
- StatusBarController: disable menu auto-enablement so explicit
isEnabled writes take effect; drop a doubled separator.
- Remove dead CorrectionContext.swift (unreferenced duplicate of
LayoutDetector's private suppression logic).
Verified: swift build clean; TestRunner 116/116, InputPipelineTestRunner
842/842 (incl. 100k-event stress); app smoke-launches with active tap.
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
2 participants
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.
Summary
A full expert review of the codebase (Core input pipeline, Dictionary, UI/Utils/App) surfaced one likely crasher, several undefined-behavior sites, and a set of correctness/robustness defects. This PR fixes all confirmed findings.
Crash / undefined behavior
performSelectionCorrectionsnapshottedpasteboard.pasteboardItems, calledclearContents(), then wrote the same items back 150 ms later. Items read from a pasteboard are invalidated by clearing; writing them back raises an ObjC exception on the main thread, and the user's clipboard was permanently lost. Now item data is copied into freshNSPasteboardItems before clearing.withUnsafeBytespointer escapes (UB) ×3:KeyCodeMapping,KeyboardMonitor.translatedCharacter,InputSourceManager.translatedCharacterall returned theUCKeyboardLayoutpointer out of the closure and calledUCKeyTranslateafterwards. The calls now happen inside the closure.BloomFilterpublic inits could trap (modulo-by-zero onbitCount: 0, out-of-bounds on undersized bit arrays). Now clamped/padded.Correctness
apply()/undo()calledTISSelectInputSourceon the correction queue; TIS is main-thread-only. Now hopped to main (the selection path already did this).object(forKey:).[ \ ] ^ _(0x41–0x7A); fixed to A–Z/a–z in bothScriptAnalyzerandLayoutDetector`.Resource management / UI
NSEventmonitor now stopped indeinit(closing Settings mid-recording leaked a monitor that swallowed keystrokes app-wide); non-CapsLockflagsChangedevents pass through.begininstead of app-modalrunModal()(which stole frontmost-app focus from the capture pipeline); stale list selections pruned on reload.AppFilter: lock-guarded; persists user deltas instead of a frozen effective set, so future default-blacklist additions reach existing users (legacy key migrated and kept in sync for downgrades).SettingsWindowController: willClose observers no longer accumulate per open/close cycle.autoenablesItems = falseso explicitisEnabledwrites take effect; removed a doubled separator.CorrectionContext.swift(unreferenced duplicate ofLayoutDetector's private logic).Known issues deliberately not addressed here
InputSourceManager.switchTotrusts a cached source ID (deliberate perf tradeoff after theTISCopyCurrentKeyboardInputSourcelag fix).launchAtLogin/SMAppServicestatus reconciliation.Test plan
swift buildclean