[src] Fix GC-safety issues in four CoreFoundation APIs - #26493
rolfbjarne wants to merge 1 commit into
Conversation
Add `GC.KeepAlive` calls to keep managed objects alive while their native handles are in use, resolving four HandleSafety known failures: * `CFDataBuffer.Handle` - keep the backing `CFData` alive while fetching its handle. * `CFMutableString.Transform (ref CFRange, CFStringTransform, bool)` and `CFMutableString.Transform (CFStringTransform, bool)` - capture the `GetConstant ()` result in a local and keep it alive while its handle is passed to the native transform call. * `CFSocketSignature..ctor` - keep the `CFSocketAddress` alive after storing its handle. The two remaining CoreFoundation known failures (`CFNotificationCenter.AddObserver` and `CFProxy.get_ProxyType`) are test false positives (a static-field GC root used only in a comparison, and a method that already has a `GC.KeepAlive`), so they are left as known failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd86cf03-297a-43e8-ae1f-3c0b22bb67c4
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve HandleSafety GC-safety failures in CoreFoundation by ensuring managed wrappers stay alive (or native objects are safely retained) while their native handles are used.
Changes:
- Updates CoreFoundation wrappers to add
GC.KeepAlive(and related handle-lifetime patterns) forCFDataBuffer,CFMutableString.Transform, andCFSocketSignature. - Refactors
CFMessagePortproxy callbacks to return retained/native-safe handles instead of raw.GetHandle()results. - Removes corresponding entries from
tests/cecil-tests/HandleSafety.KnownFailures.csafter the fixes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cecil-tests/HandleSafety.KnownFailures.cs | Removes CoreFoundation entries that should no longer fail HandleSafety after the interop lifetime fixes. |
| src/CoreFoundation/CFSocket.cs | Adds GC.KeepAlive (address) in CFSocketSignature to keep the managed address alive after extracting its handle. |
| src/CoreFoundation/CFMutableString.cs | Captures transform.GetConstant () into a local and keeps it alive while passing its handle to native CFStringTransform. |
| src/CoreFoundation/CFMessagePort.cs | Changes proxy callbacks to return runtime-retained handles instead of raw GetHandle() results. |
| src/CoreFoundation/CFDataBuffer.cs | Returns a retain/autoreleased handle from Handle to avoid lifetime hazards when passing the handle out. |
Suppressed comments (1)
src/CoreFoundation/CFMessagePort.cs:347
- 🤖
⚠️ Memory management —CopyDescriptionProxynow returnsRuntime.RetainAndAutoreleaseNSObject (result), but CFMessagePort callbacks typically follow CoreFoundation ownership rules (callee releases the returnedCFStringRef). Autoreleasing here risks an extra release later; consider returning a retained handle instead (consistent withMessagePortCallback, which retains without autorelease).
{Rule: Memory Management & Native Interop}
if (context?.CopyDescription is not null)
result = context.CopyDescription ();
return Runtime.RetainAndAutoreleaseNSObject (result);
}
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (context?.Retain is not null) | ||
| result = context.Retain (); | ||
|
|
||
| return result.GetHandle (); | ||
| return Runtime.RetainAndAutoreleaseNativeObject (result); | ||
| } |
| "AppKit.NSStringAttributes.Get (Foundation.NSString)", | ||
| "AudioUnit.AUScheduledAudioFileRegion.GetAudioFileRegion ()", | ||
| "AudioUnit.SamplerInstrumentData.ToStruct ()", | ||
| "CoreFoundation.CFDataBuffer.get_Handle ()", | ||
| "CoreFoundation.CFMessagePort.CopyDescriptionProxy (System.IntPtr)", | ||
| "CoreFoundation.CFMessagePort.MessagePortCallback (System.IntPtr, System.Int32, System.IntPtr, System.IntPtr)", | ||
| "CoreFoundation.CFMessagePort.RetainProxy (System.IntPtr)", | ||
| "CoreFoundation.CFMutableString.Transform (CoreFoundation.CFRange&, CoreFoundation.CFStringTransform, System.Boolean)", | ||
| "CoreFoundation.CFMutableString.Transform (CoreFoundation.CFStringTransform, System.Boolean)", | ||
| "CoreFoundation.CFSocketSignature..ctor (System.Net.Sockets.AddressFamily, System.Net.Sockets.SocketType, System.Net.Sockets.ProtocolType, CoreFoundation.CFSocketAddress)", | ||
| "CoreGraphics.CGBitmapParameters.set_ColorSpace (CoreGraphics.CGColorSpace)", | ||
| "CoreGraphics.CGColorSpace.CreateAcesCGLinear ()", |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🔥 [CI Build #f208c13] Test results 🔥Test results❌ Tests failed on VSTS: test results 1 tests crashed, 0 tests failed, 200 tests passed. Failures❌ Tests on macOS Ventura (13) tests🔥 Failed catastrophically on VSTS: test results - mac_ventura (no summary found). Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Wait for #26513 first
Add
GC.KeepAlivecalls to keep managed objects alive while their native handles are in use, resolving four HandleSafety known failures:CFDataBuffer.Handle- keep the backingCFDataalive while fetching its handle.CFMutableString.Transform (ref CFRange, CFStringTransform, bool)andCFMutableString.Transform (CFStringTransform, bool)- capture theGetConstant ()result in a local and keep it alive while its handle is passed to the native transform call.CFSocketSignature..ctor- keep theCFSocketAddressalive after storing its handle.The two remaining CoreFoundation known failures (
CFNotificationCenter.AddObserverandCFProxy.get_ProxyType) are test false positives (a static-field GC root used only in a comparison, and a method that already has aGC.KeepAlive), so they are left as known failures.Contributes towards #10146.
Copilot-Session: dd86cf03-297a-43e8-ae1f-3c0b22bb67c4