rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Discussion: COM Shims #4041

Open mansellan opened 6 years ago

mansellan commented 6 years ago

Currently, Rubberduck loads directly into the default AppDomain provided by the host application Add-In manager. Microsoft recommends against this approach, advocating instead for the use of a COM shim. This is a thin proxy, written in native code (C++) which registers itself as an Add-In and when loaded initialises an AppDomain, loads the managed assemblies into it, then forwards calls from the host to the extensibility interfaces across to the managed assemblies. Full details on how this works are available here.

Implementing this for Rubberduck poses a number of challenges, some of which relate to its nature as an open source project:

So we have a couple of options:

1. Implement COM shims

Pros:

Cons:

2. Keep the status quo

Pros:

Cons:

As this idea has been floated a couple of times now (including #3837), it seemed appropriate to start a disucssion.

bclothier commented 6 years ago

Just to keep things in perspective:

Having 2 shims is not a problem per se because we already use separate registry paths for each 32-bit and 64-bit. It would be a matter of handling it within the installer script since we now install both 32-bit and 64-bit unconditionally. As a matter of fact, we already are exporting both 32-bit and 64-bit TLB files for that reason.

The documentation on COM shims are somewhat dated and as such, need to be taken with a grain of salt. For example, it talks about improving code but .NET 4 has basically killed off the CAS policy. It's not clear from documentation whether that changes the requirements for code changing but my impression from skimming is that Office security is the only thing that matters, and as long we satisfy the Office's requirements, we should be OK without worrying about any previous CAS requirements.

My major concern with a COM shim, however, is that it has the potential to hide the bugs or flaws that might need to be addressed anyway. For example, we currently have a memory leak which causes an exception on the CorExitProcess, which indicates we are still missing some things and failing to release things can still affect the host in which case, the isolation from other add-ins doesn't any of the add-ins.

bclothier commented 6 years ago

There is one more con against the shim --- this would require contributors to set up a C++ development environment if they want to build using shim. We could sidestep that for a debug build but that adds more complexity to the build process which is already complex, as it would need to be able to conditionally change the registry script and also to skip building of the shim project without errors.

I'd rather not make Rubberduck project harder to contribute by requiring that any potential contributors have a C++ development environment.

PeterMTaylor commented 6 years ago

Just agreeing with @bclothier. Less maintenance from C++perspective is ideal but if someone on the team is a subject matter expert for support might be a fair compromise in that direction.

mansellan commented 6 years ago

For me the need for code-signing of all assemblies kills this idea. The benefits just don't seem to justify the costs.

carlos-quintero commented 6 years ago

Hi,

Please reconsider using a COM Shim. If add-ins for VBA don't use COM shims, they collide with InvalidCastException (#3837), even if both add-ins use the same Microsoft.Vbe.Interop PIA (because they would provide it from different locations, since that PIA is not installed by default in the GAC by Office in all versions/setup scenarios). MZ-Tools provides a COM Shim since June 1, 2016, so I am not affected, but there are other .NET-based add-ins for VBA out there. Also, if I wouldn't have provided such COM Shim, both you and me would have had some bug reports each month... So, every author of add-ins for VBA should provide its own COM Shim.

That said, I will try to address your cons:

bclothier commented 6 years ago

Considering Carlos' comments, I would point out that if we decide to adopt shims:

1) We might consider maintaining it in its own solution, apart from the current Rubberduck solution. This way, contributors wouldn't necessarily be required to have C++ build tools; we would use the Rubberduck.Deployment to distribute the compiled output of the shim project. That way, having only C# build tools is sufficient for anyone to contribute to the Rubberduck project, and we want to keep that way.

2) Since even if we do use shims, we still have to support scenarios where installing without shims is preferred. In that light, the ideal solution is to support a drop-in replacement where changing between a shim and non-shim is simply editing the registry key. That would give both developers and users the most flexibility over whether they want to use it or not.

The fact that code signing isn't required is a very good news and removes a major roadblock for using a shim.

I do not feel that we have adequately addressed the concerns about hiding bugs/flaws because I'm more concerned about issues that is only observed under one setup but not under the other. If we have a bug that only affects us when developing without a shim, we might not even notice that bug and users will never report that bug, unless they install without shim. Likewise, if the bug manifests itself only when the shim is loaded, someone will launch the debug with the shim enabled to diagnose it. The main point is that it has the potential to add complexity to the troubleshooting process. Being a community project, it can be a good way to make it less-than-fun to work and maintain. Keep in mind that we already have a complex build system. For example, we conditionally compile the binaries using customized IDLs file vs. using the tlbexp to export the COM visible types. This enables features that are otherwise not possible using pure C# toolchains (e.g. ability to rename an enum member or to decorate some attributes such as restricted which is not possible using COM Interop alone). Though it only changes a small part, it does enlarges the surface area where bugs can occur but only manifest itself only under a particular setting. If we do proceed to adopt shims, we should ensure that we provide this due consideration (e.g. beefing up the diagnosis/trace logging or something like that).

