Update ActiveX and COM interop

[weltkante]

@awakecoding As discussed over at dotnet/runtime#66674 (comment) you were looking into using the new COM interop and being held back by the lack of TLB support. The TLB tooling however isn't critical, it's always been possible to write the interface declarations manually.

I did a very quick experiment (https://github.com/weltkante/MsRdpEx/tree/srcgen) to see into what problems I'm running and to give you a little bit of early feedback. I mostly copy/pasted the declarations from ILSpy and fixed issues as they presented, instead of properly porting the IDL of the TLB. Code not required for the simple scenario of setting up a RDP connection is also commented out.

Its not my intention to keep it it this way, next step is doing it properly, but it'll take some time and with holidays in between its nicer to show some early results instead of just waiting.

Things to note so far:

• IDispatch is required for event handling and not provided by the source generator, but can be implemented manually. In the current state I just used old COM interop for the event handler interface to get the IDispatch support.
• AxHost itself is still using the old COM interop, the documentation claims to support the code generators are supposed to be castable into old-style interfaces, but I couldn't get it to work. As a workaround I did manually conversion between the two styles of interop. Will have to research a bit more and maybe ask someone from the runtime team whats up with that, but its not much of an issue since a workaround is possible.
• WinForms itself is still using old COM interop for accessibility, so in no case you'll be able to disable it entirely in .NET 8. Not sure how that affects Native AOT, I've seen people work on enabling Native AOT for WinForms in some scenarios so I assume it might still work. If not thats something to work with the WinForms team on improving. (I haven't tried Native AOT on the example code yet.)
• Being source-compatible with the old generated library isn't possible, the source generators don't support properties so they have to be written as methods instead. Also as you already noted, the legacy tooling is sometimes generating APIs that are not possible to write in C# source. Personally I don't think its a big problem as I prefer handwritten declarations over the old tooling anytime, and its a one time effort to switch the library. Let me know what you think.
• I noticed your intermediate library claims to be netstandard but has nonstandard dependencies, so is a bit of a hack. You'll want to move away from building a fake netstandard package and do a proper multitarget package because a .NET 8 source generated library won't be usable for Desktop Framework, so you can no longer have one DLL for all frameworks. Fixing the multitarget issues will require some changes to the repo build process, as currently everything outputs into a single folder regardless of target framework, so if you multitarget the different targets overwrite each other.

For now I'll work on just .NET 8 support on the branch but if you intend to keep supporting Desktop Framework (and/or older .NET Core versions) using the same handwritten API then you'll have to figure out how to do the necessary changes to support multitarget builds (shouldn't be hard). From my side it'll not be hard to write them in a way thats consumable from either target framework. I can put the interop declarations into your AxInterop.MSTSCLib as I've been doing so far, or provide the entire API as a separate nuget package external to this repo, I don't really have any preferences, you can let me know later what you prefer.

[awakecoding]

First, thanks a lot @weltkante for taking the time to look into this!

I just built your branch and I'm surprised to see it's already minimally functional with everything rebuilt from source. It's a shame we can't build an API 100% compatible with the original, since backward compatibility is quite important here. We have a lot of code in Remote Desktop Manager that wraps the original Interop interface, and this is the case for pretty much all projects integrating the RDP ActiveX, most of which have their own copy of MSTSCLib.dll, AxMSTSCLib.dll, with or without the "Interop" namespace. Searching for "MSTSCLib.dll" in GitHub is a testament to how pervasive the RDP ActiveX interface is in practice.

This being said, I have refrained from wrapping the generated RDP ActiveX too much in MsRdpEx so far, such that it's still mostly used as a nuget package with clean, up-to-date Interop DLLs + MsRdpEx.dll API hooking for extensibility. Since the newer ActiveX/COM Interop would likely not be generated, we can do it cleanly in another namespace. This way, the original AxInterop.MSTSCLib.dll and Interop.MSTSCLib.dll could still be provided in the nuget package, but I'll find a way to make it a conditional import. My thinking is we can put the Interop code under a separate namespace, probably inside the Devolutions.MsRdpEx.dll assembly, otherwise as Interop.MsRdpEx.dll. I'm open to both approaches and undecided yet, maybe you have a preference for one or two assemblies here. As for AxInterop versus Interop, it always bugged me that we needed two separate assemblies for Interop code, I'm fine with just one.

As for the netstandard2.0 hack, you are correct - there was no simple way to mean "all .NET on Windows + WinForms" which is truly what it is. Interop.MSTSCLib.dll has an hardcoded reference to net40 (!) which throws a warning when imported as "netstandard2.0", but if you add a direct assembly reference to it instead, modern Visual Studio build environments fail to build because it thinks a true net4.0 SDK must be present. This is quite annoying and I'd like just to flip the bytes to make it net48 instead, but I couldn't figure how to do it post-build. I want to keep a clean copy of the original Interop DLLs for compatibility, Microsoft still makes update to the RDP ActiveX interface with some major OS releases, and I'd like to regenerate and update with tlbimp.exe, aximp.exe when that happens, and use it as reference for the modern ActiveX/COM Interop wrappers.

Here's what I suggest: multi-targeting to net48 and net8.0-windows, with conditional compilation options in the code. We just need a good strategy to make it manageable for everything that depends on net8.0 which can't be built for net48. When importing the net48 version of the package, none of the newer COM Interop would be available, only the "classic" stuff. We literally switch from net48 to net8.0-windows last week in the develop branch of Remote Desktop Manager, we will need the old Interop code for quite some time, even in .NET 8. We would only make the switch to the newer Interop code when it is exhaustively implemented and stable, as it is a critical component for us. We can also assume that everyone currently wrapping the RDP ActiveX interface in .NET 8 is using the only Interop code available (old one).

For a .NET 8 project that doesn't bring the old Interop DLLs along, I'll look into adding a condition and a .targets file in the nuget package, such that one could avoid bringing the dependency that is not compatible with Native AOT compilation.

One other question: my understanding is that with the new COM wrappers, it is not possible to define get/set properties on the Interface, which is a bit annoying. This would obviously cause a lot of changes in the consumer code because of this limitation, if we want to build the COM interop code from source. Do you know if there's a design pattern we could implement to wrap the raw interface with a C# class? For Instance, CMsRdpClientAdvancedSettings to wrap IMsRdpClientAdvancedSettings, where CIMsRdpClientAdvancedSettings is a class wrapping the IMsRdpClientAdvancedSettings interface with C# properties. I could see benefits in going beyond what raw COM interfaces can do, even if most of the code would be extremely repetitive. Maybe there's a way to use .NET 8 source generators to help with this?

As for the original interface, there was one place where I need to manually redefine things just to pass the LoadBalanceInfo as a raw BSTR without going through string marshalling, as it needs to be encoded as UTF-8 bytes. I have added the workaround with the interface definition here in the Devolutions.MsRdpEx assembly. We should keep in mind that the newer Interop code should be able to accommodate edge cases like this.

FYI, I know the current code is experimental and handwritten, but if you close the session right now, it throws this error:

If there's a way to semi-generate the Interop code, even with just a PowerShell script, it would significantly reduce the potential of human error when writing the newer interface. That's really what scares me the most, as it may work for simple cases, but we'll really want a way to make sure it is fully correct before we make the switch, and avoid destabilizing one of our most important features in Remote Desktop Manager. This is less of a hard requirement for smaller projects though, which can be more tolerant to some breakage as long as most of the interface works well.

Thanks again for your time! It's really refreshing to have someone really dig into ActiveX/COM Interop like this. I wish to have this repository the go-to project for the RDP ActiveX Interop, with a clean migration path from the older interface to the new.

[weltkante]

Thanks for the comments, I generally understand and agree with you, to not make this overly long I'm just responding to things I have a point to make, if you want to discuss something specific I left out feel free to point it out.

Do you know if there's a design pattern we could implement to wrap the raw interface with a C# class? [...]

Yes, generation of wrappers may be more achievable than generation of interop, since it usually just passes arguments through and doesn't need to transform them. Its one of the options I'm considering, will have to see if I can come up with something simple enough to do myself, but if not its also something someone else can look into more easily than writing or generating interop itself.

[a generator] [...] would significantly reduce the potential of human error when writing the newer interface [...]

As far reliability/confidence is concerned, my suggestion would be not to focus on generating the code but to do unit tests comparing the manual written source - or rather, the compiled classes - against either the old interop assembly or even directly against the TLB. I can contribute some of this testing after I'm done with the initial pass through the interfaces and they can be extended anytime if someone feels the need for more.

That's really what scares me the most, as it may work for simple cases, but we'll really want a way to make sure it is fully correct before we make the switch, and avoid destabilizing one of our most important features in Remote Desktop Manager.

Overall I agree that its important to not destabilize the existing code/product, like you said, the new interop will need attention to make sure (as much as reasonably possible) that its correct - but it'll not be possible to guarantee a switch between technologies without friction as there may be yet undetected bugs in callers, or implicit dependencies on safeguards or behavior that have changed or been removed in the framework itself.

You are probably aware of it, but COM interop is very "dangerous" in that its easy to get semantics and responsibilities wrong. What you might not be aware is that the Desktop Framework interop implementation was never fully correct/stable either, there's always a trade-off between convenience, performance and safety against mistakes. The new implementation is taking different trade-offs in some places, so you will not get full equivalence at the runtime level either, not just at an API level.

For example releasing COM objects via the garbage collector was never possible to do in a perfectly stable way yet most people don't bother to do proper ownership management (even though it was technically possible) until they find themselves in a scenario where its absolutely needed, because its so much more convenient to ignore it. COM allows objects to assume being released on the apartment they were created on, which is something the framework cannot always arrange. Desktop Framework did a "best effort" to properly release references on their original apartment, as far as I can tell from docs and code the new generator does spend less effort on this, leaving more responsibility to the developer if they are working with objects that are "apartment sensitive" in that particular aspect. (I have no idea if thats relevant for the particular API we are working with here.)

My suggestion is to just take this as an experiment for now, see how far we get, and then make your own tests with your private code to be able to make an informed decision whether its worth to actually make the switch. The new interop definitely has advantages, but there are also risks that can't be avoided and need to be handled properly through testing of each consuming codebase using the new interop style. If the risks appear too high its always possible to give feedback to the runtime team or wait a release for the new infrastructure to stabilize and perhaps new features arrive, before evaluating the situation again.

Here's what I suggest: multi-targeting to net48 and net8.0-windows, with conditional compilation options in the code.

Should be possible to work out something as we go, like I said, the main problem at this point is that multi-targeting is not possible because all target outputs go into the same directory. Needs some restructuring of how the project is built, thats all. What needs to be done beyond that to build a decent package I can't tell yet, but you seem to have some ideas.

[...] if you close the session right now, it throws this error

Yeah, noticed this as well but hadn't figured it out when I did the first push, has been fixed now, was missing a conversion between interop styles in the CreateSink method and the exception there did not break into the debugger so I didn't immediately notice it. We'll have to determine later what the risk of these kinds of mistakes are, they happen when passing or retrieving interop references to/from WinForms, as they still use the old style interop. AxHost seems manageable via workarounds, but if references are exchanged with WinForms in a lot of other client code it may need improvements in WinForms, we'll have to see.

(one of my open points) [...] the documentation claims to support the code generators are supposed to be castable into old-style interfaces, but I couldn't get it to work.

Figured this one out, this particular behavior is off by default and has to be opted-in via a project file setting. The catch is that it doesn't work with Native AOT, so I'll keep the manual conversions since AOT mode is of particular interest to you.

[weltkante]

I'm done with the first round through the TLB and got all interfaces written down, not pushed yet, will clean it up a bit and probably push it for the weekend. Also created a WinForms issue to track what needs to be improved there to get the prototype fully working with COM source generators.

As it stands currently, the controls event interface cannot be source generated in .NET 8 as far as I can tell, WinForms simply seems to reject it, even if it implements both source generator and classic interfaces. We'll get the bindings mostly source generated and I'll do PRs on WinForms to improve its support for a fully generated application for .NET 9, we'll have to see how much problems come up on fixing their bugs.

As for the original interface, there was one place where I need to manually redefine things just to pass the LoadBalanceInfo as a raw BSTR without going through string marshalling, as it needs to be encoded as UTF-8 bytes.

I've looked closer at BSTR interop and it seems to support both text content and binary content, while .NET just supports text content marshalling. This is a bit disappointing because it means you cannot use standard marshalling for the cases where the API wants to pass binary data through a BSTR parameter. The question is, if its even possible to identify all methods that might want to pass binary data through a BSTR - if not then my personal preference would be to just not use the standard marshalling at all for any BSTR and do a custom type instead of strings. There is a bit of design space and trade-offs to chose when deciding how the APIs should look like, you still want to be able to easily pass in or retrieve normal strings of course. I'm currently prototyping this along the rest of the interfaces but whether or not to do it, or how exactly the replacement type should look like and behave, can still be looked at later. In particular there's a trade-of between performance and safety against incorrect use to make which I'll explain later when I pushed.

After pushing the interfaces for the weekend, next week I'll be looking at compatibility with the old interop dll (some kind of generator I guess, hopefully simply just forwarding arguments between the two APIs).

[awakecoding]

Regarding BSTR marshalling - LoadBalanceInfo is really the only edge case I am aware of in the RDP ActiveX interface. I'm fine if we have to use something special to make it work - I used an alternative COM interface definition just to get the BSTR pointer as an IntPtr, but I guess we could also try using MsRdpEx extended COM interfaces. However, we then rely on the MsRdpEx API hooking to pass a BSTR which is supported in the regular public interface, and I don't really like that.

I get the feeling that there is no shortcut for COM Interop - we'll need a way to replace the accessors in the interfaces, and a way to pass that BSTR as an IntPtr

[weltkante]

I get the feeling that there is no shortcut for COM Interop - we'll need a way to replace the accessors in the interfaces, and a way to pass that BSTR as an IntPtr

You can just define your own class, including marshalling, you'll see when I push tomorrow. I did a BinaryString class/struct that manages the lifetime, is disposable etc. while implicit cast operator handles the conversion from both string and char/byte spans so it transparently supports all usescases BSTR has (no need for callers to change how they call it). Making it disposable/finalizable allows for safe lifetime management (much better than a plain IntPtr but has a small performance cost because to have a finalizer you have to do an additional heap allocation you don't have with string/IntPtr). If you can "trust" all callers to know what they are doing you can drop some of this convenience/safety and get more performance/less allocations but mistakes will be more fatal or harder to notice.

These custom rules of course deviate from standard interop (and the existing interface signatures) so it slightly increases friction.

While transcribing the interface it looked like a few more strings may be using binary data, maybe you don't use those APIs, maybe they are just normal strings. If we end up not needing custom BSTR handling except in that one place thats easy to change back to standard string APIs, we'll just have to see and try what we think works best.

[weltkante]

• pushed the current state to the branch
• started doing unit tests, comparing the API to the original library (did help finding/fixing a few initial mistakes)
• the required mappings are surprisingly simple
• some compatibility mappings since COM source generators don't support IDispatch/Variant/bool (yet)
• BinaryString is just there because of the experiment to support both string and binary data, could be dropped back to string
• there are two bugs in the automatically generated assembly, see the last three entries in the mapping function, they have links to the docs (which differ from what the generated assembly does for these functions)
• the two event interfaces are still classic COM, they need bugfixing in WinForms before I can switch them
• I probably can get a source-compatible (minus the two API bugs) compatibility layer generated next week

[awakecoding]

@weltkante I'll be back from vacation tomorrow, and I'll next to fix an issue with .NET Framework UTF-8 string conversion in bindings that aren't part of the RDP ActiveX Interface + make a new release of MsRdpEx. It's annoying, but [MarshalAs(UnmanagedType.LPStr)] breaks UTF-8, and the proper UTF-8 support only came in after .NET Framework. I have to maintain and update MsRdpEx for Remote Desktop Manager 2023.3 which is still using .NET Framework 4.8, but even after we've switched to .NET 8 in Remote Desktop Manager 2024.1, I think we'll need to keep .NET 4.8 support for a while. Just recently I've used MsRdpEx in a decompiled Hyper-V Manager replacement.

I'm surprised you could find bugs in the original generated assemblies, I guess nobody noticed them for all that time? I'm amazed at how deep your knowledge is in COM Interop and .NET, keep up the good work. I'm learning a lot just by watching the progress you're making on your branch. Once we have a solution for source-compatible bindings, it should become a lot easier to start merging things.

Speaking of which, do you have a list of changes to MsRdpEx that could be done to help prepare it to receive your changes, given the requirement of backward compatibility with .NET Framework and the old COM Interop? I've mentioned multi-targeting and conditional compilation before + a .targets file for conditional import of specific assemblies in the consumer, this is something I could give a try after I'm done fixing the UTF-8 string conversion issues mentioned earlier. I don't want to be a blocker for this amazing work.

[weltkante]

[MarshalAs(UnmanagedType.LPStr)] breaks UTF-8

Seems obvious to me, that mode uses the encoding for whatever codepage the OS is configured to use (may be set UTF-8 on modern Windows, but usually it isn't defaulting to that yet). Personally I'd always marshal UTF-8 as byte arrays/pointers/spans. LPSTR marshalling should only be used for APIs that expect strings in the current Windows ANSI codepage.

I'm surprised you could find bugs in the original generated assemblies, I guess nobody noticed them for all that time?

The bugs would trigger if you use NotifyRedirectDeviceChange on a 32 bit machine with the interop DLL generated from the 64 bit TLB (or vice-versa), or if you try to make use of the HWND in the RemoteWindowDisplayed event. Maybe nobody needs to do that, or those that do have workarounds, like you have been using the custom interface definition to be able to pass UTF8 through a BSTR.

Speaking of which, do you have a list of changes to MsRdpEx that could be done to help prepare it to receive your changes, given the requirement of backward compatibility with .NET Framework and the old COM Interop?

Changing the project to support multitargeting would be pretty high up the list because its holding me back from adding support for Desktop Framework. Like I said, I don't mind writing it in a way that can support both once the project can support it. It'll probably need a bit of experimentation to find the sweet spot between compatibility and supporting better APIs where available.

Beyond that I currently don't yet have anything else that is important, but once the first pass of the compatibility layer is generated (if all goes well by end of next week) I'll probably turn to debugging WinForms and see how to improve things on their side. You could see if you have any feedback on the compatibility layer and/or APIs at that point.

Also don't worry if you shouldn't be able to get things done in time, we can always take a break and come back to it later.

[awakecoding]

@weltkante I got my UTF-8 issue fixed earlier today - I think what happened is that I broke system code page support when I modified my native code to assume UTF-8 strings everywhere.

I've started a new branch to work on multi-targeting to net48 and net8.0-windows here, maybe you have comments already:
https://github.com/Devolutions/MsRdpEx/tree/multi-target

[weltkante]

took a quick look and seems to work, will try moving the code over and see if I can do multitarget declarations

[awakecoding]

@weltkante I've got a pull request for initial multi-targeting, if it's all good with you, I'd merge that upstream: #101

[weltkante]

Your choice if you want to merge or wait, I don't see anything wrong with it so far. I'll need a day or two to get everything up and running, can do that on the branch as well if you don't want to merge yet. Just rebased my code on top of the branch and got .NET 8 to compile, but of course Desktop Framework needs a bit more work to run off the same codebase (instead of just conditionally compiling the new files only for .NET 8) and I might wait a bit to get it working with the compatibility layer right away.

[awakecoding]

@weltkante I merged it, I just wanted to remove some of the noise. I expect more changes on top of this first pass as you work your way through it, but that's fine, at least we've got initial multi-targeting now