PropertyGrid allocates ImageList handles while disposing

[weltkante]

.NET Core Version:
master

Have you experienced this same bug with .NET Framework?:
unknown

Problem description:
PropertyGrid creates native ImageList handles when disposing. Those handles are not disposed and left for the GC to collect.

Stack Trace A

call site

System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_SetWithHandle_GetReturnsExpected

allocation stack

   at System.Windows.Forms.ImageList.NativeImageList..ctor(IntPtr himl)
   at System.Windows.Forms.ImageList.CreateHandle()
   at System.Windows.Forms.ImageList.get_Handle()
   at Interop.ComCtl32.ImageList.DrawEx(IHandle himl, Int32 i, HandleRef hdcDst, Int32 x, Int32 y, Int32 dx, Int32 dy, Int32 rgbBk, Int32 rgbFg, ILD fStyle)
   at System.Windows.Forms.ImageList.GetBitmap(Int32 index)
   at System.Windows.Forms.ImageList.ImageCollection.get_Item(Int32 index)
   at System.Windows.Forms.ToolStripItem.get_Image()
   at System.Windows.Forms.ToolStripItem.Animate(Boolean animate)
   at System.Windows.Forms.ToolStripItem.StopAnimate()
   at System.Windows.Forms.ToolStripItem.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.ToolStrip.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.Control.Dispose(Boolean disposing)
   at System.Windows.Forms.ContainerControl.Dispose(Boolean disposing)
   at System.Windows.Forms.PropertyGrid.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_SetWithHandle_GetReturnsExpected(Boolean visible, Boolean value, Int32 expectedLayoutCallCount1, Int32 expectedInvalidatedCallCount1, Int32 expectedLayoutCallCount2, Int32 expectedInvalidatedCallCount2)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at Xunit.Sdk.TestInvoker`1.CallTestMethod(Object testClassInstance)
   at Xunit.Sdk.UITestInvoker.<>c__DisplayClass2_0.<<RunAsync>b__2>d.MoveNext()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackDo(ThreadMethodEntry tme)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackHelper(Object obj)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Windows.Forms.Control.InvokeMarshaledCallback(ThreadMethodEntry tme)
   at System.Windows.Forms.Control.InvokeMarshaledCallbacks()
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.Control.ControlNativeWindow.OnMessage(Message& m)
   at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, WM msg, IntPtr wparam, IntPtr lparam)
   at Interop.User32.DispatchMessageW(MSG& msg)
   at Interop.User32.DispatchMessageW(MSG& msg)
   at System.Windows.Forms.Application.ComponentManager.Interop.Mso.IMsoComponentManager.FPushMessageLoop(UIntPtr dwComponentID, msoloop uReason, Void* pvLoopData)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(msoloop reason, ApplicationContext context)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(msoloop reason, ApplicationContext context)
   at System.Windows.Forms.Application.DoEvents()
   at Xunit.Sdk.WinFormsSynchronizationContextAdapter.PumpTill(SynchronizationContext synchronizationContext, Task task)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)

Stack Trace B

call site

System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_Set_GetReturnsExpected

allocation stack

   at System.Windows.Forms.ImageList.NativeImageList..ctor(IntPtr himl)
   at System.Windows.Forms.ImageList.CreateHandle()
   at System.Windows.Forms.ImageList.get_Handle()
   at Interop.ComCtl32.ImageList.DrawEx(IHandle himl, Int32 i, HandleRef hdcDst, Int32 x, Int32 y, Int32 dx, Int32 dy, Int32 rgbBk, Int32 rgbFg, ILD fStyle)
   at System.Windows.Forms.ImageList.GetBitmap(Int32 index)
   at System.Windows.Forms.ImageList.ImageCollection.get_Item(Int32 index)
   at System.Windows.Forms.ToolStripItem.get_Image()
   at System.Windows.Forms.ToolStripItem.Animate(Boolean animate)
   at System.Windows.Forms.ToolStripItem.StopAnimate()
   at System.Windows.Forms.ToolStripItem.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.ToolStrip.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.Control.Dispose(Boolean disposing)
   at System.Windows.Forms.ContainerControl.Dispose(Boolean disposing)
   at System.Windows.Forms.PropertyGrid.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_Set_GetReturnsExpected(Boolean visible, Boolean value)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at Xunit.Sdk.TestInvoker`1.CallTestMethod(Object testClassInstance)
   at Xunit.Sdk.UITestInvoker.<>c__DisplayClass2_0.<<RunAsync>b__2>d.MoveNext()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackDo(ThreadMethodEntry tme)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackHelper(Object obj)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Windows.Forms.Control.InvokeMarshaledCallback(ThreadMethodEntry tme)
   at System.Windows.Forms.Control.InvokeMarshaledCallbacks()
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.Control.ControlNativeWindow.OnMessage(Message& m)
   at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, WM msg, IntPtr wparam, IntPtr lparam)
   at Interop.User32.DispatchMessageW(MSG& msg)
   at Interop.User32.DispatchMessageW(MSG& msg)
   at System.Windows.Forms.Application.ComponentManager.Interop.Mso.IMsoComponentManager.FPushMessageLoop(UIntPtr dwComponentID, msoloop uReason, Void* pvLoopData)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(msoloop reason, ApplicationContext context)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(msoloop reason, ApplicationContext context)
   at System.Windows.Forms.Application.DoEvents()
   at Xunit.Sdk.WinFormsSynchronizationContextAdapter.PumpTill(SynchronizationContext synchronizationContext, Task task)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)

Expected behavior:
PropertyGrid should not allocate new handles during disposal

Minimal repro:
Put a breakpoint in NativeImageList finalizer and run one of these two tests:

• System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_SetWithHandle_GetReturnsExpected
• System.Windows.Forms.Tests.PropertyGridTests.PropertyGrid_LargeButtons_Set_GetReturnsExpected

[weltkante]

/cc @RussKie @JeremyKuhne

Could this be responsible for memory corruption in #3358? ImageList is implemented in common controls, can someone check whether its safe to release them in the finalizer, or whether they are UI thread bound (like HWND) and must be released on the thread that created them? (i.e. is the finalizer of NativeImageList safe to keep or should it be replaced with different logic?)

There are a bunch of other ImageLists that weren't disposed properly, fixing them in a separate PR (#3486), created an issue for these two since the fix is non-obvious.

[edit] couldn't repro any crashes by writing an app that stresses multithreaded ImageList allocation/collection so I doubt its causing corruption, the handle creation during dispose should still be fixed of course.

[RussKie]

Looking at the stack trace I think it's quite possible that it is related.

[RussKie]

BUT having said the above, a fix would be to stop new allocations during the dispose sequence.