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.

Sign up for GitHub

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

Fix #12661 Null Reference Exception: System.Windows.Forms.TabControl.<WmSelChange> #12683

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix #12661 Null Reference Exception: System.Windows.Forms.TabControl.<WmSelChange> #12683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1e2cb8
Fix issue 12661 and add unit test
John-Qiao Dec 25, 2024
43f7faa
Updated the unit test code lines to resolve the conversations from Tanya
John-Qiao Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/System.Windows.Forms/src/System/Windows/Forms/Controls/TabControl/TabControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1957,7 +1957,7 @@ private bool WmSelChange()
if (IsAccessibilityObjectCreated && SelectedTab?.ParentInternal is TabControl)
{
SelectedTab.TabAccessibilityObject.RaiseAutomationEvent(UIA_EVENT_ID.UIA_SelectionItem_ElementSelectedEventId);
BeginInvoke((MethodInvoker)(() => SelectedTab.TabAccessibilityObject.RaiseAutomationEvent(UIA_EVENT_ID.UIA_AutomationFocusChangedEventId)));
BeginInvoke((MethodInvoker)(() => SelectedTab?.TabAccessibilityObject.RaiseAutomationEvent(UIA_EVENT_ID.UIA_AutomationFocusChangedEventId)));
Copy link
Member

Choose a reason for hiding this comment

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

We should also test if (IsAccessibilityObjectCreated && SelectedTab?.ParentInternal is TabControl) inside the method invoker delegate in case the control had been disposed before the delegate was invoked.

Copy link
Member

Choose a reason for hiding this comment

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

In the BeginInvoke delegate, should we consider adding conditional checks for IsAccessibilityObjectCreated and SelectedTab?.ParentInternal is TabControl to ensure the state is valid at execution time?

BeginInvoke((MethodInvoker)(() =>
{
     if (IsAccessibilityObjectCreated && SelectedTab?.ParentInternal is TabControl && 
          !SelectedTab.IsDisposed && SelectedTab.TabAccessibilityObject != null)
     {
        SelectedTab.TabAccessibilityObject.RaiseAutomationEvent(UIA_EVENT_ID.UIA_AutomationFocusChangedEventId);
     }
}));

Copy link
Member

@Tanya-Solyanik Tanya-Solyanik Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is SelectedTab.IsDisposed possible? Please double-check if we are un-selecting tabs before disposing them.

It's ok to create accessibility object for a non-disposed child control, if the parent has an accessible object. The child accessible object will be created to support some other user action anyway if accessibility tools are being used. And we know that accessibility tools are used because they initiated creation of the parent accessible object.

}
}
else
Expand Down
24 changes: 24 additions & 0 deletions src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TabControlTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5703,6 +5703,30 @@ public void TabControl_Invokes_SetToolTip_IfExternalToolTipIsSet()
Assert.Equal(text, actual);
}

[WinFormsFact]
public void TabControl_WmSelChange_SelectedTabIsNull_DoesNotThrowException()
{
using Form form = new();
using TabControl control = new();
using TabPage page1 = new("text1");
using TabPage page2 = new("text2");
control.TabPages.Add(page1);
control.TabPages.Add(page2);
_ = control.AccessibilityObject;

form.Controls.Add(control);
form.Show();

control.SelectedIndex = 1;
control.TabPages.Clear();

Action act = () => control.TestAccessor().Dynamic.WmSelChange();
Application.DoEvents();
Thread.Sleep(1000);
John-Qiao marked this conversation as resolved.
Show resolved Hide resolved

act.Should().NotThrow();
Copy link
Member

Choose a reason for hiding this comment

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

In this scenario, exception happens after WmSelChange had been executed, the delegate is only scheduled for execution with BeginInvoke.

    control.TestAccessor().Dynamic.WmSelChange();
    Application.DoEvents();
    Thread.Sleep(100);
   // no exception

Copy link
Member

Choose a reason for hiding this comment

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

So here we need to ensure that SelectedTab is diaposed after WmSelChange is executed and before the event UIA_AutomationFocusChangedEventId is executed.

 Action act = () => control.TestAccessor().Dynamic.WmSelChange();
 act.Should().NotThrow();

 control.TabPages.Clear();

 var exception = Record.Exception(() =>
 {
     Application.DoEvents();
     Thread.Sleep(100);
 });

 exception.Should().BeNull();

}

private class SubTabPage : TabPage
{
}
Expand Down