NativeAOT ComWrappers: extend managed object lifetime based on refcount #85087
Conversation
Fixes: microsoft/CsWinRT#1321 Make ComWrappers in C# work closely with how C++ part works - `RefCount` => `curr` is plain typo - Do not destroy wrappers if ref count == 0. That's mimic how CoreCLR work - Also discover 2 tests which are plainly wrong, since they do not work under CoreCLR
ghost
added
area-NativeAOT-coreclr
community-contribution
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis PR builds on #84927 by @kant2002 and is inspired by the comment by @jkoritzinsky. It uses a ref-counted GC handle to extend the lifetime of the managed object while the managed object wrapper still has a non-zero refcount. I have added two test cases to the ComWrappers unit tests to show the problem. On the
|
These now pass again with the ref counted handle.
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Show resolved
Hide resolved
| { | ||
| get | ||
| { | ||
| IntPtr handle = HolderHandle; |
There was a problem hiding this comment.
All other accesses to HolderHandle seem to be using interlocked APIs (which mean they also imply having volatile semantics for reads/writes). Should this read be volatile (or, just the field itself be marked as such) for consistency?
There was a problem hiding this comment.
I can add an Interlocked.Read here.
The existing RefCount field has potentially the same problem. It is updated using interlocked but read using simple field access in some places. I am not familiar enough with the memory model of .NET to know if this is "ok".
There was a problem hiding this comment.
Nevermind, Interlocked.Read only exists for Int64 and UInt64. I'll use volatile.
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Outdated
Show resolved
Hide resolved
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Show resolved
Hide resolved
…e/InteropServices/ComWrappers.NativeAot.cs Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
jkoritzinsky
left a comment
jkoritzinsky
left a comment
There was a problem hiding this comment.
I've validated that this works locally
4 participants
This PR builds on #84927 by @kant2002 and is inspired by the comment by @jkoritzinsky. It uses a ref-counted GC handle to extend the lifetime of the managed object while the managed object wrapper still has a non-zero refcount.
I have added two test cases to the ComWrappers unit tests to show the problem. On the
mainbranch the tests pass on CoreCLR and fail on NativeAOT. The tests pass on NativeAOT after this change.