Missing Control.Dispose calls in unit tests

[weltkante]

.NET Core Version: github master

Have you experienced this same bug with .NET Framework?: n/a

Problem description: Disposing controls in unit tests is required because the usage of multiple UI threads. If the control has not been GC'ed by the time the UI thread terminates this can lead to hangs.

Related to #3348 but this is short-term bugfixing while the other issue is long-term reliability to detect regressions in unit test quality.

Following call sites do not dispose their controls:

• [x] AxHostTests.AxHost_EndInit_InvokeWithParent_CreatesControl
• [x] ComboBoxTests.CtrlBackspaceDeletesSelection
• [x] ComboBoxTests.CtrlBackspaceRepeatedTextChanged
• [x] ComboBoxTests.CtrlBackspaceTextChanged
• [x] ComboBoxTests.CtrlBackspaceTextRemainsEmpty
• [x] ControlControlAccessibleObject.Handle_Set_TestData
• [x] ControlControlCollectionTests.ControlCollection_Add _DifferentThreadValueControl_ThrowsArgumentException
• [x] ControlControlCollectionTests.ControlCollection_Add _DifferentThreadValueOwner_ThrowsArgumentException
• [x] ControlTests.AccessibilityObject_CustomCreateAccessibilityInstance_TestData
• [x] ControlTests.Control_Dispose_InvokeNotDisposingWithHandle_Success
• [x] ListViewTests.ListView_BackColor_SetWithHandle_GetReturnsExpected
• [x] ListViewTests.ListView_DoubleBuffered_SetWithHandle_GetReturnsExpected
• [x] ListViewTests.ListView_FocusedItem_SetChildWithHandle_GetReturnsExpected
• [x] ListViewTests.ListView_FocusedItem_SetWithHandle_GetReturnsExpected
• [x] ListViewTests.ListView_ForeColor_SetWithHandle_GetReturnsExpected
• [x] PictureBoxTests.PictureBox_Enabled_SetWithHandle_GetReturnsExpected
• [x] PropertyGridInternal.Tests.GridViewListBoxAccessibleObjectTests .GridViewListBoxAccessibleObject_DoesNotThrowException_OnFocus
• [x] ScreenTests.FromHandle_TestData
• [x] ToolStripRenderEventArgsTests.ConnectedArea_Empty_TestData
• [x] ToolTipTests.ToolTip_Show_InvokeStringIWin32WindowControlWindow_Nop
• [x] ToolTipTests.ToolTip_Show_InvokeStringIWin32WindowIntControlWindow_Nop
• [x] WebBrowserTests.WebBrowserDispose_InvokeNotDisposing_Success

Checked entries look like they are straightforward to fix, unchecked ones require refactoring of the tests or deeper investigation.

Further entries discovered independently (see issue discussion below)

• [x] ErrorProviderAccessibleObjectTests constructor, test class needs to be disposable
• [x] ColorDialog never disposes its ColorUI - a cached popup control

Expected behavior: All unit tests dispose their controls.

Minimal repro: In the absence of #3348 you can set a conditional breakpoint (or write logging code) in NativeWindow.ForceExitMessageLoop and check for handle != IntPtr.Zero && ownedHandle to detect problematic cases. To identify who created the control capture a stack trace in the NativeWindow constructor (call new StackTrace() and store it as a member).

[weltkante]

/cc @hughbe in case this is of interest, /cc @RussKie FYI

I'm not starting a PR since I only know how to fix the trivial cases (marked above) - though if you want a partial PR for this issue I can do that.

[weltkante]

While looking at #3391 I noticed ErrorProviderAccessibleObjectTests test class constructor creates controls which are apparently not disposed. Fix is to implement IDisposable on the test class.

I'm surprised this doesn't appear in my list above, seems the GC never ran during my experimenting, maybe the test executed late and process terminated before GC collected these.

[RussKie]

We're stress testing the runtime and the tests we've (👀 @ @hughbe) written. Whilst it is somewhat frustrating, I think, overall it has been a very good learning for all of us. I expect to find more bugs, but I think we're getting better at it :) To top it off we found few genuine bugs too.

[RussKie]

if you want a partial PR for this issue I can do that.

I'm happy with partial fixes, less to worry about :)

[weltkante]

PR for the simple cases is up.

Also found a control leak outside unit tests, ColorDialog caches its ColorUI popup but never disposes it, and the class is not disposable either.

Currently this is a liability for unit testing, disposing of the dialog is only possible via reflection. I doubt it'll cause issues in the field unless someone builds a multithreaded designer for some reason which shows popups on a seperate UI thread.

[Tanya-Solyanik]

@LeafShi1 - please verify if this issue is still applicable, we were fixing test data as we were editing the tests

[LeafShi1]

please verify if this issue is still applicable, we were fixing test data as we were editing the tests

@Tanya-Solyanik Look like it is still applicable. But I don't know how to verify whether these objects are disposed successfully. Also, do objects in TestData need to be disposed as well? How should this be done?

https://github.com/dotnet/winforms/blob/cfb1cf0db1fe7932c8f1abeaa38465a7043a3301/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/AccessibleObjects/Control.ControlAccessibleObjectTests.cs#L226-L232

[weltkante]

But I don't know how to verify whether these objects are disposed successfully.

See last paragraph in original post, when I originally debugged this issue there was no infrastructure and you had to set breakpoints or modify the code to verify this. By now #3348 has added some infrastructure but I'm not familiar with how it works.

What you want to ensure is that the finalizer isn't used at all to dispose/cleanup controls, because by the time it runs the thread it needs for disposal may already be gone, causing unstable tests that occasionally hang or fail (at least that was what happened back then when I looked into it).

Also, do objects in TestData need to be disposed as well?

Objects implementing IDisposable in the test data are disposed, but in the quoted example the control is just created and never put into the test data, so no automatic disposal happening.