Review COM interface declarations for conflicting ComImport/ComVisible attributes

[weltkante]

During review of PR #1868 I noticed IOleInPlaceActiveObject was declared with both [ComImport] and [ComVisible(true)]. This is wrong, ComVisible(true) is for interfaces owned by the assembly, ComImport is for external interfaces.

In desktop framework this probably has little consequence since the assemblies come preinstalled, but now that its possible to deploy WinForms with an application it may more likely be processed by tools like regasm or TLB generators, which attempt to register COM interop owned by the assembly.

When ComVisible(true) is present this will cause registry corruption by overwriting the true owners registry entries (usually owned by Windows OS) and replacing them by .NET based values. For interfaces I believe this mostly replaces the ProxyStubClsid32 pointing to another implementation than the original entries used to have.

There should be a review for conflicting ComImport/ComVisible(true) entries, or better, just review ComVisible(true) in general and make sure it is only placed on classes/interfaces owned by WinForms.

[weltkante]

a quick search for ComVisible(true) yields 238 matches in 182 files, not all of them technically wrong (for example it is placed on control classes which are certainly implemented and owned by WinForms)

However considering that the codebase comes from Desktop Framework I wonder if there should be any ComVisible(true) attributes left in place at all, even for classes and interfaces actually owned by WinForms. The ComVisible WinForms GUIDs registered globally always used to be Desktop Framework, I wonder how many of the ComVisible classes/interfaces owned by WinForms actually need to be ComVisible in .NET Core for supported interop scenarios. Maybe most (or all) of them can be removed?

[weltkante]

@hughbe @RussKie I don't think this is fixed, I still find plenty of wrong [ComVisible(true)] attributes. Needs to be reopened or creating a follow-up issue for proper cleanup. I also noticed new [ComVisible(true)] attributes were being added in accessibility PRs, flagged them, but I think this needs a bit care in reviews, at least for the current time being.

[JeremyKuhne]

However considering that the codebase comes from Desktop Framework I wonder if there should be any ComVisible(true) attributes left in place at all, even for classes and interfaces actually owned by WinForms. The ComVisible WinForms GUIDs registered globally always used to be Desktop Framework, I wonder how many of the ComVisible classes/interfaces owned by WinForms actually need to be ComVisible in .NET Core for supported interop scenarios. Maybe most (or all) of them can be removed?

If classes had [ComVisible] on Framework I'd prefer to keep the attribute unless we can clearly document that it has some negative impact.

The auditing is a must for 5.0, I don't want to see us inadvertently exposing this attribute in unintended places.

[weltkante]

If classes had [ComVisible] on Framework I'd prefer to keep the attribute

I scrolled through all remaining ComVisible which can be found by "search references" in VS and they all (except one) are specified without GUID, meaning they get a generated GUID. I don't remember if I checked previously, but the generated GUID seems to be different from Desktop Framework, so having these ComVisible left over is probably acceptable since they cause no conflicts, but I still think most (if not all) ComVisible attributes are redundant.

They certainly don't contribute to compatibility with Desktop Framework since there is no shared TLB (Desktop Framework has a TLB and primary interop assembly for WinForms, you'd have to be compatible with those if you wanted that level of compatibility). So users of WinForms via COM have to rewrite all their interop code anyways and this time they don't have a TLB to import either. So while I don't think its harmful it also doesn't seem to provide any value, and it will make AOT efforts like corert potentially more complicated if they have to predeclare CCWs for objects never intended to be accessible from COM.

That said its probably still good to remove ComVisible later if you don't feel confident doing it for 5.0 - unless people start taking a dependency on them, but I don't know why anyone would be doing that.

unless we can clearly document that it has some negative impact.

The only negative impacts I currently can think of involve sharing a CLSID/IID with Desktop Framework, those cases should definitely be fixed.

There is one ComVisible + GUID remaining on IWin32Window which is shared with Desktop Framework, the registry has a TLB entry pointing to the Desktop Framework version. .NET Core should probably have its own GUID or switch to ComImport, .NET Core is not the owner of that IID.

[199621616]

@JeremyKuhne 我们的每个用户，都需要与当地的医疗保险系统或卫生信息系统对接，多使用COM互操作来实现。正是由于.NetCore3.1目前没有解决此问题，我们的对接模块不得不仍然停留在.NetFramwork4.6上。
Each of our users needs to interface with the local medical insurance system or health information system, and use COM interoperability to achieve more. Because. NETCORE 3.1 does not solve this problem at present, our docking module has to stay on. Netframework 4.6.

[hughbe]

If classes had [ComVisible] on Framework I'd prefer to keep the attribute unless we can clearly document that it has some negative impact.

By the way, what is the criteria for including them. I noticed that we do so for AccessibleObject subclasses. Do these attributes actually do anything?

[hughbe]

In desktop framework this probably has little consequence since the assemblies come preinstalled, but now that its possible to deploy WinForms with an application it may more likely be processed by tools like regasm or TLB generators, which attempt to register COM interop owned by the assembly.

