Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ComSourceGenerator-based ActiveX controls #10583

Open
weltkante opened this issue Jan 4, 2024 · 15 comments · May be fixed by #10927
Open

Support ComSourceGenerator-based ActiveX controls #10583

weltkante opened this issue Jan 4, 2024 · 15 comments · May be fixed by #10927
Assignees
Labels
area-COM 🚧 work in progress Work that is current in progress
Milestone

Comments

@weltkante
Copy link
Contributor

weltkante commented Jan 4, 2024

.NET version

.NET 8

Did it work in .NET Framework?

Yes (using classic COM interop)

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Works using the classic COM interop.

Issue description

TLB support and tooling isn't ready yet in .NET 8 so its understandable that there has been limited attention to support the new COM source generators in WinForms, but there is still desire (dotnet/runtime#66674 (comment)) to get it working eventually. I'm currently prototyping (Devolutions/MsRdpEx#97) a hand written binding for the Microsoft RDP Client, which is apparently only provided in form of an ActiveX control, so should be a decent usecase and test scenario for ActiveX support in WinForms against the new COM source generators (besides the custom MFC based controls we previously used for testing compatibility, I might also extend those to provide test coverage, not sure how well a RDP client control is suited for unit testing)

A few things to note so far, I'll probably add more as the prototype comes along:

  • returning a ComWrappers instance from CreateInstanceCore makes WinForms fail to resolve its interfaces
    • according to the docs this is supported by the runtime, but its off by default and has to be opted-in and is not NativeAOT compatible, so I do not opt-in and do things manually
    • it is possible to work around this by manually translating it to a classic interop COM object (__ComObject instance obtained via Marshal.GetObjectForIUnknown on the ComWrappers IUnknown pointer)
    • ideally WinForms will support either being returned here, maybe probing which variant it is or something, I'll experiment and see what I can get to work here
  • passing a ComWrappers instance to ConnectionPointCookie makes WinForms fail to understand it
    • the current ConnectionPointCookie API is not well-suited to custom bindings since it wants an interface type for reflection
    • even if the event handler interface implements both ComWrapper and classic COM interfaces WinForms still fails

My primary goal of this issue will be tracking required changes to support getting the prototype running entirely with handwritten source generator bindings, I'll open a PR for suggested fixes once I figure out more about why WinForms fails interacting with ComWrapper based instances.

Steps to reproduce

Write COM source generator bindings against an ActiveX control and try to use it with WinForms AxHost

@weltkante
Copy link
Contributor Author

tried to tag a few issues I could find that seem related, if you have anything else in scope of this let me know, and I can see if it makes sense to contribute as part of the prototyping work

@elachlan
Copy link
Contributor

elachlan commented Jan 4, 2024

@weltkante I am unsure if it solves your problem directly, but have you looked into CsWin32? Winforms uses it, but the team has had to write a lot of code for it to function properly. The advantage here is that it should enable AOT for winforms.

@JeremyKuhne
Copy link
Member

@weltkante we plan to finish making AxHost trim friendly. I also made ComNativeDescriptor trim friendly and ComWrappers friendly in .NET 8.

An initial thing I wanted to do was to provide something that would make manually writing AxHost derived classes that are trim-friendly easier. I've messed with this concept in my "thirtytwo" project, which does not use any legacy COM interop. The sample I was working on was this:

    private class MediaPlayer(Rectangle bounds, Window parentWindow, nint parameters = 0)
        : ActiveXControl(CLSID.WindowsMediaPlayer, bounds, parentWindow, parameters)
    {
        public string? URL
        {
            get => (string?)GetComProperty("URL");
            set => SetComProperty("URL", value);
        }

        public bool StretchToFit
        {
            get => (bool)(GetComProperty("stretchToFit") ?? false);
            set => SetComProperty("stretchToFit", value);
        }
    }

I haven't handled events yet, but I am intending to.

My presumption is that we'd provide a source generator that does something akin to what AxImporter does. AxImporter has quite a bit of code behind it (many thousands of lines), including a lot of native runtime code via TypeLibConverter. None of it has been ported.

My hope was that we could just start with the "basic" scenarios and keep layering in support.

@weltkante
Copy link
Contributor Author

@elachlan as far as I know thats for generating interop against (known) headers? what I'm trying to do is consuming ActiveX controls, those are defined in TLBs and historically had tooling build into the framework (TlbImp, AxImp) to generate .NET DLLs with the interop code. However TLB support didn't make the cut for .NET 8 so no tooling yet.

If CsWin32 supports TLBs for builtin controls (some do have headers after all, so might not be unreasonable) then I'm unaware of it, don't really know how to check easily.

Either way, TLB tooling ist just convenience, it has always been possible to write the interop declarations manually, so thats what I'm doing with this prototype. The main goal is to test out the runtime support (of .NET and WinForms) and get things tested/fixed early without waiting for the TLB tooling. I've already transcribed the TLB by hand (it wasn't that big thats why I picked it) so looking at CsWin32 now wouldn't help much as far as the prototype/testing is concerned. Might keep it in mind for later though.

The advantage here is that it should enable AOT for winforms.

Writing declarations by hand (for the COM source generator to consume) also enables NativeAOT, which is the whole point of this exercise, people wanting to try it out.

Besides the missing TLB support (which I can overcome easily) WinForms is the one stopping the attempt, as far as I can tell because its not fully done switching away from classic COM interop. If I need to make a PR to port more WinForms-internal code over to using CsWin32 I'll discover that when I get around to debugging the issues described above and make a PR.

@JeremyKuhne
Copy link
Member

@weltkante on the off chance you know how to create a sample of hosting a WinForms .NET 8 control in MFC I'll be in your debt and able to spend some time looking at this stuff for a bit. :)

@weltkante
Copy link
Contributor Author

weltkante commented Jan 4, 2024

@weltkante on the off chance you know how to create a sample of hosting a WinForms .NET 8 control in MFC I'll be in your debt and able to spend some time looking at this stuff for a bit. :)

I probably can get that to work, got it to work in earlier .NET Core releases with the old interop. I also found your failed VB6 unit test and will give it a try to debug, we still have a bit of VB6 legacy code in our products so I'm quite used to having to deal with those pains 😅 If you have anything specific beyond that feel free to leave a note in one of the appropriate issues, maybe the unit test issue.

My presumption is that we'd provide a source generator that does something akin to what AxImporter does. AxImporter has quite a bit of code behind it (many thousands of lines), including a lot of native runtime code via TypeLibConverter. None of it has been ported.

yea, thats what I'm hoping for, the manual transcribing I'm doing here is for testing things early and getting the runtime tested and working before all the generation stuff is done

@JeremyKuhne
Copy link
Member

@weltkante The VB6 unit test thing we investigated briefly. The tests work fine on their own, no matter how many times you run them. Running with other tests at scale causes memory corruption. Runtime folks looked at it with me, but without a user repro we had to shelve the investigation after sinking some time into it without luck.

@weltkante
Copy link
Contributor Author

weltkante commented Jan 4, 2024

@weltkante on the off chance you know how to create a sample of hosting a WinForms .NET 8 control in MFC I'll be in your debt and able to spend some time looking at this stuff for a bit. :)

whats wrong with https://github.com/dotnet/winforms/tree/main/src/System.Windows.Forms/tests/IntegrationTests/NativeHost and NativeHost.ManagedControl in the same folder?

I mean besides it crashing ;-) It used to work, now crashes with an AV in IDispatch from MFC to .NET. Did you want me to debug what broke in the interop (I'd probably try a TTD debug session to see what happened to that AV'ed address) or just needed a reminder we already had a sample? (To be fair, took me a while to remember we had one too, already started writing a new one but the docs I had to look up seemed awfully familiar ...)

@JeremyKuhne
Copy link
Member

@weltkante Well, that's embarrassing. 😆 If you get a chance to look at the break that would be fantastic.

@weltkante
Copy link
Contributor Author

@JeremyKuhne setting the Debugger to "mixed .NET Core" on NativeHost lets you debug from VS, by default it is native only because .NET Core gets loaded later when the ActiveX Control is loaded.

The crash is happening here because it tries to debug-print a NULL message pointer (MFC explicitly passes in NULL as you can see in the MFC source when you go up the call stack, caller is COleControlSite::DoVerb)

  • Fixing this lets the sample UserControl load into the MFC host.
  • Note that the MSG struct doesn't even have a ToString anymore so the debug print is useless

Due to the latter I'll not do a PR since I'm not sure how you want to fix the diagnostic messages or remove them.

@JeremyKuhne
Copy link
Member

Thanks @weltkante, I'll take care of it.

@JeremyKuhne
Copy link
Member

#10599

@merriemcgaw merriemcgaw removed the untriaged The team needs to look at this issue in the next triage label Jan 11, 2024
@merriemcgaw merriemcgaw added this to the .NET 9.0 milestone Jan 11, 2024
@merriemcgaw
Copy link
Member

Assigning to @JeremyKuhne for an initial spike and investigation. We'll report back soon.

@JeremyKuhne
Copy link
Member

@weltkante I did confirm that I could handle events in an AOT friendly TypeDescriptor as I've done in the thirtytwo project I linked above. Complex types wouldn't work as we couldn't generate anything (enums would have to be their underlying type, for example). It wouldn't be particularly performant at startup as we'd have to walk the entire graph to build the TypeDescriptor. It would take some effort to implement this TypeDescriptor, but it could be done incrementally.

Another possible option: If we get a public VARIANT type from the runtime maybe we could provide a low-level AxHost derived class where we just provide some basic IDispatch related virtuals. This could be done both for outgoing calls and callbacks.

@weltkante weltkante linked a pull request Feb 21, 2024 that will close this issue
@dotnet-policy-service dotnet-policy-service bot added the 🚧 work in progress Work that is current in progress label Feb 21, 2024
@weltkante
Copy link
Contributor Author

weltkante commented Feb 21, 2024

I think I've identified the issues holding back running AxHost with <BuiltInComInteropSupport>false</BuiltInComInteropSupport>. I'm making a draft PR so you can have a look at it. If the ATL ActiveX control PR eventually goes through I would also be able to add unit tests for this one, but for now its just "proof of concept".

If we get a public VARIANT type from the runtime

They added VARIANT interop via ComVariant (which I used in the PR to replace classic VARIANT marshalling in one place)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-COM 🚧 work in progress Work that is current in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants