Construction of `AccessibleObject` forces creation of `Handle`

[RussKie]

• .NET Core Version:
  .NET 5 P4

Problem description:

Multiple code paths involving a construction or an interaction with an AccessibleObject on a control that hasn't had its Handle created cause the creation of the Handle.
This may lead to undesired side-effects like described in #2797.

Expected behavior:

None of AccessibleObject events must be raised if an owner control doesn't have a valid `Handle.

winforms/src/System.Windows.Forms/src/System/Windows/Forms/AccessibleObject.cs

Lines 2227 to 2294 in b666dc7

	public bool RaiseAutomationNotification(AutomationNotificationKind notificationKind, AutomationNotificationProcessing notificationProcessing, string notificationText)
	{
	if (!notificationEventAvailable)
	{
	return false;
	}
	try
	{
	// The activityId can be any string. It cannot be null. It is not used currently.
	HRESULT result = UiaCore.UiaRaiseNotificationEvent(
	this,
	notificationKind,
	notificationProcessing,
	notificationText,
	string.Empty);
	return result == HRESULT.S_OK;
	}
	catch (EntryPointNotFoundException)
	{
	// The UIA Notification event is not available, so don't attempt to raise it again.
	notificationEventAvailable = false;
	return false;
	}
	}
	/// <summary>
	/// Raises the LiveRegionChanged UIA event.
	/// This method must be overridden in derived classes that support the UIA live region feature.
	/// </summary>
	/// <returns>True if operation succeeds, False otherwise.</returns>
	public virtual bool RaiseLiveRegionChanged()
	{
	throw new NotSupportedException(SR.AccessibleObjectLiveRegionNotSupported);
	}
	internal bool RaiseAutomationEvent(UiaCore.UIA eventId)
	{
	if (UiaCore.UiaClientsAreListening().IsTrue())
	{
	HRESULT result = UiaCore.UiaRaiseAutomationEvent(this, eventId);
	return result == HRESULT.S_OK;
	}
	return false;
	}
	internal bool RaiseAutomationPropertyChangedEvent(UiaCore.UIA propertyId, object oldValue, object newValue)
	{
	if (UiaCore.UiaClientsAreListening().IsTrue())
	{
	HRESULT result = UiaCore.UiaRaiseAutomationPropertyChangedEvent(this, propertyId, oldValue, newValue);
	return result == HRESULT.S_OK;
	}
	return false;
	}
	internal bool RaiseStructureChangedEvent(UiaCore.StructureChangeType structureChangeType, int[] runtimeId)
	{
	if (UiaCore.UiaClientsAreListening().IsTrue())
	{
	HRESULT result = UiaCore.UiaRaiseStructureChangedEvent(this, structureChangeType, runtimeId, runtimeId == null ? 0 : runtimeId.Length);
	return result == HRESULT.S_OK;
	}
	return false;
	}

Minimal repro:

Run the following test:

winforms/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ToolStripItemTests.cs

Lines 14005 to 14022 in b666dc7

	[WinFormsFact]
	public void ToolStripItem_Select_InvokeWithParent_Success()
	{
	using var parent = new ToolStrip();
	using var item = new SubToolStripItem
	{
	Parent = parent
	};
	item.Select();
	Assert.True(item.Selected);
	Assert.False(parent.IsHandleCreated);
	// Select again.
	item.Select();
	Assert.True(item.Selected);
	Assert.False(parent.IsHandleCreated);
	}

Line:14014 will cause ToolStripItem.Handle to be created.

 	System.Windows.Forms.dll!System.Windows.Forms.Control.CreateHandle() Line 4912	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.Control.Handle.get() Line 2464	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.Control.ControlAccessibleObject.ControlAccessibleObject(System.Windows.Forms.Control ownerControl = {System.Windows.Forms.ToolStrip}) Line 32	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.ToolStrip.ToolStripAccessibleObject.ToolStripAccessibleObject(System.Windows.Forms.ToolStrip owner = {System.Windows.Forms.ToolStrip}) Line 5016	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.ToolStrip.CreateAccessibilityInstance() Line 4985	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.Control.AccessibilityObject.get() Line 498	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.ToolStripItem.ToolStripItemAccessibleObject.FragmentRoot.get() Line 364	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.AccessibleObject.Interop.UiaCore.IRawElementProviderFragment.FragmentRoot.get() Line 819	C#	Symbols loaded.
 	[Native to Managed Transition]		Annotated Frame
 	[Managed to Native Transition]		Annotated Frame
 	System.Windows.Forms.dll!System.Windows.Forms.AccessibleObject.RaiseAutomationEvent(Interop.UiaCore.UIA eventId = AutomationFocusChangedEventId) Line 2267	C#	Symbols loaded.
>	System.Windows.Forms.dll!System.Windows.Forms.ToolStripItem.ToolStripItemAccessibleObject.RaiseFocusChanged() Line 481	C#	Symbols loaded.
 	System.Windows.Forms.dll!System.Windows.Forms.ToolStripItem.Select() Line 3094	C#	Symbols loaded.
 	System.Windows.Forms.Tests.dll!System.Windows.Forms.Tests.ToolStripItemTests.ToolStripItem_Select_InvokeWithParent_Success() Line 14017	C#	Symbols loaded.

[RussKie]

We could, for example, add the following guard and then run tests:

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlAccessibleObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlAccessibleObject.cs
index b1bb53959..fa1699363 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlAccessibleObject.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlAccessibleObject.cs
@@ -29,6 +29,12 @@ namespace System.Windows.Forms
             public ControlAccessibleObject(Control ownerControl)
             {
                 Owner = ownerControl ?? throw new ArgumentNullException(nameof(ownerControl));
+
+                if (!ownerControl.IsHandleCreated)
+                {
+                    Debug.Fail("Accessibility object must not be created for a control without a valid handle");
+                }
+
                 IntPtr handle = ownerControl.Handle;
                 Handle = handle;
             }

This guard will crash the test runner, highlighting the paths where we fire off accessibility events on controls without valid handles.

Would this be a correct approach?

[hughbe]

I'm curious why I can reproduce this test failure but it doesn't repro on the CI? Any ideas

[RussKie]

No, and this has been bugging me for months. I get about a dozen of failures when run tests locally, and those are consistent (clipboard, monthcalendar dimensions, axhost...).
There must be some subtle nuances that make-or-break the tests, that we're yet to identify.

[weltkante]

There must be some subtle nuances that make-or-break the tests, that we're yet to identify.

If the local failures are deterministic and not entirely caused by native win32 differences you should be able to put additional logging into the test, dumping all relevant conditions leading to the failure and at last step fail the test intentionally. Put it in a PR to get CI execute it and look at the log to determine which of the conditions differs to local execution. Add static variables to help logging if necessary, this is just diagnostic and not supposed to be merged.

If the failure happens in native win32 code its harder because you have to guess what conditions influence the call.

I can take a look if you don't beat me to it. Feel free to ping me on any other instances of this kind if you want me to look at, as long as they are deterministic local failures it should be possible to figure out the differences.