When ComVisible(true) is present this will cause registry corruption by overwriting the true owners registry entries (usually owned by Windows OS) and replacing them by .NET based values. For interfaces I believe this mostly replaces the ProxyStubClsid32 pointing to another implementation than the original entries used to have.

Surely having it on IWin32Window won't cause any problems, as we don't override any windows registry entries?

[hughbe]

Also, would the fix be to simply have

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime.InteropServices;

namespace System.Windows.Forms
{
    [ComImport]
    [Guid("458AB8A2-A1EA-4d7b-8EBE-DEE5D3D9442C")]
    [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
    public interface IWin32Window
    {
        /// <summary>
        ///  Gets the handle to the window represented by the implementor.
        /// </summary>
        IntPtr Handle { get; }
    }
}

[weltkante]

Do these attributes actually do anything?

As far as I'm aware the only thing left is to allow COM to call CoCreateInstance (call the default constructor). You still have to do some registration or manifest declarations for this case besides using the attribute. I might be missing other arcane use cases.

Previously the most important usecase was to indicate "this class/interface/struct/enum is owned by this assembly and needs to be registered as such", including marking the types for TLB generation, but neither is currently implemented in .NET Core, you have to do it manually. If you do it manually then its a supported scenario of .NET Core, they brought that part of COM back with .NET Core 3.0 I think.

I noticed that we do [remove the attributes] for AccessibleObject subclasses.

This was before I noticed that ComVisible without GUID is safe to use because you get a different GUID generated.

Its still useless because AccessibleObject instances have no default constructor to be called from COM, and for exposing an existing instance you don't need that attribute.

Surely having it on IWin32Window won't cause any problems, as we don't override any windows registry entries?

Specifying ComVisible(true) has the semantics of overriding the windows registry entries. The fact thats currently not enabled for .NET Core does not mean anything. If anyone has custom registration logic (e.g. we have it in our products) and aren't aware that you assume the semantic got changed they might register .NET Core WinForms assemblies and overwrite the registry.

Note that having a reference to WinForms is enough to require registration of the assembly, they don't need to have written explicit code for registering .NET Core Framework assemblies. This is how the default logic of regasm rolled, and everyone who has written custom ones will have oriented themselves on its logic.

Also, would the fix be to simply have [...]

You can do that as a "quick fix" to become safe, but it would give you a dependency on the Desktop Framework being installed if you ever call that interface across apartments. (This interface GUID is not a Windows API, its owned and installed by the .NET Desktop Framework WinForms TLB - yes they install a TLB with the Desktop Framework.) You probably should use [ComVisible(true)] and use a new GUID.

[JeremyKuhne]

@199621616 if you have a specific WinForms COM scenario that is broken can you please file an issue with details so we can track it? If it is an issue with .NET Core COM interop in general you should create the issue in https://github.com/dotnet/runtime.

[JeremyKuhne]

@AaronRobinsonMSFT do we have any best practices for porting code from .NET Framework to .NET Core that used to use [ComVisible]? With or without specific GUIDs as @weltkante mentions in the discussion above?

[AaronRobinsonMSFT]

@JeremyKuhne This is a declarative mechanism and depending on how .NET controls are being passed around/consumed this could result in some very complicated/nasty scenarios.

As mentioned, if the interface is something that is new and desired to be provided to COM clients then ComVisibleAttribute is the appropriate mechanism. For .NET Core COM servers, I believe we decided to require the Guid because the auto-generation logic is more trouble than it is worth. Supplying the attribute is actually faster than generation and easy on Windows - (1) Developer command prompt, (2) uuidgen /c, (3) [Guid("...")].

For declaring an existing COM interface in .NET, the ComImportAttribute is used and requires the GuidAttribute for the exact reason it already exists.

If one declares a type ComVisible that is really an existing COM interface then it is shadowing that interface and could cause serious issues in a COM dependent scenario. Using the regasm tool this would result in overwriting existing registration and play heck with other control consumers. In .NET Core regasm isn't used, we updated the approach to use regsvr32 and follow the built in COM tools as much as possible. This doesn't change the fact that this approach would also overwrite existing registration.

I suggest marking interfaces following the intent of each of the aforementioned attributes. I prefer to always provide a GuidAttribute because it follows what happens in native COM. The only reason not to do this seems to be to not want to type a line of code or add additional metadata - neither resonate with me.

[JeremyKuhne]

@AaronRobinsonMSFT thanks! When it comes to GUIDs and [ComVisible] we should then:

1. Always provide an explicit GUID
2. Don't use [ComVisible] for actual COM interfaces (e.g. along with a [ComImport])
3. Never match a [ComVisible] GUID from .NET Framework?

Is this the right summary?

[AaronRobinsonMSFT]

3. Never match a [ComVisible] GUID from .NET Framework?

This is the safest approach. There are some caveats here that permit having the same Guids in most theoretical cases. One of the most important is that all .NET objects are free threaded and don't need apartment marshalling. However, there are issues with COM aggregation scenarios that make this far more complicated in practice.

[RussKie]

A follow up question - our accessibility object implementations are decorated with [ComVisible(true)], e.g.:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/AccessibleObject.cs

Lines 21 to 46 in e3f5000

	[ComVisible(true)]
	public partial class AccessibleObject :
	StandardOleMarshalObject,
	IReflect,
	IAccessible,
	UiaCore.IAccessibleEx,
	Ole32.IServiceProvider,
	UiaCore.IRawElementProviderSimple,
	UiaCore.IRawElementProviderFragment,
	UiaCore.IRawElementProviderFragmentRoot,
	UiaCore.IInvokeProvider,
	UiaCore.IValueProvider,
	UiaCore.IRangeValueProvider,
	UiaCore.IExpandCollapseProvider,
	UiaCore.IToggleProvider,
	UiaCore.ITableProvider,
	UiaCore.ITableItemProvider,
	UiaCore.IGridProvider,
	UiaCore.IGridItemProvider,
	Oleaut32.IEnumVariant,
	Ole32.IOleWindow,
	UiaCore.ILegacyIAccessibleProvider,
	UiaCore.ISelectionProvider,
	UiaCore.ISelectionItemProvider,
	UiaCore.IRawElementProviderHwndOverride,
	UiaCore.IScrollItemProvider

Is this correct?

/cc: @M-Lipin @vladimir-krestov

[weltkante]

It doesn't cause problems because there's no [Guid(...)] specified which could be shared with Desktop Framework. The .NET Core runtime generates different GUIDs so it would work. But its not best practice as discussed above, you'd either remove ComVisible(true) or specify a (newly generated) Guid attribute. I think we still need to decide which of the two options to go for.

Given that most AccessibleObject implementations are not createable from COM (they have a constructor requiring arguments) I personally would go for removing ComVisible(true), but if @JeremyKuhne prefers keeping them I'm fine as long as no GUIDs are shared with Desktop Framework.

[hughbe]

FYI in dotnet/runtime#37269

Types should not be both [ComImport] and [ComVisible(true)]
Types should not be marked as both imported from COM and made visible to COM.
COM-visible types should be accessible and creatable
Types marked ComVisible(true) should be public, non-abstract, and have a public parameterless constructor.
CA1409 looks to have the same intent, but only specifically flagged the public parameterless constructor.

[RussKie]

Ah found it under dotnet/runtime#37039

[JeremyKuhne]

Given that most AccessibleObject implementations are not createable from COM

I'd prefer removing the [ComVisible] attributes on these accessibility objects. I'd rather try and shed this and consider pulling it back if someone genuinely has a need for it (highly unlikely). If we can't come up with a semi-plausible use case for things with [ComVisible] I'd rather see us remove the attribute. It is likely to save us more time in the long run by not having to keep analyzing it- or surface use cases we're totally not aware of (again, highly unlikely).

[RussKie]

We had a private discussion with Jeremy, and concluded that we should remove all [ComVisible] decorations.
If/when we hear from our customers that some use-case requires the decoration - we'll add it back, and have the use-case documented.

@weltkante please let me know if you want to do it, or I'll set it up for grabs.
Thank you

[199621616]

@JeremyKuhne
示例一：医疗保险系统对接的COM互操作：
保险系统接口规范提供PB调用示例：

//创建地纬嵌入式接口对象，调用com组件
int vi Oleobject sei                                 
sei =create Oleobject     //创建ole对象seiproxy
vi= sei.connecttonewobject('sei3') //连接com组件 if vi <> 0 then    
messagebox('错误','创建地纬嵌入式接口sei对象失败！')    
return end if

我们使用.NetFramework4.6.1调用方式：
Type type = Type.GetTypeFromProgID("sei3", true);
dynamic COM = Activator.CreateInstance(type);

使用同样的方式，在.NetCore3.1.4上运行返回异常：
Creating an instance of the COM component with CLSID {D5D8D5B2-CCD5-4EEC-BE61-0CA13A1940C7} from the IClassFactory failed due to the following error: ffffdf8f 0xFFFFDF8F.

---

Example 1: com interoperability of medical insurance system docking:

The insurance system interface specification provides Pb call examples:

//Create the ground latitude embedded interface object and call COM component

int vi Oleobject sei                                 

SEI = create oleobject / / create OLE object seiproxy

vi= sei.connecttonewobject ('sei3 ') / / connect COM component if VI < > 0 then

MessageBox ('error ',' failed to create SEI object of ground latitude embedded interface! ')    

return end if

We use. Netframework 4.6.1 to call:

Type type = Type.GetTypeFromProgID ("sei3", true);

dynamic COM = Activator.CreateInstance (type);

Using the same method, run return exception on. NETCORE 3.1.4:

Creating an instance of the COM component with CLSID {D5D8D5B2-CCD5-4EEC-BE61-0CA13A1940C7} from the IClassFactory failed due to the following error: ffffdf8f 0xFFFFDF8F.

[weltkante]

please let me know if you want to do it

@RussKie I'll put a PR up later today

[RussKie]

Awesome 👍

[JeremyKuhne]

@199621616 I've created dotnet/runtime#37362 to follow up on your comment as it appears that you have a general COM interop issue.

cc: @AaronRobinsonMSFT