carlos-quintero commented 6 years ago

Thanks for considering it. Based on my experience, once you have the COM Shim properly coded, it is transparent. I spent some time (Jun-Sep 2016) until I coded it correctly, but since then I have not received bug reports about it. I documented all the issues in some posts of my blog but it was mainly because my add-in can run on CLR 2.0 or CLR 4.0. Since you are running always on CLR 4.0 you should not suffer those issues.

rubberduck203 commented 6 years ago

Early on in the project, Carlos helped us through some show stopping issues around plug-in development. He’s absolutely a subject matter expert here and I’d highly encourage the community to take his advice on this.

mansellan commented 6 years ago

I'd be happy to look into this, although I would have to start learning C++ at the same time.

rubberduck203 commented 6 years ago

I know a bit of C++ and can be available via email if you need a hand. (Just remember that I don’t know squat about COM shims and might have to set up a Windows VM!)

carlos-quintero commented 6 years ago

@mansellan @rubberduck203. IIRC, the approach that I used was:

  1. Run the COM Shim Wizard 2.0 for VS 2005. Note: you don't need VS 2005, it's a wizard that generates VS 2005 projects that you can convert to VS 2017. You need a non high-DPI display, though. At this point, if you register the COM Shim as VBA add-in, you can test the Very High or High macros security settings to ensure that not using code signing is not a problem.

  2. For VBA 32-bit: Run the COM Shim Wizard 2.3.2.0 for VS 2010 supplying the same values and GUIDs. Again, VS 2010 is not required. In theory you would use the code generated by this wizard, but it uses "COM aggregation" instead of "COM containment". I figured out that aggregation is not needed for VBA add-ins, just containment (a C++ VBA add-in that calls internally a .NET VBA add-in). Comparing to the code of COM Shim Wizard 2.0 of step 1 (hence the need to use the same values and GUIDs) you can figure out which code to remove to get rid of the unneeded ManagedAggregator.

  3. For VBA 64-bit: repeat step 2 with different GUIDs and change the project properties to convert to 64-bit.

  4. Refactor projects of steps 2 and 3 to reuse code files with conditional compilation and #define to supply different values.

  5. For VB6 (32-bit): Repeat steps 2 and 4 if you target also VB6, not only VBA 32-bit and VBA 64-bit.

It seems messy but I managed to make it work with no knowledge of C++ or COM / ATL, just a bit of rusty C to create .h files and #define macros. The key is that the wizard generates code that builds, so perform small changes while maintaining the compilation without errors.

The remaining tasks (setup script, show the AppDomain name in the About window for diagnostics, PowerShell scripts to register with or without the COM Shim, etc.) are quite trivial.

Remember that you need this tooling:

“.NET Framework 4.6.1 SDK”: otherwise mscoree.h / mscoree.lib are not found “VC++ 2017 v141 toolset (x86, x64)” “Visual C++ ATL Support”: otherwise atlbase.h, etc. are not found “Windows 10 SDK 10.0.10240.0”

If you decide to start all this and need specific help let me know.

mansellan commented 6 years ago

That's awesome, thanks so much for the roadmap!

bclothier commented 6 years ago

For interest, this suggests a single C++ project can be used to compile both 32-bit and 64-bit output from a single project. Not sure if that's practical but something worth checking.

bclothier commented 6 years ago

Linking issue #4283 as it seems to be also affected by the lack of shim. Being just AutoCAD, it is not subject to the issue Carlos identified with the Inventor.

Update: On further investigation, there is no other add-ins loaded so it might not be related after all?

bclothier commented 4 years ago

I thought it was already mentioned somewhere but I can't find it ATM. For the record, we need to weight whether we want to bother with COM shims. With .NET Core 5 coming out, it will significantly change how the assemblies are loaded, and because .NET Core 5 would have its own "shim" DLL directly generated rather than using mscoree.dll, that may make the COM shim moot. More research needed.

mansellan commented 4 years ago

.Net 5 just dropped its final RC (RC2), it goes live in 3 weeks time...