FIX(macos): Update some of macOS obj-c code for 10.15 minimum #7004
Conversation
WalkthroughReplaces manual Pre-merge checks and finishing touches✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mumble/GlobalShortcut_macx.mm (1)
205-235: StrengthendumpEventTapslogging aroundprocessNamewhile using the new poolWrapping
dumpEventTaps()in@autoreleasepoolis appropriate for scopingNSRunningApplicationandNSStringusage during logging. One improvement worth making in this touched block:processNamecan remainnilifrunningApplicationWithProcessIdentifier:fails, but it’s passed directly to%svia[processName UTF8String]. Consider guarding this and substituting a fallback string (e.g."?"or"<unknown>") whenprocessNameisnilto avoid relying on implementation‑defined handling of a nullchar *in the logging backend. This is a preexisting edge case but easy to fix while you’re modifying the function.After applying a guard, re-run the Ctrl+Alt+Cmd dump to confirm all event taps log correctly, including those where
NSRunningApplicationreturnsnil.src/mumble/os_macx.mm (1)
24-36:query_language(): consider CF safety checks and releasingcfaLangsThe new
@autoreleasepoolwrapper is fine here and doesn’t change behavior, but inside this block there are a couple of CoreFoundation details worth tightening up:
CFPreferencesCopyAppValuefollows the “Create/Copy” rule, socfaLangsshould beCFReleased once you’re done with it.cfaLangsis assumed to be a non‑nullCFArrayRefwith at least one element; if that assumption ever fails (unexpected prefs state),CFArrayGetValueAtIndex((CFArrayRef)cfaLangs, 0)could crash.Consider adding:
- A null/type check (and, ideally,
CFArrayGetCount(...) > 0) beforeCFArrayGetValueAtIndex.- A corresponding
CFRelease(cfaLangs)before every return path that uses it.These are preexisting issues but easy to address in this touched function without changing its external behavior.
After adjusting, please test startup on macOS with normal and customized language settings to ensure
os_langis still set as expected and no new crashes occur inquery_language().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mumble/GlobalShortcut_macx.mm(2 hunks)src/mumble/Overlay_macx.mm(1 hunks)src/mumble/os_macx.mm(1 hunks)
🔇 Additional comments (2)
src/mumble/Overlay_macx.mm (1)
80-128: Autorelease pool migration inappLaunched:keeps behavior intactUsing
@autoreleasepoolhere is correct in this MRC file: all Objective‑C objects created in the block stay scoped to the pool, and the earlyreturnwhen the launched app is the main bundle will still drain the pool on exit. The exclusion‑mode logic andoverlayEnabledhandling are preserved as before, with no new lifetime issues introduced.Please rerun the overlay‑injection path on app launch (including the self‑bundle early‑return case) to confirm behavior matches pre‑change.
src/mumble/GlobalShortcut_macx.mm (1)
115-121: Overlay forwarding path:@autoreleasepoolconversion looks correctThe new
@autoreleasepoolaround creating and queuingNSEvent *evtpreserves the previous semantics: autoreleased intermediates are drained, while the explicitretain/releasepair forevtstill governs its lifetime across the queued connection. Returningnullptrimmediately after the block maintains the original behavior of consuming events whenforward && Global::get().ocInterceptis true.Please verify via manual testing that forwarded mouse/keyboard events still reach
forwardEventand are no longer passed back to the system when the overlay is active, matching prior behavior.
klobucar
force-pushed
the
jk-autorelese-macos
branch
from
719c377 to
873ca6a
Compare
klobucar
changed the title
klobucar
force-pushed
the
jk-autorelese-macos
branch
from
873ca6a to
e27d277
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mumble/os_macx.mm (1)
23-36:query_languageautorelease pool change preserves behavior; consider tightening CFPreferences handlingThe switch to
@autoreleasepoolhere looks fine:static char lang[16];inside the block still has static storage duration, soos_lang = lang;remains valid after the pool drains, and the control flow is unchanged.While you’re touching this code, there are a couple of long‑standing robustness gaps that would be good to fix (not introduced by this PR):
CFPreferencesCopyAppValuecan returnnullptror a non‑array;CFArrayGetValueAtIndex((CFArrayRef)cfaLangs, 0)on that would be undefined.- The
CFPropertyListRefreturned byCFPreferencesCopyAppValueis neverCFRelease’d (minor one‑shot leak fromos_init()).If you want to harden this, something along these lines would be safer:
- cfaLangs = CFPreferencesCopyAppValue(CFSTR("AppleLanguages"), kCFPreferencesCurrentApplication); - cfsLang = (CFStringRef) CFArrayGetValueAtIndex((CFArrayRef)cfaLangs, 0); - - if (! CFStringGetCString(cfsLang, lang, 16, kCFStringEncodingUTF8)) - return; - - os_lang = lang; + cfaLangs = CFPreferencesCopyAppValue(CFSTR("AppleLanguages"), kCFPreferencesCurrentApplication); + if (cfaLangs && CFGetTypeID(cfaLangs) == CFArrayGetTypeID() + && CFArrayGetCount((CFArrayRef)cfaLangs) > 0) { + cfsLang = (CFStringRef) CFArrayGetValueAtIndex((CFArrayRef)cfaLangs, 0); + if (CFStringGetCString(cfsLang, lang, sizeof(lang), kCFStringEncodingUTF8)) { + os_lang = lang; + } + } + if (cfaLangs) { + CFRelease(cfaLangs); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mumble/GlobalShortcut_macx.mm(2 hunks)src/mumble/Overlay_macx.mm(1 hunks)src/mumble/os_macx.mm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: PR (Docs)
- GitHub Check: PR (macOS)
- GitHub Check: PR (Translations)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (Linux)
🔇 Additional comments (2)
src/mumble/GlobalShortcut_macx.mm (1)
115-121: Autorelease pool usage around forwarded NSEvent looks correctThe new
@autoreleasepoolaround the overlay forwarding path is fine in a non‑ARC, manual retain/release setup:
eventWithCGEvent:returns an autoreleasedNSEvent; the explicitretainkeeps it alive past the pool drain.forwardEventeventually calls[event release];, so there’s no leak.- Returning
nullptrwhenforward && ocInterceptis true preserves the early‑exit behavior for intercepted events.No changes needed here.
src/mumble/Overlay_macx.mm (1)
80-129:appLaunched:refactor to@autoreleasepooland reformatted switch looks safeThe new
@autoreleasepoolaround theactivebranch, plus movinguserInfo/bundleId/SBApplicationusage inside it, matches the previous lifetime expectations and keeps all Cocoa objects scoped to the pool.The rewritten
switch (Global::get().s.os.oemOverlayExcludeMode):
- Still enables the overlay unconditionally for
LauncherFilterExclusionMode(with the same log).- Enables only when the bundle is whitelisted for
WhitelistExclusionMode.- Enables only when the bundle is not blacklisted for
BlacklistExclusionMode.- Uses explicit braces and
breaks, so there’s no fall‑through risk.Early return when launching the main bundle is preserved, just inside the pool now. Overall, behavior and memory management look correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mumble/GlobalShortcut_macx.mm (1)
205-235: Still a potential crash whenprocessNameis nil indumpEventTaps
processNameis initialized toniland only set whenNSRunningApplication *appis found. Ifappisnil,[processName UTF8String]yields a null pointer and the%sinqWarningcan dereference it and crash. The new@autoreleasepooldoesn’t change this.You can guard it like this:
- NSString *processName = nil; - NSRunningApplication *app = [NSRunningApplication runningApplicationWithProcessIdentifier: info->processBeingTapped]; - if (app) { - processName = [app localizedName]; - } + NSString *processName = nil; + NSRunningApplication *app = + [NSRunningApplication runningApplicationWithProcessIdentifier:info->processBeingTapped]; + if (app) { + processName = [app localizedName]; + } @@ - qWarning(" tappingProcess = %i (%s)", info->tappingProcess, [processName UTF8String]); + const char *processNameUtf8 = processName ? [processName UTF8String] : "unknown"; + qWarning(" tappingProcess = %i (%s)", info->tappingProcess, processNameUtf8);This keeps the logging detail but guarantees a non-null
%sargument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mumble/GlobalShortcut_macx.mm(2 hunks)src/mumble/Overlay_macx.mm(1 hunks)src/mumble/os_macx.mm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: PR (Docs)
- GitHub Check: PR (macOS)
- GitHub Check: PR (Translations)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (Linux)
🔇 Additional comments (2)
src/mumble/Overlay_macx.mm (1)
83-128:appLaunched:pool and exclusion-mode switch look goodUsing
@autoreleasepoolhere is appropriate for the Cocoa/SBApplication objects, and the refactoredoemOverlayExcludeModeswitch (with explicitbreaks and per‑mode blocks) preserves the previous logic while improving readability. No functional issues spotted.src/mumble/GlobalShortcut_macx.mm (1)
115-121: Overlay event forwarding pool is correctWrapping the NSEvent creation and
QMetaObject::invokeMethodin@autoreleasepoolkeeps Cocoa allocations bounded, while the explicitretainand laterreleaseinforwardEvent()preserve the lifetime across the queued connection. Returningnullptrto swallow the event when forwarding to the overlay matches the intended behavior.
klobucar
changed the title
|
Do you want me to add these suggestions in? |
- Replace [dict objectForKey:] with modern subscripting syntax dict[key] - Replace NSAutoreleasePool with @autoreleasepool blocks - Replace deprecated AudioDeviceGetProperty with AudioObjectGetPropertyData - Remove obsolete macOS version checks for < 10.15 - Remove dead 32-bit code (__LP64__ guards) - Remove unnecessary respondsToSelector: checks for APIs available in 10.15+ - Fix potential crash in dumpEventTaps when process name is nil - Fix memory leak in query_language by adding proper CFRelease calls - Add defensive null checks in query_language for CFPreferencesCopyAppValue - Fix unused variable warning in AppNap.mm - Remove duplicate [reason release] call - Remove deprecated API warning suppressions from CoreAudio.mm and Overlay_macx.mm - Comment out deprecated AuthorizationExecuteWithPrivileges with migration note
klobucar
force-pushed
the
jk-autorelese-macos
branch
from
e27d277 to
ff23d2c
Compare
klobucar
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mumble/Overlay_macx.mm (1)
320-336: MakeauthExecexplicitly a disabled stub instead of doing a dummywait()With
AuthorizationExecuteWithPrivilegesremoved,authExecnow waits on a non‑existent child and always returnsfalse, andargvis unused. That’s functionally a no‑op, but the remainingwait()/status logic is misleading.You could simplify this to a clear stub that documents that privileged uninstall is disabled on 10.15+:
static bool authExec(AuthorizationRef ref, const char **argv) { - OSStatus err = noErr; - int pid = 0, status = 0; - - // Deprecated: AuthorizationExecuteWithPrivileges removed. Consider using SMJobBless or a helper tool for privileged operations. - if (err == errAuthorizationSuccess) { - do { - pid = wait(&status); - } while (pid == -1 && errno == EINTR); - return (pid != -1 && WIFEXITED(status) && WEXITSTATUS(status) == 0); - } - - qWarning("Overlay_macx: Failed to AuthorizeExecuteWithPrivileges. (err=%i)", err); - qWarning("Overlay_macx: Status: (pid=%i, exited=%u, exitStatus=%u)", pid, WIFEXITED(status), WEXITSTATUS(status)); - - return false; + Q_UNUSED(ref); + Q_UNUSED(argv); + + qWarning("Overlay_macx: authExec is disabled because AuthorizationExecuteWithPrivileges is " + "deprecated/removed on macOS 10.15+. Uninstall must be handled by a new helper (e.g. SMJobBless) " + "or manually."); + return false; }This makes it obvious to future readers that uninstall is intentionally disabled until a new privileged helper is implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mumble/AppNap.mm(1 hunks)src/mumble/CoreAudio.mm(3 hunks)src/mumble/GlobalShortcut_macx.mm(3 hunks)src/mumble/Overlay_macx.mm(3 hunks)src/mumble/TextToSpeech_macx.mm(1 hunks)src/mumble/os_macx.mm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (Translations)
- GitHub Check: PR (Docs)
- GitHub Check: PR (Linux)
- GitHub Check: PR (macOS)
- GitHub Check: freebsd
🔇 Additional comments (11)
src/mumble/CoreAudio.mm (2)
16-17: Good: Removed blanket deprecation warning suppression.Removing the file-wide
#pragma clang diagnostic ignored "-Wdeprecated-declarations"is the right approach. This ensures any remaining deprecated API usage will be flagged during compilation.
719-720: LGTM: Correct migration from deprecatedAudioDeviceGetPropertytoAudioObjectGetPropertyData.The new call correctly uses the already-defined
propertyAddressstructure (set tokAudioDevicePropertyBufferFrameSizeon line 714) and follows the same pattern used elsewhere in the file.src/mumble/TextToSpeech_macx.mm (1)
135-137: LGTM: Safe removal ofrespondsToSelector:check.
setVolume:has been available onNSSpeechSynthesizersince macOS 10.5, so the runtime check is unnecessary with a 10.15 minimum target. The volume scaling from 0-100 to 0.0-1.0 is correct.src/mumble/os_macx.mm (1)
23-48: LGTM: Properly addresses memory leak and crash potential.The implementation now:
- Validates
cfaLangsis non-null, is actually aCFArray, and has at least one element before accessing- Correctly releases
cfaLangson all code paths (early returns and success)- Uses
sizeof(lang)instead of hardcoded16for the buffer sizeThis addresses the concerns raised in the previous review.
src/mumble/AppNap.mm (2)
21-22: LGTM: Cleaner NSString construction.Using
initWithCharacters:length:withQString::unicode()is a direct and correct way to create an NSString from a QString's UTF-16 data.
24-29: LGTM: Safe removal of version guards.
disableAutomaticTermination:andenableAutomaticTermination:have been available since macOS 10.7, so they can be called unconditionally with a 10.15 minimum target.src/mumble/GlobalShortcut_macx.mm (3)
115-121: LGTM: Proper autoreleasepool scoping for NSEvent.Wrapping the
NSEventcreation in@autoreleasepoolensures any temporary autorelease objects are cleaned up promptly. Theretainon line 117 ensures the event survives past the pool for later use inforwardEvent. Movingreturn nullptrinside the block is correct—the pool drains before the return executes.
193-224: LGTM: Crash fix for nilprocessNameand proper autoreleasepool usage.The previous review identified a crash when
processNameis nil. Line 213 now correctly handles this:const char *processNameUtf8 = processName ? [processName UTF8String] : "unknown";The
@autoreleasepoolblock appropriately scopes theNSRunningApplicationlookups, ensuring their associated autorelease objects are released promptly.
174-176: LGTM: Simplified keyboard layout initialization.Removal of version-specific fallback guards aligns with the 10.15 minimum requirement.
src/mumble/Overlay_macx.mm (2)
78-126: Autorelease pool migration and overlay selection logic look goodThe move to
@autoreleasepooland dictionary subscripting is correct, and the whitelist/blacklist handling now clearly matches the exclusion mode semantics. No functional issues spotted here.
305-315: Keyed subscripting forMumbleOverlayVersionis fineUsing
infoPlist[@"MumbleOverlayVersion"]withunsignedIntegerValueis an idiomatic drop‑in replacement for the oldobjectForKey:usage; this block looks good.
|
|
||
| // Ignore deprecation warnings for the whole file, for now. | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
|
|
There was a problem hiding this comment.
The comment above should be removed as well
| int pid = 0, status = 0; | ||
|
|
||
| err = AuthorizationExecuteWithPrivileges(ref, argv[0], kAuthorizationFlagDefaults, const_cast<char * const *>(&argv[1]), nullptr); | ||
| // Deprecated: AuthorizationExecuteWithPrivileges removed. Consider using SMJobBless or a helper tool for privileged operations. |
There was a problem hiding this comment.
It seems like this effectively removes the entire functionality of authExec - in that case, we should return false (with a log message stating that this needs to be reimplemented.
There was a problem hiding this comment.
Instead of removing the functionality entirely, we can replace it with a supported approach.
From what I can gather, the main action in the uninstallFiles function is running the command
/bin/mv /Library/ScriptingAdditions/MumbleOverlay.osax /tmp/%1_Uninstalled_MumbleOverlay.osaxWe can achieve the same effect using AppleScript.
NSAppleScript provides the command do shell script ... with administrator privileges. Where ... is the command we want to run. This way, we could remove the authExec function entirely and update the rest of uninstallFiles to use the modern API.
From the AppleScript documentation under do shell script:
Execute the command as the administrator. Once a script is correctly authenticated, it will not ask for authentication again for five minutes. The elevated privileges and the grace period do not extend to any other scripts or to the rest of the system.
We can use the NSAppleScript class to create the commands and execute them. This should keep the functionality the same but use the supported Apple API.
Sure! I’ve been quite busy with university, so I apologise on my recent absence. But I’ll take a look at this PR today! |
3 participants
Make the macOS code in obj-c with some changes to modernize
Replace [dict objectForKey:] with modern subscripting syntax dict[key]
Replace NSAutoreleasePool with
@autoreleasepoolblocksReplace deprecated AudioDeviceGetProperty with AudioObjectGetPropertyData
Remove obsolete macOS version checks for < 10.15
Remove dead 32-bit code (LP64 guards)
Remove unnecessary respondsToSelector: checks for APIs available in 10.15+
Fix potential crash in dumpEventTaps when process name is nil
Fix memory leak in query_language by adding proper CFRelease calls
Add defensive null checks in query_language for CFPreferencesCopyAppValue
Fix unused variable warning in AppNap.mm
Remove duplicate [reason release] call
Remove deprecated API warning suppressions from CoreAudio.mm and Overlay_macx.mm
Comment out deprecated AuthorizationExecuteWithPrivileges with migration note
Checks