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

[GeneratedComInterface] based Windows automation #16543

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Jul 31, 2024

What is the current behavior?

Avalonia Windows accessibility/automation is not compatible with trimming or AOT compilation due to legacy COM interop usage.

What is the updated/expected behavior with this PR?

.NET 8 target uses source generated COM interop which is well compatible with AOT and trimming.

How was the solution implemented (if it's not obvious)?

  1. By using new GeneratedComInterface source generator.
  2. SafeArray and ComVariant implementation was partially copied from .NET 9 code, partially written by hand, but generally overly simplified for our needs. Note, .NET 9 still doesn't have SafeArray marshalling implemented, it's not something we can use in the next year.
  3. .NetStandard 2.0 and .NET 6 targets still support legacy COM interop via some #if directives.
  4. GeneratedComInterface doesn't support COM interface properties (these are not part of the COM standard) - had to replace with methods.
  5. Since GeneratedComInterface required disabling runtime marshalling, but our Win32 backend is not ready for that yet (see Make Avalonia not rely on runtime marshaling #16273), I had to move accessibility/automation implementation into a separated assembly. This assembly only contains internal types (visible to Avalonia.Win32) and nu-merged into Avalonia.Win32, without adding any new packages.

Extra:
On legacy targets, dropped ComVisible and instead used ComImport and fixed interface definitions (WPF had them defined wrong, relying on broken by design COM support in old .NET), completing this idea.

Alternative solutions

  1. Just use MicroCom. Since I already wrote SafeArray and ComWrapper marshalling, it might be relatively easy to make them work with MicroCom.
  2. Keep GeneratedComInterface approach, but wait until we update Win32 backend to not rely on runtime marshalling, and keep this logic in the Win32 package as it was. @kekekeks I am interested in implementing this idea and can pick it up. But on the other hand, this new assembly can always be removed without any breaking changes in the future.

Open questions

  1. We now have an ability to run Windows Automation tests with NativeAOT. I am optimistic about this option and believe we should do that.
  2. With this PR, CI runs only .NET 8 builds (using new COM interop internally). If anything breaks with legacy COM interop targets, it won't be noticed. Should we add another azure job just for .NET 6 target? Maybe even NET Framework?

Breaking changes

Old ComVisible interfaces were public and accessible to users. Since it was only part of platform specific package, and not directly usable with other Avalonia APIs, it should be safe to hide them. But just to be safe, marking this PR as wont-backport. If necessary (probably not), this PR can be backported while keeping old COM interfaces in Avalonia.Win32 unused.

Fixed issues

Fixes #8006
Fixes #16697
Closes #13924

@maxkatz6
Copy link
Member Author

cc @hez2010 if you have time to take a look.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050727-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@rabbitism
Copy link
Contributor

Also invite @MouriNaruto for review

nukebuild/numerge.config Outdated Show resolved Hide resolved
src/Windows/Avalonia.Win32.Automation/AutomationNode.cs Outdated Show resolved Hide resolved
/// <summary>
/// Release resources owned by this <see cref="ComVariant"/> instance.
/// </summary>
public unsafe void Dispose()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't VariantClear be sufficient?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would need a this pointer which might be a problem for structs.

src/Windows/Avalonia.Win32/Avalonia.Win32.csproj Outdated Show resolved Hide resolved
src/Windows/Avalonia.Win32/Avalonia.Win32.csproj Outdated Show resolved Hide resolved
# Conflicts:
#	src/Windows/Avalonia.Win32.Automation/AutomationNode.cs
#	src/Windows/Avalonia.Win32.Automation/InteropAutomationNode.cs
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052573-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052579-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Marshal.Copy(data, pointers, 0, array.Length);
for (int i = 0; i < pointers.Length; i++)
{
if (ComWrappers.TryGetObject(pointers[i], out var instance))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryGetObject will only unwrap an existing managed object, so it won't work for COM objects we didn't create. We should probably use ComInterfaceMarshaller.ConvertToManaged() here instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be a bit concerned about RCW ownership and potential finalization in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants