Skip to content

Conversation

@kant2002
Copy link
Contributor

@kant2002 kant2002 commented Apr 17, 2023

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. MOWHolder's finilizer calls Destroy
  • Also discover 2 tests which are plainly wrong, since they do not work under CoreCLR

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
@kant2002
Copy link
Contributor Author

/cc @dotnet/interop-contrib

{
ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis);
uint refcount = wrapper->Release();
if (wrapper->RefCount == 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Released actually here

~ManagedObjectWrapperHolder()
{
// Release GC handle created when MOW was built.
_wrapper->Destroy();
NativeMemory.Free(_wrapper);
}

@jkoritzinsky
Copy link
Member

LGTM other than what looks like some remains of the dead code in the smoke test.

@Sergio0694
Copy link
Contributor

Would this meet the bar for a .NET 7 servicing release, or will this be .NET 8 only?

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as draft May 15, 2023 20:19
@kant2002 kant2002 closed this Jun 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ComWrappers/CsWinRT code produces different (wrong) results on NativeAOT

3 participants