Marshal.ReleaseComObject does not account for ComWrappers

[kant2002]

I try to make WPF work using ComWrappers only, and hit followin road block. Currently RCW in WPF released using Marshal.ReleaseComObject. This method does not take into account that some code is using ComWrappers

I think in spirit of #50500 that behaviour should change.
/cc @AaronRobinsonMSFT

[AaronRobinsonMSFT]

@kant2002 This is by design. The appropriate API to call instead is Marshal.Release or some other variant to do the decrement. The Marshal.ReleaseComObject API is targeted for manipulating the built-in COM interop wrappers not user defined wrappers (that is, ComWrappers). What is the intent of calling this API on a ComWrappers instance?

[AaronRobinsonMSFT]

/cc @jkoritzinsky @elinor-fung

[kant2002]

I'm hit that when experimenting how far can I go with ComWrappers in WPF (to see NativeAOT ability potentially)

WPF retreive object using DllImport, and it is created using ComWrappers, then it is assumed that this is __ComObject and using Marshal.ReleaseComObject to release instance. I assume this is would be common pattern for existing COM codebases.

This is quick summary on the libraries which use this functions. I count only files, because it's easier to do in the browser.

https://github.com/dotnet/winforms/search?q=ReleaseComObject 6 non test files
https://github.com/dotnet/wpf/search?q=ReleaseComObject 30 no test files

https://github.com/dotnet/runtime/search?q=ReleaseComObject
System.DirectoryServices - 1 file
System.Data.OleDb - 7 file
System.Speech - 9 file
System.Drawing.Common - 1 file (Icon class)
System.DirectoryServices.AccountManagement - 1 file
System.Management - 4 file

Other files.
libraries/Common/src/System/Runtime/InteropServices/ComEventsSink.cs - 1
tools/Common/TypeSystem/Ecma/SymbolReader/UnmanagedPdbSymbolReader.cs - 1

WinForms almost not impacted if not using ActiveX. It works perfectly fine for me in NativeAOT in relatively complex app.
WPF not even starts with ComWrappers attached.
DirectoryServices, Management, Speech based on previous discussion are less supported. I would cry only about Management because admin work, and Speech because it's fun. I always can contribute if would be desperate.

[AaronRobinsonMSFT]

@kant2002 Rather than updating this API I would recommend converting entirely to ComWrappers and the returned wrapper could implement IDisposable. From within that I would release as desired. I don't think updating that API to handle ComWrappers wrappers is appropriate because there isn't an obvious way to convert a user defined wrapper into something we could release – it is an implementation detail of a ComWrappers instance.

[jkotas]

I agree that the ultimate goal should be to get to 100% ComWrappers, but I think it would make sense to update APIs like Marshal.ReleaseComObject to handle ComWrappers to make migration to ComWrappers easier.

Supporting ComWrappers in APIs like Marshal.ReleaseComObject costs very little and it is in the same spirit as ComWrappers.RegisterForMarshalling API that was also introduce to ease the migration.

[AaronRobinsonMSFT]

Supporting ComWrappers in APIs like Marshal.ReleaseComObject costs very little and it is in the same spirit as ComWrappers.RegisterForMarshalling API that was also introduce to ease the migration.

I don't think it costs very little. It is a complicated scenario with a fair bit of nuance.

1. This could only be made to work for a globally registered ComWrappers instance for the marshalling scenario. Which means we would need to detect that one was registered and the object passed in was created with it.
2. We would then call the ReleaseObjects() on the ComWrappers instance and pass in a single object – which is far more expensive than the current API implementation.

The above would mean (1) when and how this works is complicated and (2) the performance relative to the existing behavior (i.e., built-in) is much more and that defeats the purpose of ComWrappers.

I would argue we should separate them at this point and rely on either an IDisposable contract or perhaps something else. I do not see the value in overloading this API when it has a fair bit of complexity that users are bound to get wrong and shouldn't really be using anyways. The number of times we've had to diagnose early release RCWs is too many to count for me and introducing that outside of the ComWrappers API seems going down a road that has already be tread with many lessons learned.

[AaronRobinsonMSFT]

@kant2002 I took another look at this and really am concerned about the complexity here. We would need to update both Marshal.ReleaseComObject and Marshal.FinalReleaseComObject – the behavior would basically be identical. Aside from the relative cost of implementing these APIs we also must rely on a user's ComWrappers implementation. The contract for these APIs is rather specific and users have come to rely on an actual release – we can guarantee that with the built-in interop. In this new world we would rely on the ComWrappers implementation to provide a ReleaseObjects() that disables the wrappers safely and doesn't just return.

An argument could be made this is an advanced scenario so that is the cost. I'm sympathetic to this perspective actually. However, the arguments I am hearing are around successfully transitioning from the built-in system. This means we want to guarantee semantics to some degree and I don't see how we can do that. I will say that as we move forward with ComWrappers a convention for how wrappers are written should be defined. I personally like IDisposable but there are a lot of options here.

I would recommend doing the following prior to these calls:

if (Marshal.IsComObject(obj))
    Marshal.ReleaseComObject(obj);

[jkoritzinsky]

@AaronRobinsonMSFT System.__ComObject is internal to CoreLib, so that’s not possible.

[AaronRobinsonMSFT]

@AaronRobinsonMSFT System.__ComObject is internal to CoreLib, so that’s not possible.

Doh! All those years of seeing that in the VS debugger and I never realized it was marked internal. Hmmm. Now I think we need to talk about this...

@jkoritzinsky I've updated the example so it will work now but I do agree we should perhaps talk more about the needs here.