[ObjCRuntime] Provide a way to remove an object from the object map.#25166
Conversation
Problem: empty NSStrings (@"") are singletons, and trying to create one always returns the same handle.
This causes problems for the following sequence of events:
* Such a singleton is exposed to managed code, and a corresponding managed wrapper is created.
* The managed wrapper is manually disposed.
* The singleton surfaces again, and now we can end up returning the existing (disposed) wrapper from the object map instead of a new managed instance.
For empty NSStrings in particular:
1. There's a managed static NSString.Empty field, with a managed wrapper for a native @"" singleton.
2. Tests can create managed wrappers for @"", and if those managed wrappers are disposed, the managed NSString.Empty field now points to a disposed managed wrapper.
The MonoTouchFixtures.Foundation.BundleTest.GetLocalizedString test in particular disposes of strings, and can dispose NSString.Empty. This is from a modified runtime where an exception is thrown if NSString.Empty is disposed:
MonoTouchFixtures.Foundation.BundleTest
[FAIL] GetLocalizedString : System.InvalidOperationException : This Foundation.NSString instance was unexpectedly disposed
at Foundation.NSObject.Dispose(Boolean disposing) in /Users/rolf/work/dotnet/macios/main/macios/src/Foundation/NSObject2.cs:line 1108
at Foundation.NSObject.Dispose() in /Users/rolf/work/dotnet/macios/main/macios/src/Foundation/NSObject2.cs:line 365
at MonoTouchFixtures.Foundation.BundleTest.GetLocalizedString()
at MonoTouchFixtures.Foundation.BundleTest.GetLocalizedString()
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, Void** arguments, ObjectHandleOnStack sig, BOOL isConstructor, ObjectHandleOnStack result)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
Fixes this test failure:
MonoTouchFixtures.Foundation.StringTest
[FAIL] ReleaseEmptyString : RetainCount
Expected: 18446744073709551615
But was: 0
at MonoTouchFixtures.Foundation.StringTest.ReleaseEmptyString()
because this test calls release a number of times on the static `NSString.Empty` field, and expects a certain `retainCount` afterwards, but that fails because `NSString.Empty` has been disposed, and then `RetainCount` returns 0 because it's being called on a null native object.
There was a problem hiding this comment.
Pull request overview
Adds an internal mechanism to remove specific managed wrappers from the ObjCRuntime object map to avoid reusing disposed wrappers for native singleton/constant handles (notably the @"" empty NSString singleton).
Changes:
- Add
Runtime.RemoveFromObjectMap (NSObject obj)to remove a specific wrapper from the runtime object map. - Add an internal
NSObject.RemoveFromObjectMapsetter hook to invoke the runtime removal from object-initializer syntax. - Mark
NSString.Emptyto be removed from the object map immediately after creation to prevent it from being returned/disposed via handle lookups.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ObjCRuntime/Runtime.cs | Introduces RemoveFromObjectMap API and related documentation near object-map lookup logic. |
| src/Foundation/NSObject2.cs | Adds an internal setter-only hook to trigger object-map removal on an instance. |
| src/Foundation/NSString.cs | Updates NSString.Empty initialization to remove it from the object map. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
dalexsoto
left a comment
There was a problem hiding this comment.
Whoa really interesting issue 👍
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #85d5962] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #85d5962] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ 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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #85d5962] Build passed (Build macOS tests) ✅Pipeline on Agent |
🚀 [CI Build #85d5962] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: 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 |
Problem: empty NSStrings (@"") are singletons, and trying to create one always returns the same handle.
This causes problems for the following sequence of events:
For empty NSStrings in particular:
The MonoTouchFixtures.Foundation.BundleTest.GetLocalizedString test in particular disposes of strings, and can dispose NSString.Empty. This is from a modified runtime where an exception is thrown if NSString.Empty is disposed:
Fixes this test failure:
because this test calls release a number of times on the static
NSString.Emptyfield, and expects a certainretainCountafterwards, but that fails becauseNSString.Emptyhas been disposed, and thenRetainCountreturns 0 because it's being called on a null native object.