diff --git a/src/System.Windows.Forms/System/Windows/Forms/Accessibility/LabelEditNativeWindow.cs b/src/System.Windows.Forms/System/Windows/Forms/Accessibility/LabelEditNativeWindow.cs index 6c675ed0348..7cce1f367d1 100644 --- a/src/System.Windows.Forms/System/Windows/Forms/Accessibility/LabelEditNativeWindow.cs +++ b/src/System.Windows.Forms/System/Windows/Forms/Accessibility/LabelEditNativeWindow.cs @@ -15,6 +15,7 @@ internal class LabelEditNativeWindow : NativeWindow private HWINEVENTHOOK _valueChangeHook; private HWINEVENTHOOK _textSelectionChangedHook; private bool _winEventHooksInstalled; + private bool _isReleasing; private delegate void WINEVENTPROC( HWINEVENTHOOK hWinEventHook, @@ -74,8 +75,24 @@ protected override void OnHandleChange() { base.OnHandleChange(); + // Prevent reentrant hook installation during ReleaseHandle. + // Without this guard, UnhookWinEvent can trigger OnHandleChange (via NativeWindow.Callback), + // causing hooks to be reinstalled on a window that is being destroyed. + if (_isReleasing) + { + return; + } + if (!IsHandleCreated) { + // The native window handle was destroyed (e.g., Windows sent WM_NCDESTROY and NativeWindow + // cleared the handle without going through our ReleaseHandle override). Unhook the WinEvent + // hooks immediately so the WINEVENT_INCONTEXT hook cannot fire after _winEventProcCallback is + // garbage collected. Without this, the LabelEditNativeWindow is removed from the NativeWindow + // handle table and becomes eligible for GC while the hooks remain registered. The next + // CallWindowProc on any window on this thread (e.g. GridViewTextBox.DefWndProc) would then + // invoke the GC'd delegate and trigger a FailFast. + UnhookWinEventHooks(); return; } @@ -92,31 +109,52 @@ protected override void OnHandleChange() public override unsafe void ReleaseHandle() { - if (_winEventHooksInstalled) + _isReleasing = true; + try { - PInvoke.UnhookWinEvent(_valueChangeHook); - PInvoke.UnhookWinEvent(_textSelectionChangedHook); + UnhookWinEventHooks(); - _winEventHooksInstalled = false; - } + if (IsHandleCreated) + { + // When a window that previously returned providers has been destroyed, + // you should notify UI Automation by calling the UiaReturnRawElementProvider + // as follows: UiaReturnRawElementProvider(hwnd, 0, 0, NULL). This call tells + // UI Automation that it can safely remove all map entries that refer to the specified window. + PInvoke.UiaReturnRawElementProvider(HWND, wParam: 0, lParam: 0, (IRawElementProviderSimple*)null); + } - if (IsHandleCreated) + PInvoke.UiaDisconnectProvider(AccessibilityObject); + AccessibilityObject = null; + base.ReleaseHandle(); + } + finally { - // When a window that previously returned providers has been destroyed, - // you should notify UI Automation by calling the UiaReturnRawElementProvider - // as follows: UiaReturnRawElementProvider(hwnd, 0, 0, NULL). This call tells - // UI Automation that it can safely remove all map entries that refer to the specified window. - PInvoke.UiaReturnRawElementProvider(HWND, wParam: 0, lParam: 0, (IRawElementProviderSimple*)null); + _isReleasing = false; } + } - PInvoke.UiaDisconnectProvider(AccessibilityObject); + private void UnhookWinEventHooks() + { + if (_winEventHooksInstalled) + { + PInvoke.UnhookWinEvent(_valueChangeHook); + PInvoke.UnhookWinEvent(_textSelectionChangedHook); + _winEventHooksInstalled = false; - AccessibilityObject = null; - base.ReleaseHandle(); + // Allow the delegate to be garbage collected now that no hook holds a reference + // to its underlying unmanaged function pointer. + _winEventProcCallback = null; + } } private void WinEventProcCallback(HWINEVENTHOOK hWinEventHook, uint eventId, HWND hwnd, int idObject, int idChild, uint idEventThread, uint dwmsEventTime) { + // Late-arriving callbacks should be ignored if we are releasing or hooks are already uninstalled. + if (_isReleasing || !_winEventHooksInstalled) + { + return; + } + if (hwnd != Handle || idObject != (int)OBJECT_IDENTIFIER.OBJID_CLIENT || !IsAccessibilityObjectCreated) { return; @@ -141,7 +179,9 @@ private void WmGetObject(ref Message m) // Accessibility object was likely requested by an assistive tech (which sent WM_GETOBJECT message). // We may need to install winevent hooks to produce the automation events related to the text pattern. - if (!_winEventHooksInstalled) + // However, if we are in the process of releasing the handle, skip hook installation to avoid + // creating new hooks on a window that is being destroyed. + if (!_winEventHooksInstalled && !_isReleasing) { InstallWinEventHooks(); } diff --git a/src/test/unit/System.Windows.Forms/System/Windows/Forms/AccessibleObjects/TreeViewLabelEditAccessibleObjectTests.cs b/src/test/unit/System.Windows.Forms/System/Windows/Forms/AccessibleObjects/TreeViewLabelEditAccessibleObjectTests.cs index c57a1beeefd..f524026f5a1 100644 --- a/src/test/unit/System.Windows.Forms/System/Windows/Forms/AccessibleObjects/TreeViewLabelEditAccessibleObjectTests.cs +++ b/src/test/unit/System.Windows.Forms/System/Windows/Forms/AccessibleObjects/TreeViewLabelEditAccessibleObjectTests.cs @@ -132,6 +132,56 @@ public void TreeViewLabelEditNativeWindow_Ctor_NullOwningTreeView_ThrowsArgument Assert.Throws(() => new TreeViewLabelEditNativeWindow(null)); } + [WinFormsFact] + public void LabelEditNativeWindow_OnHandleChange_DuringRelease_DoesNotReinstallHooks() + { + using TreeView treeView = new() { Size = new Size(300, 200) }; + treeView.CreateControl(); + + TreeViewLabelEditNativeWindow labelEdit = new(treeView); + + labelEdit.TestAccessor.Dynamic._isReleasing = true; + labelEdit.TestAccessor.Dynamic.OnHandleChange(); + + Assert.False((bool)labelEdit.TestAccessor.Dynamic._winEventHooksInstalled); + } + + [WinFormsFact] + public void TreeViewLabelEditNativeWindow_ReleaseHandle_UnhooksAndCleansUpState() + { + using TreeView treeView = new() { Size = new Size(300, 200) }; + treeView.CreateControl(); + + TreeViewLabelEditNativeWindow labelEdit = new(treeView); + labelEdit.AssignHandle(treeView.Handle); + + // Simulate hooks installed + labelEdit.TestAccessor.Dynamic._winEventHooksInstalled = true; + + labelEdit.ReleaseHandle(); + + Assert.False((bool)labelEdit.TestAccessor.Dynamic._winEventHooksInstalled); + Assert.Null(labelEdit.TestAccessor.Dynamic._winEventProcCallback); + } + + [WinFormsFact] + public void TreeViewLabelEditNativeWindow_ReleaseHandle_PreventsReentrantHookInstallation() + { + using TreeView treeView = new() { Size = new Size(300, 200) }; + treeView.CreateControl(); + + TreeViewLabelEditNativeWindow labelEdit = new(treeView); + + labelEdit.TestAccessor.Dynamic._isReleasing = true; + labelEdit.TestAccessor.Dynamic._winEventHooksInstalled = false; + + // Simulates OnHandleChange being triggered mid-release + labelEdit.TestAccessor.Dynamic.OnHandleChange(); + + // Hooks must NOT be reinstalled while releasing + Assert.False((bool)labelEdit.TestAccessor.Dynamic._winEventHooksInstalled); + } + [WinFormsFact] public void TreeViewLabelEditUiaTextProvider_Ctor_NullOwningTreeView_ThrowsArgumentNullException() {