From 0082fc6b2a9542b3651c3f56eca267f69ec9f208 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 7 Sep 2026 13:45:14 +0000 Subject: [PATCH] Fix UIA selection never reaching the client on Windows (#22151) * Add failing tests for SAFEARRAY marshalling of COM providers SafeArrayRef.CreateFromObjects sizes the SAFEARRAY to the pooled buffer it rents rather than to the input, and only fills a slot when the object already has a live COM wrapper. Cover both with tests driven through SafeArrayMarshaller, which is how the UIA interop actually reaches this code. Two of the three tests fail: a two-element input produces a 16-element array of nulls. The string test passes and is the control. Each provider test checks the length before it reads the entries. A wrongly sized array holds slots that were never written, and dereferencing those takes down the test host with an access violation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RYYMWsTkmkKjptFL88xjAg * Fix ISelectionProvider.GetSelection() never reaching the UIA client SafeArrayRef.CreateFromObjects had two defects, and between them every VT_UNKNOWN SAFEARRAY the automation interop produced was empty. It sized the array to the buffer it rented from ArrayPool rather than to the input, so a one-element selection became a 16-element array. The rented buffer isn't cleared, so stale pointers from an earlier rent could leak in and later be released by SafeArrayDestroy. It also filled a slot only when ComWrappers.TryGetComInstance succeeded. That call resolves a managed object that wraps a native COM instance; for a purely managed object it always fails, so no slot was ever filled and UIA received an array of nulls. Wrappers now come from ComInterfaceMarshaller, which is what the rest of the interop uses. Sharing the instance matters: UI Automation identifies elements by IUnknown pointer, so a wrapper from a second ComWrappers would read as a different element. The element type isn't recoverable from the values, so SafeArrayMarshaller passes it through a new generic TryCreate overload. The object branch of the non-generic TryCreate is gone with it. Nothing reached it: the only other caller is ComVariant.Create, and no property value is an array of providers. This fixes ISelectionProvider.GetSelection() and, by the same route, ITableProvider.GetRowHeaders/GetColumnHeaders, ITableItemProvider, and ITextProvider.GetSelection/GetVisibleRanges. Fixes #22150 Claude-Session: https://claude.ai/code/session_01RYYMWsTkmkKjptFL88xjAg Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../Avalonia.Win32.Automation.csproj | 1 + .../Marshalling/SafeArrayMarshaller.cs | 2 +- .../Marshalling/SafeArrayRef.cs | 127 ++++++++++-------- .../Automation/SafeArrayRefTests.cs | 83 ++++++++++++ 4 files changed, 156 insertions(+), 57 deletions(-) create mode 100644 tests/Avalonia.IntegrationTests.Win32/Automation/SafeArrayRefTests.cs diff --git a/src/Windows/Avalonia.Win32.Automation/Avalonia.Win32.Automation.csproj b/src/Windows/Avalonia.Win32.Automation/Avalonia.Win32.Automation.csproj index c56131c495..0d30f8e74b 100644 --- a/src/Windows/Avalonia.Win32.Automation/Avalonia.Win32.Automation.csproj +++ b/src/Windows/Avalonia.Win32.Automation/Avalonia.Win32.Automation.csproj @@ -15,5 +15,6 @@ + diff --git a/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayMarshaller.cs b/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayMarshaller.cs index ef3af4a3b2..a82ad213c4 100644 --- a/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayMarshaller.cs +++ b/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayMarshaller.cs @@ -10,7 +10,7 @@ internal static class SafeArrayMarshaller where T : notnull { public static SafeArrayRef ConvertToUnmanaged(T[]? managed) => managed is null ? new SafeArrayRef() - : SafeArrayRef.TryCreate(managed, out var result, out _) ? result.Value + : SafeArrayRef.TryCreate(managed, out var result, out _) ? result.Value : throw new NotImplementedException($"SafeArray marshalling for '{managed?.GetType().Name}' is not implemented."); public static T[]? ConvertToManaged(SafeArrayRef unmanaged) => SafeArrayRef.ToArray(unmanaged); diff --git a/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayRef.cs b/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayRef.cs index f27c67e36e..b4c2e53710 100644 --- a/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayRef.cs +++ b/src/Windows/Avalonia.Win32.Automation/Marshalling/SafeArrayRef.cs @@ -8,6 +8,7 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Runtime.InteropServices.Marshalling; // ReSharper disable InconsistentNaming namespace Avalonia.Win32.Automation.Marshalling; @@ -160,6 +161,25 @@ internal unsafe partial struct SafeArrayRef }); } + /// + /// Creates a SAFEARRAY from a typed array. + /// + /// + /// COM interfaces have to come through here rather than through the non-generic overload: the + /// element type is what tells us to wrap each item, and it isn't recoverable from the values. + /// + public static bool TryCreate(T[]? managed, [NotNullWhen(true)] out SafeArrayRef? safearray, out VarEnum varEnum) + { + if (managed is not null && typeof(T).IsInterface) + { + safearray = CreateFromComObjects(managed); + varEnum = VarEnum.VT_UNKNOWN; + return true; + } + + return TryCreate((IEnumerable?)managed, out safearray, out varEnum); + } + public static bool TryCreate(IEnumerable? managed, [NotNullWhen(true)] out SafeArrayRef? safearray, out VarEnum varEnum) { safearray = default; @@ -181,38 +201,6 @@ internal unsafe partial struct SafeArrayRef return CreateFromSpan(collectionSpan, varEnum); } - static SafeArrayRef CreateFromSpan(ReadOnlySpan span, VarEnum varEnum) - { - var bound = new SAFEARRAYBOUND { cElements = (uint)span.Length, lLbound = 0 }; - var safearray = SafeArrayCreate(varEnum, 1, bound); - if (span.Length == 0) - { - return new SafeArrayRef - { - _ptr = safearray - }; - } - - var lockResult = SafeArrayLock(safearray); - if (lockResult != 0) throw new Win32Exception(lockResult); - - try - { - // We assume it has the same length. - var output = new Span(safearray->pvData, (int)safearray->rgsabound[0].cElements); - span.CopyTo(output); - } - finally - { - SafeArrayUnlock(safearray); - } - - return new SafeArrayRef - { - _ptr = safearray - }; - } - static SafeArrayRef CreateFromStrings(IReadOnlyList strings, VarEnum varEnum) { Debug.Assert(varEnum == VarEnum.VT_BSTR); // other types not supported yet @@ -251,28 +239,6 @@ internal unsafe partial struct SafeArrayRef } } - static SafeArrayRef CreateFromObjects(IReadOnlyList objects, VarEnum varEnum) - { - Debug.Assert(varEnum == VarEnum.VT_UNKNOWN); // other types not supported yet - var pointers = ArrayPool.Shared.Rent(objects.Count); - try - { - for (int i = 0; i < objects.Count; i++) - { - if (ComWrappers.TryGetComInstance(objects[i], out var pointer)) - { - pointers[i] = pointer; - } - } - - return CreateFromSpan(pointers, varEnum); - } - finally - { - ArrayPool.Shared.Return(pointers); - } - } - safearray = managed switch { IReadOnlyCollection ints => CreateFromCollection(ints, varEnum = VarEnum.VT_I1), @@ -295,14 +261,63 @@ internal unsafe partial struct SafeArrayRef IReadOnlyList strings => CreateFromStrings(strings, varEnum = VarEnum.VT_BSTR), - IReadOnlyList objects => CreateFromObjects(objects, varEnum = VarEnum.VT_UNKNOWN), - _ => null }; return safearray is not null; } + private static SafeArrayRef CreateFromSpan(ReadOnlySpan span, VarEnum varEnum) + { + var bound = new SAFEARRAYBOUND { cElements = (uint)span.Length, lLbound = 0 }; + var safearray = SafeArrayCreate(varEnum, 1, bound); + if (span.Length == 0) + { + return new SafeArrayRef + { + _ptr = safearray + }; + } + + var lockResult = SafeArrayLock(safearray); + if (lockResult != 0) throw new Win32Exception(lockResult); + + try + { + // We assume it has the same length. + var output = new Span(safearray->pvData, (int)safearray->rgsabound[0].cElements); + span.CopyTo(output); + } + finally + { + SafeArrayUnlock(safearray); + } + + return new SafeArrayRef + { + _ptr = safearray + }; + } + + private static SafeArrayRef CreateFromComObjects(T[] objects) + { + var pointers = ArrayPool.Shared.Rent(objects.Length); + + try + { + for (var i = 0; i < objects.Length; i++) + { + pointers[i] = (IntPtr)ComInterfaceMarshaller.ConvertToUnmanaged(objects[i]); + } + + return CreateFromSpan(pointers.AsSpan(0, objects.Length), VarEnum.VT_UNKNOWN); + } + finally + { + ArrayPool.Shared.Return(pointers); + } + } + [LibraryImport("oleaut32.dll")] private static unsafe partial SAFEARRAY* SafeArrayCreate(VarEnum vt, uint cDims, in SAFEARRAYBOUND rgsabound); diff --git a/tests/Avalonia.IntegrationTests.Win32/Automation/SafeArrayRefTests.cs b/tests/Avalonia.IntegrationTests.Win32/Automation/SafeArrayRefTests.cs new file mode 100644 index 0000000000..8794a03b76 --- /dev/null +++ b/tests/Avalonia.IntegrationTests.Win32/Automation/SafeArrayRefTests.cs @@ -0,0 +1,83 @@ +using System; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Runtime.InteropServices.Marshalling; +using Avalonia.Win32.Automation.Marshalling; +using Xunit; + +namespace Avalonia.IntegrationTests.Win32.Automation; + +public unsafe class SafeArrayRefTests +{ + [Fact] + public void ConvertToUnmanaged_Sizes_String_Array_To_Input() + { + var safeArray = SafeArrayMarshaller.ConvertToUnmanaged(["foo", "bar", "baz"]); + + Assert.Equal(3, GetLength(safeArray)); + + SafeArrayMarshaller.Free(safeArray); + } + + [Fact] + public void ConvertToUnmanaged_Sizes_Provider_Array_To_Input() + { + ITestProvider[] providers = [new TestProvider(1), new TestProvider(2)]; + + var safeArray = SafeArrayMarshaller.ConvertToUnmanaged(providers); + + Assert.Equal(providers.Length, GetLength(safeArray)); + + SafeArrayMarshaller.Free(safeArray); + } + + [Fact] + public void ConvertToUnmanaged_Fills_Provider_Array_With_Com_Wrappers() + { + ITestProvider[] providers = [new TestProvider(1), new TestProvider(2)]; + + var safeArray = SafeArrayMarshaller.ConvertToUnmanaged(providers); + + // Check the length before reading the entries. A wrongly sized array holds slots that were + // never written, and dereferencing those below would take down the test host. + Assert.Equal(providers.Length, GetLength(safeArray)); + Assert.All(GetEntries(safeArray), x => Assert.NotEqual(IntPtr.Zero, x)); + + var roundTripped = SafeArrayMarshaller.ConvertToManaged(safeArray); + Assert.NotNull(roundTripped); + Assert.Equal([1, 2], Array.ConvertAll(roundTripped, x => x.GetValue())); + + SafeArrayMarshaller.Free(safeArray); + } + + private static int GetLength(SafeArrayRef safeArray) + { + var ptr = (SafeArrayRef.SAFEARRAY*)Unsafe.As(ref safeArray); + return (int)ptr->rgsabound[0].cElements; + } + + private static IntPtr[] GetEntries(SafeArrayRef safeArray) + { + var ptr = (SafeArrayRef.SAFEARRAY*)Unsafe.As(ref safeArray); + var result = new IntPtr[(int)ptr->rgsabound[0].cElements]; + new Span(ptr->pvData, result.Length).CopyTo(result); + return result; + } +} + +[GeneratedComInterface] +[Guid("6ADEBBF3-6C63-4C1B-9C4F-9EE7C3B8A6D1")] +internal partial interface ITestProvider +{ + int GetValue(); +} + +[GeneratedComClass] +internal partial class TestProvider : ITestProvider +{ + private readonly int _value; + + public TestProvider(int value) => _value = value; + + public int GetValue() => _value; +}