[Foundation] Fix NSObjectData leak. - #24114
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory leak in NSObjectData by addressing the lifetime management of NSObjectDataHandle instances during object finalization and resurrection. The core issue stems from CriticalHandle being collected when the parent object becomes finalizable, even if the object is later resurrected - which happens for all NSObject instances in this codebase.
Key Changes
- Fixed critical bug in
NSObjectDataHandle.IsInvalidproperty logic (was inverted) - Added
RecreateDataHandle()method to create a new handle pointing to the same native memory during finalization, preventing premature collection - Added
Invalidate()method andinvalidatedfield to prevent double-freeing of native memory
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Foundation/NSObject2.cs | Core fix: Added RecreateDataHandle mechanism, fixed IsInvalid logic, added handle invalidation support to prevent memory leaks during object resurrection |
| tests/dotnet/UnitTests/expected/iOS-MonoVM-preservedapis.txt | Updated preserved API list to reflect new public members (RecreateDataHandle, Invalidate, new constructor, invalidated field) |
| tests/dotnet/UnitTests/expected/iOS-MonoVM-interpreter-preservedapis.txt | Updated preserved API list for interpreter configuration with same new members |
| // nothing to do here. | ||
| } else { | ||
| unsafe { | ||
| NativeMemory.Free ((void*) handle); |
There was a problem hiding this comment.
While this seems impossible in practice, I'm wondering whether there is a theoretical bug of accessing this memory after it was freed, particularly as we move over to CoreCLR which collects objects more aggressively compared to mono, where we used to pin a lot. We have the constraint of NSObject keeping NSObjectData alive. Consider the code:
set { GetData ()->flags = value; }
which is translates to
NSObjectData* data = this->GetData();
// `this` could potentially be collected at this point, with the data going down with it
data->flags = value;
A fix for this would be to add GC.KeepAlive(this) after access data.
Not really related to this particular change, but just something that I noticed which might be problematic in theory, as we will start running on CoreCLR. There might be other bits around hitting patterns like this.
There was a problem hiding this comment.
We already use (and run tests on) CoreCLR on macOS, so it's been at least validated that if there are any race conditions, they're not very frequent (since the tests are passing).
That said:
NSObjectData* data = this->GetData();
// `this` could potentially be collected at this point, with the data going down with it
data->flags = value;I guess this can happen if the GC runs to completion at line 2, collecting all normal objects and then also all CriticalHandles / critical finalizers, before line 3 is executed, so I'll add the GC.KeepAlive.
Going forward, for CoreCLR, it will probably make sense to use ObjectiveCMarshal.CreateReferenceTrackingHandle's tagged memory as a replacement for NSObjectData; the downside would be that we'd have very different code paths between Mono and CoreCLR.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
✅ [PR Build #aadfba4] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #aadfba4] Build passed (Build packages) ✅Pipeline on Agent |
✅ 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 #aadfba4] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #aadfba4] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #aadfba4] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #aadfba4] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #aadfba4] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #aadfba4] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
🚀 [CI Build #aadfba4] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 117 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Every
NSObjectallocates native memory forNSObjectDatavia aNSObjectDataHandlecritical handle. When the handle dies, the memory wasn't reclaimed because theNSObjectDataHandle.IsInvalidmethod was implemented incorrectly.Fixing this bug, leads to crashes in
ReleaseManagedRefwhich tries to access the native data that was already freed. Normally, this shouldn't happen becauseNSObjectis a normal finalizable object andNSObjectDataHandleis a critical finalizable object. This means that the finalizer forNSObjectwould always run first, considering that the 2 objects die at the same time. This means that the nativeNSObjectDatawould be cleared only after the finalizer forNSObjecthas run andReleaseManagedReffinished executing. However, this is not the case because the "finalization" code ofNSObjectis not run from the finalizer thread but it is enqueued toNSObject_Disposer. This means that now the critical finalizer could have actually released the native memory before the finalization of theNSObjectis done.Fix this by creating a new
NSObjectDataHandle, wrapping the same native memory pointer and preventing the previousNSObjectDataHandlefrom freeing the memory, when scheduling the actual finalization of theNSObjectinstance to the main thread.