dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 984 forks source link

Hosting UserControl via ActiveX does not work correctly #4370

Closed arknu closed 2 years ago

arknu commented 3 years ago

Problem description: I am experimenting with doing Office Addins without VSTO and using .NET 5. So far, I have succesfully manged to add ribbon buttons and I'm now working on a custom TaskPane, hosting a Winforms UserControl.

So far, so good. However, when instantiating the UserControl, an exception is thrown here: https://github.com/dotnet/winforms/blob/cb03d37fe73fb178d3c3827ad895bb3e7c50fe4c/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXImpl.cs#L841

_inPlaceUiWindow is null here and there is no check for that case.

Comparing to .NET Framework 4.8 reference source (https://referencesource.microsoft.com/#System.Windows.Forms/winforms/Managed/System/WinForms/Control.cs,17065), the implementation is different and I suspect that a regression has snuck in.

Expected behavior: No NullReferenceException should be thrown.

Minimal repro: You can download the project here: https://1drv.ms/u/s!AkwgkpetCvCpr4JeJeZfCGlKlHs4qA?e=PeB0So You need to manually run regsvr32 OutlookAddinNet5.comhost.dll in the bin folder and register the addin in outlook:

Windows Registry Editor Version 5.00

[HKEY_CURRENT_USER\SOFTWARE\Microsoft\Office\Outlook\Addins\OutlookAddinNet5]
"CommandLineSafe"=dword:00000001
"Description"="OutlookAddinNet5 description"
"FirendlyName"="OutlookAddinNet5 name"
"LoadBehavior"=dword:00000003

Then Open Outlook and click the [2] button added to the ribbon.

weltkante commented 3 years ago

I am experimenting with doing Office Addins without VSTO and using .NET 5.

Unfortunately there is no interest to support that scenario from the Office side (see e.g. #1763) so while the last statement from the WinForms team is to be willing to look into and fix bugs, you'll probably have a tough road ahead of you to get it working.

As far as Office integration support is concerned you cannot rely on compatibility of WinForms in .NET Core with .NET Framework since the Desktop Framework was using undocumented "Component Manager" integration with Office, and the Office team is not providing support to WinForms in .NET Core to support it properly (#247).

This is just FYI and no argument against fixing the problem you found.

Also, aside from above, TLB support has been dropped from COM interop in .NET Core so you'll either have to reinvent that or use Desktop Framework to generate the TLB for scenarios that need it. The new COM interop layer currently being built will most likely not support TLB out of the box either.

arknu commented 3 years ago

@weltkante Thank you for the context, wasn't having much luck searching. The Office team really needs to wake up and realize that HTML/JS is fine for simple addins, but complex addins with actual functionality need a proper language and runtime with full system access, i.e. .NET and C#. VSTO needs a path forward, even if that is just doing raw COM addins with C#.

My impression was always that since Office addins under the covers are just COM and WinForms is just a Win32 wrapper, it should "just work", once you implement the correct interfaces and get Office to load you addin. If you can do a native addin in C++ and COM, surely WinForms should work without extra support from the Office team. As long as they support COM addins...

However, I am not sure that this bug is anything Office-specific. This appears to be a more general regression in the COM/ActiveX interop that I just happened to hit doing my Office addin experiments.

weltkante commented 3 years ago

However, I am not sure that this bug is anything Office-sepcific.

No, definitely not, ActiveX is separate and can be hosted outside Office as well, just wanted to inform you that you are likely to run into more (possibly subtile) problems.

surely WinForms should work without extra support from the Office team

Unfortunately WinForms in Desktop Framework integrated with Office to communicate form modality (WPF does not). The WinForms team didn't agree to remove that code (see discussions in the linked issues for their reasons), so since it isn't supported or tested in any way regressions are possible (and potentially subtile since they relate to how Office behaves when you show a modal WinForms form).

If you have the resources to see your project through that surely would help improve quality, we have this kind of Office Addins as well but currently all our .NET Core efforts are halted until custom control support is finished, so it may be a while before I get around to come back to rewrite and test our own addins.

RussKie commented 3 years ago

Thank you @weltkante. @arknu VSTO interop story is currently a grey area, even for us. That said, if you stumble across any bugs in our codebase - feel free to send fixes in :)

arknu commented 3 years ago

@RussKie I don't believe this has anything to do with VSTO, so that label should be removed. In fact the original post explicitly states that I am not using VSTO at all. This is an ActiveX/COM interop issue that just happens to surface in Office, because that was where I was using it.

The code in question is missing null checks. Every other use of _inPlaceUiWindow in the code mentioned above has null checks, except this one. That seems like a mistake.

1R053 commented 3 years ago

I just had a look at the source for this in InPlaceActivate(OLEIVERB verb). There is a call to

HRESULT hr = _inPlaceUiWindow.GetWindow(&hwndParent);

at a location in the code where _inPlaceUiWindow has never been instantiated. This piece of code cannot work anywhere.

1R053 commented 3 years ago

created a PR #4626 imo it was a copy & paste error when porting this code. In case this goes through, will it be backported to 5.x or should this be a separate PR?

arknu commented 3 years ago

@1R053 Thank you for creating the PR. It certainly fixes the NullReference exception for me. However, there appears to be other issues resulting in the ActiveX control not being created - at least when running in Office (via the ICTPFactory.CreateCTP method). I don't get any exceptions or details, just "The ActiveX object could not be created". However, those issues are probably best discussed in a separate issue, which I'll create once I have done some more investigation.

@RussKie Are you interested in having ActiveX hosting work correctly in .NET 6? If so, I'll be happy to work with you to test this. I have a proof-of-concept Outlook addin, which I can provide the code for, if needed. My specific scenario is Office, as stated previously. I suspect this will be the most common case these days, but I don't think any of these issues are Office-specific.

1R053 commented 3 years ago

@arknu Thanks - I've observed this as well. It would be great if you can assist in identifying the next issues.

1R053 commented 3 years ago

after the fix I think the class type of the control cannot be identified somehow. When looking into Internal.Runtime.InteropServices.ComActivator->FindClassType there seems to be no type returned in my case

arknu commented 3 years ago

@1R053 For me it actually gets as far as calling the constructor of the UserControl now. Definitely progress. But after executing the constructor successfully, it fails. I have a feeling that there may be another copy-paste bug lurking somewhere...

1R053 commented 3 years ago

@arknu Can you register/unregister the comhost.dll with Core 6? I had also issues here. If it was already registered before I also had a successful instantiation but no success with ICTPFactory.CreateCTP

RussKie commented 3 years ago

Are you interested in having ActiveX hosting work correctly in .NET 6? If so, I'll be happy to work with you to test this. I have a proof-of-concept Outlook addin, which I can provide the code for, if needed. My specific scenario is Office, as stated previously. I suspect this will be the most common case these days, but I don't think any of these issues are Office-specific.

Our current team is lacking in understanding how Office interop and ActiveX are used in the wild, and we're very interested in sample apps that can demonstrate real scenarios. Having these scenarios we'll help us building a test suite and identifying gaps, and may aid in cross-org discussion about the future of the interop.

I can't make any promises on the future support of VSTO and Office interop, but the more we know how our users use (and abuse :)) this area - the more we may be able to help.

arknu commented 3 years ago

@RussKie As for my use case, I'm currently attempting a proof of concept to determine whether whether we can do an Outlook addin without VSTO, since there apparently no desire to update VSTO (which is a shame!). We currently have a fairly complex commercial VSTO-based addin, and no desire to be stuck on a dead .NET Framework with outdated libraries - and no desire to use a subpar language like JS. OfficeJS is not even remotely capable of doing what we need. So we are looking to do an addin using raw COM APIs directly from .NET. This should be possible and mostly is. The only issue is showing a task pane, which uses a WinForms UserControl hosted as an ActiveX object. So I have created a .NET 5-based COM addin that works perfectly - except for showing the task pane, where we run into various issues, as detailed above.

So if we get these issues fixed, we will be using Office interop and COM for a full-fledged commercial Outlook addin.

RussKie commented 3 years ago

If you have any repros to share but don't want those to be public please feel free to email me at Igor.Velikorossov at microsoft.

arknu commented 3 years ago

@RussKie What I have is a proof of concept, so I have no issues sharing it publicly. I'll upload it to GitHub later this week and post a link here - I just need to clean it up slightly first.

arknu commented 3 years ago

@RussKie As promised, I have now uploaded my proof-of-concept to a GitHub repository: https://github.com/arknu/OutlookAddinNet5. It is pretty much as simple as can be - adding a few buttons to the ribbon and attempting to show a taskpane with a UserControl. This last part being what fails.

It is currently based on .NET 5, haven't tried the .NET previews yet.

I'm running it with a private patched build of WinForms, which includes @1R053's patch for the NullReferenceException.

weltkante commented 3 years ago

I'll have a look

weltkante commented 3 years ago

ok, found two issues:

weltkante commented 3 years ago

Can you register/unregister the comhost.dll with Core 6? I had also issues here.

@1R053 I've successfully done that while debugging the above, but I had to install the 32 bit SDK for some reason to get the addin registering in a 32 bit office installation (just building 32 bit worked via 64 bit SDK, but registering did not; I did have both 32 bit and 64 bit runtimes installed) - I wonder if these addins even register via regsvr32 on machines where the SDK is not installed in the first place?

If regsvr32 is broken without an SDK installed matching the bitness of the DLL then it probably needs to be reported on the dotnet runtime repo.

Also note that AnyCPU builds probably won't work for COM libraries, since the comhost.dll is likely to be a native DLL so you need to build twice for specific bitness to get both variations of the comhost.dll if you want to support both 32 bit and 64 bit office. For Desktop Framework this was not necessary because mscoree.dll, the equivalent of the comhost.dll, was built into the framework and available in both bitness variations out of the box.

arknu commented 3 years ago

@weltkante Thank you for taking a look at my code. Adding the interface and the ComDefaultInterface attribute fixed the problem. I can now show a custom task pane with a WinForms UserControl: image

Regarding the other issue you mention, that does seem a bit suspicious, yes...

Regarding regsvr32 - I'll do some testing in a few VMs and report back in the coming week. I'll then file a new issue in dotnet/runtime if required.

Now that I have working code with .NET 5, I'll switch to testing with the .NET 6 preview and report back here.

1R053 commented 3 years ago

Thanks @weltkante - the task pane looks good now with this fix!

arknu commented 3 years ago

@weltkante @RussKie A quick question: I have now made a .NET 6 version of the proof-of-concept and I noticed something strange: I cannot load both the .NET 5 and 6 versions in Outlook at the same time. Either one alone loads fine, both both at the same time will cause the other one to fail to load. Is this supposed to be supported? The two addins have different namespaces and GUIDs, so from what I can see it should work.

1R053 commented 3 years ago

@arknu besides the COM namespaces - did you also change the addin name under Software/Microsoft/Office/Outlook/Addins?

weltkante commented 3 years ago

Is this supposed to be supported?

Probably not, last statement I've seen about this was that it is not supported to load .NET more than once, since it uses global resources (like named mutexes or whatever) loading multiple instances of coreclr into the same process cannot work. They are aware of this restriction and considering lifting it, but I don't know what the progress is on that. If its not been worked on yet you will run into problems if more than one addin uses .NET Core, but really this is a question for the dotnet runtime team so you probably should post there to ask for the status of their support.

arknu commented 3 years ago

@1R053 Yes, Outlook has two distinct addins: image

But the .NET 6 one has a COM runtime error when loading: image

However, if I disable the .NET 5 addin (by changing LoadBehavior to ), the .NET 6 addin loads just fine.

So, it seems to have an issue with loading multiple COM-hosting versions of .NET at the same time... Which I would consider a bug. Should I file an issue in dotnet/runtime to discuss this further?

1R053 commented 3 years ago

if this does not work, it would be a big issue imo. Since this would mean that two addins from two vendors that are usually not synced with respect to their .NET version would crash each other.

weltkante commented 3 years ago

See for example dotnet/runtime#12018 (which is quite old) so its probably worth to ask again and present your usecase.

if this does not work, it would be a big issue imo

definitely, you'd risk your addin not being loadable by relying on .NET core if someone else gets loaded earlier

would crash each other.

I'd hope it'd just not load, not crash the Office process. If it crashes it may be worth fixing that crash independently from adding side-by-side support.

1R053 commented 3 years ago

Maybe a custom shim instantiating the runtime with a custom assembly context could work https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext?view=net-5.0

does somebody have an example for a custom shim with .NET Core?

weltkante commented 3 years ago

Maybe a custom shim instantiating the runtime with a custom assembly context could work

No at the end of the issue I linked above they mention they have been using global resources. If they didn't change that yet it won't work at all.

You'd need to move your addin out of process to make the runtime resolve uniquely, and have a C++ COM addin as a shim (or any other language that can load side-by-side)

We actually do something very similar for different reasons, on Desktop Framework WinForms already is a shared resource and we ran into problems because other addins may have loaded the same 3rd party WinForms controls we use before us with a different global configuration.

Therefore we moved all our UI into an out-of-process COM object, keeping an in-process addin to communicate with Office and shim all calls that do UI to our out-of-process objects. This is nontrivial due to how COM behaves when crossing apartmant boundaries, especially message pumping happening to wait for results of the call requires low level workarounds (you don't want message pumping when the ribbon is being loaded for example, so we do the out-of-process COM calls on the thread pool, with special non-pumping code to wait for results, being careful that we don't have deadlocks).

If we were to port to .NET Core we could probably update that approach and make the in-process addin C++ only until .NET Core actually supports loading side-by-side (though I don't know if we would take that step, considering we prefer coding in C# where possible)

weltkante commented 3 years ago

Personally I'd suggest opening another issue on the runtime, I mentioned COM addins as a major problem in the linked issue, and missed when that issue got closed later. Having the runtime unable to load side-by-side basically makes it useless for implementing COM objects.

1R053 commented 3 years ago

You'd need to move your addin out of process

@weltkante do you have a C++ example project that does this with .NET Core? That could be the base for a custom shim if I understand you correctly.

arknu commented 3 years ago

@weltkante Thank you for all your effort on this. I have now filed https://github.com/dotnet/runtime/issues/49686 for discussing the issue of multiple versions of .NET with COM hosting.

arknu commented 3 years ago

To get a little more back on topic, I have now created a .NET 6 version of this proof-of-concept. It loads OK (once you disable the .NET 5 version), but the UserControl in the task pane is non-interactive. It looks like mouse messages or something are not being processed correctly. It loads fine, but does not react to mouse movement or clicking the button. It works in .NET 5. Should I file a new issue for this problem or is it perhaps related to the known issue with PropertyGrid for .NET 6 preview 2?

weltkante commented 3 years ago

@weltkante do you have a C++ example project that does this with .NET Core?

No, nothing finished anyways, ours is currently C# Desktop Framework (which is able to load side-by-side) but it wouldn't be too hard to create one, its just a bit of work. You'd probably use ATL or WRL to implement those COM objects and IDL to define the interfaces (you need a TLB to have COM talk cross-process, which .NET does not support generating anymore, so you have to define all out-of-process accessible interfaces in IDL you compile to a TLB and [ComImport] them, instead of using [ComVisible(true)]).

I might consider making an example but it would probably take a while (time scale would probably be more than a week but less than a month)

It loads fine, but does not react to mouse movement or clicking the button.

I believe that worked for me when I tested, but I may mix up memory of testing in the MFC host which I also tried. Also note that there still is #4685 returning wrong flags to Outlook if you didn't patch it (I did in my tests) so it may be related to that regression.

I'll look at it again later today.

Should I file a new issue for this problem

Not speaking for the team (which may have a different opinion) but I'd probably just rename the issue to 'hosting UserControl via ActiveX does not work' and we should try getting the basic functionality working in this issue. Makes not much sense to leave it in a partially working state, we should try to get the basics functional again.

arknu commented 3 years ago

@weltkante I agree, I've renamed the issue accordingly.

I haven't applied your latest patch to .NET 6 yet, so that may certainly be related. That said, I didn't apply it to my custom .NET 5 build and that works fine...

1R053 commented 3 years ago

Thanks @weltkante - I think the COM / IDL side I could manage. What would help would be to understand how the equivalent of metahost->CLRCreateInstance, metahost->EnumerateInstalledRuntimes or metahost->GetRuntimes is properly handled for .NET Core in a C++ project.

weltkante commented 3 years ago

What would help would be to understand how the equivalent of metahost->CLRCreateInstance, metahost->EnumerateInstalledRuntimes or metahost->GetRuntimes is properly handled for .NET Core in a C++ project.

You don't need to, out of process hosts are an .exe so you can just make a normal .NET exe and it'll activate .NET Core itself.

You specifically wouldn't want to load the .NET runtime in the C++ shim since it'd fail anyways if someone already loaded a different version. The C++ shim would just mediate between COM objects (Office and your out-of-process COM objects) and not need to be aware of .NET at all.

PS: if you want to discuss more of that I'm also on gitter, might be getting a bit off topic for this issue.

merriemcgaw commented 2 years ago

Is there anything else we can do with this issue? Is there more work that we need to do?

weltkante commented 2 years ago

Personally I'd suggest to move "implementing ActiveX controls in WinForms and hosting them in external environments" into "unsupported" territory

PS: the reverse scenario, hosting external ActiveX controls inside a WinForms control or form, is supportable.

also if there are bugs reproed/identified they can still of course be fixed, but the overall scenario itself cannot be supported given the current capabilities of dotnet, its always going to be a hack job for the one using this capability

1R053 commented 2 years ago

What is then the recommended architecture to host user controls in taskpanes of Office COM addins?

weltkante commented 2 years ago

Office COM addins are no longer supported in context of .NET Core, and there's no plan to restore support

So if this is a requirement you have to remain with Desktop Framework or chose a different language like C/C++ for implementing the panel.

See also here and here for mentioning that VSTO is discontinued and will not be updated for dotnet core. Generally the Office team doesn't seem to like that people still use these old technologies.

I don't like the situation myself, as I maintain a COM Office Addin myself, but I can only report what the current situation is.

RussKie commented 2 years ago

Adding @AaronRobinsonMSFT for general awareness.

1R053 commented 2 years ago

Office COM addins are still supported by MS Office and I do not expect that to go away soon, especially since the JavaScript based Office addin infrastructure lacks relevant features for more advanced scenarios. That .NET Core does not want to support it is very unfortunate for this not unimportant use case. The lack of multiple runtime support also means probably, that any vendor trying to establish an add-in ecosystem cannot really use .NET Core to develop their software, as they inevitably would run into similar issues. That seriously limits .NET Core usefulness for more advanced scenarios.

Anyway, thanks for pointing that out.

ghost commented 2 years ago

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.