Marshal.ReleaseComObject does not account for ComWrappers #54317

kant2002 · 2021-06-17T08:06:26Z

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

The text was updated successfully, but these errors were encountered:

AaronRobinsonMSFT · 2021-06-17T16:56:57Z

@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 · 2021-06-17T16:57:18Z

/cc @jkoritzinsky @elinor-fung

kant2002 · 2021-06-17T18:20:16Z

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 · 2021-06-17T18:36:36Z

@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 · 2021-06-18T02:04:51Z

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 · 2021-06-18T03:31:13Z

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.

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.
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 · 2021-06-18T04:43:09Z

@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 · 2021-06-18T04:56:43Z

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

AaronRobinsonMSFT · 2021-06-18T05:04:29Z

@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.

kant2002 · 2021-06-18T10:39:51Z

I'm fine with either IDisposable, finalizer or other custom interface, but I would like that somehow this function work almost like it is expected in that function.
Maybe behind some switch. Without that, you have to go for ComWrappers and rewrite all P/Invoke calls, and wait for libraries to catch up.
Essentially this prevents global ComWrappers to be useful for existing applications in my opinion.

I personally think about this interface

interface IComReference
{
   int Release();
}

jkotas · 2021-06-18T12:30:48Z

Essentially this prevents global ComWrappers to be useful for existing applications in my opinion.

The global ComWrappers instance is designed to only able to cover the simplest COM interop cases transparently (simple interface marshaling). It won't cover existing libraries and application that use COM interop in more advanced ways. It is very likely that applications or libraries that are using COM interop heavily will require changes to work well with ComWrappers (either global or local instance).

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

This will prevent the app from crashing with ComWrappers, but it won't make it work well. The code was presumably explicitly releasing the object for a reason, and not just because of it can. I think it would need to be:

if (Marshal.IsComObject(obj))
    Marshal.ReleaseComObject(obj);
else
    (obj as IDisposable)?.Dispose();

and the respective ComWrappers would have to implement IDisposable.

kant2002 · 2021-06-18T13:13:18Z

I think it would need to be:
That's seems pragmatic. I see how it can be easy applied across codebases.

If nothing would be added/changed, I can provide separate PR's for each library in this repo. Any objections?

jkotas · 2021-06-18T14:20:56Z

The challenge with changes like this is testing - how do we know that it actually works?

I think it would preferable to work towards the 100% ComWrappers solution: Pick one of these libraries, figure out how to make wean it off of the built-in COM support by default, and make it happen. Rinse & repeat.

kant2002 · 2021-06-18T15:19:02Z

Let's see. I would pick System.Drawing.Common as easiest target with only 2 interfaces. IStream and IPicture.
How I create RCW for IPicture, and CCW for IStream? Should I create them manually, or should I use codegen of some form? This one seems easy to create manually, but what about other libraries?

AaronRobinsonMSFT · 2021-06-18T15:31:22Z

@kant2002 I believe that @eerhardt is also looking into System.Drawing.Common. You two might want to sync.

eerhardt · 2021-06-18T15:46:41Z

Yes, I started working on moving System.Drawing.Common over to COM wrappers in order to make it trim compatible. Here are the current ILLink warnings in Drawing:

runtime/src/libraries/System.Drawing.Common/src/ILLink/ILLink.Suppressions.xml

Lines 16 to 81 in cff5854

    
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.Icon.OleCreatePictureIndirect(System.Drawing.Icon.PICTDESC,System.Guid@,System.Boolean)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipCreateBitmapFromStream(Interop.Ole32.IStream,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipCreateBitmapFromStreamICM(Interop.Ole32.IStream,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipCreateMetafileFromStream(Interop.Ole32.IStream,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipGetMetafileHeaderFromStream(Interop.Ole32.IStream,System.IntPtr)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipLoadImageFromStream(Interop.Ole32.IStream,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipLoadImageFromStreamICM(Interop.Ole32.IStream,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipRecordMetafileStream(Interop.Ole32.IStream,System.IntPtr,System.Drawing.Imaging.EmfType,System.Drawing.RectangleF@,System.Drawing.Imaging.MetafileFrameUnit,System.String,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipRecordMetafileStream(Interop.Ole32.IStream,System.IntPtr,System.Drawing.Imaging.EmfType,System.IntPtr,System.Drawing.Imaging.MetafileFrameUnit,System.String,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipRecordMetafileStreamI(Interop.Ole32.IStream,System.IntPtr,System.Drawing.Imaging.EmfType,System.Drawing.Rectangle@,System.Drawing.Imaging.MetafileFrameUnit,System.String,System.IntPtr@)</property> 
        
           </attribute> 
        
           <attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"> 
        
             <argument>ILLink</argument> 
        
             <argument>IL2050</argument> 
        
             <property name="Scope">member</property> 
        
             <property name="Target">M:System.Drawing.SafeNativeMethods.Gdip.GdipSaveImageToStream(System.Runtime.InteropServices.HandleRef,Interop.Ole32.IStream,System.Guid@,System.Runtime.InteropServices.HandleRef)</property> 
        
           </attribute>

They are all:

IL2050: Trim analysis: Correctness of COM interop cannot be guaranteed
P/invoke method 'method' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.

Moving them to use COM wrappers will resolve these ILLink warnings.

eerhardt · 2021-06-18T15:50:40Z

@kant2002 - if you want, you could pick up one of the other libraries?

https://github.com/dotnet/runtime/search?q=IL2050

System.Management
System.DirectoryServices
System.Data.Odbc
System.Data.OleDb

kant2002 · 2021-06-18T16:00:54Z

@eerhardt For now I would like to shadow you, to see how ComWrappers should be implemented. Can you tag me if you have PR or draft? Is there general consensus how it should be done? I can start poking into System.Data.Odbc or System.Management based on what should be done. Want to limit scope to easiest target first.

eerhardt · 2021-06-18T16:02:43Z

For sure! I'll tag you when I have a PR ready. Probably sometime next week.

eerhardt · 2021-06-23T18:32:15Z

FYI - the first round of converting System.Drawing.Common to ComWrappers: #54636.

kant2002 · 2021-06-28T08:43:53Z

Does System.Speech not appear in search for IL2050 because it uses ComImport attribute for object creation and no built-in COM marshalling is used, just built-in object creation?

Remove unused functions which trigger IL2050 As per discussed in dotnet#54317

eerhardt · 2021-06-28T14:59:17Z

Does System.Speech not appear in search for IL2050 because it uses ComImport attribute for object creation and no built-in COM marshalling is used, just built-in object creation?

No, we choose not to analyze System.Speech for trimmability yet, because it isn't a very used assembly (it only got supported on .NET Core in the 5.0 time frame). Here is the list of assemblies we ignore for trimmability (for now):

runtime/src/libraries/illink-oob.targets

Lines 15 to 25 in bde0322

    
           <!-- The following is the list of all the OOBs we will ignore for now --> 
        
           <_OOBsToIgnore Include="System.CodeDom" /> 
        
           <_OOBsToIgnore Include="System.ComponentModel.Composition" /> 
        
           <_OOBsToIgnore Include="System.ComponentModel.Composition.Registration" /> 
        
           <_OOBsToIgnore Include="System.Composition.AttributedModel" /> 
        
           <_OOBsToIgnore Include="System.Composition.Convention" /> 
        
           <_OOBsToIgnore Include="System.Composition.Hosting" /> 
        
           <_OOBsToIgnore Include="System.Composition.Runtime" /> 
        
           <_OOBsToIgnore Include="System.Composition.TypedParts" /> 
        
           <_OOBsToIgnore Include="System.Configuration.ConfigurationManager" /> 
        
           <_OOBsToIgnore Include="System.Speech" />

Some of that reasoning is listed here: #52272 (comment)

* Remove IL2050 in System.Management Remove unused functions which trigger IL2050 As per discussed in #54317 * Fix location of variables

AaronRobinsonMSFT · 2021-08-11T04:50:52Z

I think the follow up here should be documentation regarding contracts and how implementers should consume ComWrappers.

AaronRobinsonMSFT · 2022-06-17T00:21:09Z

This behavior has been documented in detail in the official walkthrough -https://docs.microsoft.com/en-us/dotnet/standard/native-interop/tutorial-comwrappers#step-4--handle-native-object-wrapper-lifetime-details.

dotnet-issue-labeler bot added area-Interop-coreclr untriaged New issue has not been triaged by the area owner labels Jun 17, 2021

AaronRobinsonMSFT added needs more info and removed untriaged New issue has not been triaged by the area owner labels Jun 17, 2021

AaronRobinsonMSFT added this to the Future milestone Jun 17, 2021

kant2002 mentioned this issue Jun 18, 2021

Marshal.ReleaseComObject in NativeAOT dotnet/runtimelab#1254

Closed

This was referenced Jun 28, 2021

ComWrappers support dotnet/wpf#4762

Open

Remove IL2050 warning for System.Data.Odbc #54809

Merged

kant2002 added a commit to kant2002/runtime that referenced this issue Jun 28, 2021

Remove IL2050 in System.Management

a9ede71

Remove unused functions which trigger IL2050 As per discussed in dotnet#54317

kant2002 mentioned this issue Jun 28, 2021

Remove IL2050 in System.Management #54810

Merged

marek-safar pushed a commit that referenced this issue Jul 13, 2021

Remove IL2050 in System.Management (#54810)

a38fc2f

* Remove IL2050 in System.Management Remove unused functions which trigger IL2050 As per discussed in #54317 * Fix location of variables

kant2002 mentioned this issue Jul 14, 2021

can you develop a COM Wrapper version that can run with wpf and corert? kant2002/WinFormsComInterop#1

Open

AaronRobinsonMSFT removed the needs more info label Aug 11, 2021

AaronRobinsonMSFT modified the milestones: Future, 7.0.0 Aug 11, 2021

AaronRobinsonMSFT mentioned this issue Sep 2, 2021

ComWrappers feature imbalance between platforms #56474

Closed

3 tasks

AaronRobinsonMSFT closed this as completed Jun 17, 2022

ghost locked as resolved and limited conversation to collaborators Jul 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marshal.ReleaseComObject does not account for ComWrappers #54317

Marshal.ReleaseComObject does not account for ComWrappers #54317

kant2002 commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021

kant2002 commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021 •

edited

Loading

jkotas commented Jun 18, 2021 •

edited

Loading

AaronRobinsonMSFT commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021 •

edited

Loading

jkoritzinsky commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021 •

edited

Loading

kant2002 commented Jun 18, 2021

jkotas commented Jun 18, 2021

kant2002 commented Jun 18, 2021

jkotas commented Jun 18, 2021 •

edited

Loading

kant2002 commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021

eerhardt commented Jun 18, 2021

eerhardt commented Jun 18, 2021

kant2002 commented Jun 18, 2021

eerhardt commented Jun 18, 2021

eerhardt commented Jun 23, 2021

kant2002 commented Jun 28, 2021

eerhardt commented Jun 28, 2021 •

edited

Loading

AaronRobinsonMSFT commented Aug 11, 2021

AaronRobinsonMSFT commented Jun 17, 2022

Marshal.ReleaseComObject does not account for ComWrappers #54317

Marshal.ReleaseComObject does not account for ComWrappers #54317

Comments

kant2002 commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021

kant2002 commented Jun 17, 2021

AaronRobinsonMSFT commented Jun 17, 2021 • edited Loading

jkotas commented Jun 18, 2021 • edited Loading

AaronRobinsonMSFT commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021 • edited Loading

jkoritzinsky commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021 • edited Loading

kant2002 commented Jun 18, 2021

jkotas commented Jun 18, 2021

kant2002 commented Jun 18, 2021

jkotas commented Jun 18, 2021 • edited Loading

kant2002 commented Jun 18, 2021

AaronRobinsonMSFT commented Jun 18, 2021

eerhardt commented Jun 18, 2021

eerhardt commented Jun 18, 2021

kant2002 commented Jun 18, 2021

eerhardt commented Jun 18, 2021

eerhardt commented Jun 23, 2021

kant2002 commented Jun 28, 2021

eerhardt commented Jun 28, 2021 • edited Loading

AaronRobinsonMSFT commented Aug 11, 2021

AaronRobinsonMSFT commented Jun 17, 2022

AaronRobinsonMSFT commented Jun 17, 2021 •

edited

Loading

jkotas commented Jun 18, 2021 •

edited

Loading

AaronRobinsonMSFT commented Jun 18, 2021 •

edited

Loading

AaronRobinsonMSFT commented Jun 18, 2021 •

edited

Loading

jkotas commented Jun 18, 2021 •

edited

Loading

eerhardt commented Jun 28, 2021 •

edited

Loading