Fix for incorrect release of CCW holder#84927
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
the
|
/cc @dotnet/interop-contrib |
| { | ||
| ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis); | ||
| uint refcount = wrapper->Release(); | ||
| if (wrapper->RefCount == 0) |
There was a problem hiding this comment.
Released actually here
|
LGTM other than what looks like some remains of the dead code in the smoke test. |
|
Would this meet the bar for a .NET 7 servicing release, or will this be .NET 8 only? |
jkoritzinsky
left a comment
jkoritzinsky
left a comment
There was a problem hiding this comment.
After looking into this more, there's a more serious issue here that needs to be fixed before this is merged in. The ComWrappers implementation on NativeAOT uses a strong GC handle to hold the managed object alive from the MOW. With this change, the managed object will never be released, as the MOW will hold a strong ref to the managed object that will not be released until the MOW is destroyed, which won't happen until after the managed object is released since the MOW is kept alive by the managed object through a ConditionalWeakTable.
I believe we need to change the ComWrappers implementation to use ref-counted handles in this case. NativeAOT has partial support for ref-counted handles, but it might need to be extended slightly to work as well as possible.
3 participants
Fixes: microsoft/CsWinRT#1321 Make ComWrappers in C# work closely with how C++ part works
RefCount=>curris plain typo