Leak of NativeImageList

[JeremyKuhne]

The finalizer asserted when I was fiddling around in WinFormsControlTest. Here is the dumped originating stack:

   at System.Windows.Forms.ImageList.NativeImageList..ctor(IStream pstm)
   at System.Windows.Forms.ImageListStreamer..ctor(SerializationInfo info, StreamingContext context)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeConstructorInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at System.Runtime.Serialization.ObjectManager.CompleteISerializableObject(Object obj, SerializationInfo info, StreamingContext context)
   at System.Runtime.Serialization.ObjectManager.FixupSpecialObject(ObjectHolder holder)
   at System.Runtime.Serialization.ObjectManager.DoFixups()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at System.Resources.Extensions.DeserializingResourceReader.ReadBinaryFormattedObject()
   at System.Resources.Extensions.DeserializingResourceReader.DeserializeObject(Int32 typeIndex)
   at System.Resources.Extensions.DeserializingResourceReader.LoadObjectV2(Int32 pos, ResourceTypeCode& typeCode)
   at System.Resources.Extensions.DeserializingResourceReader.ResourceEnumerator.get_Entry()
   at System.Resources.Extensions.DeserializingResourceReader.ResourceEnumerator.get_Current()
   at System.ComponentModel.ComponentResourceManager.FillResources(CultureInfo culture, ResourceSet& resourceSet)
   at System.ComponentModel.ComponentResourceManager.ApplyResources(Object value, String objectName, CultureInfo culture)
   at System.ComponentModel.ComponentResourceManager.ApplyResources(Object value, String objectName)
   at WinformsControlsTest.TreeViewTest.InitializeComponent()
   at WinformsControlsTest.TreeViewTest..ctor()
   at WinformsControlsTest.MainForm.<>c.<GetButtonsInitInfo>b__1_9(Object obj, EventArgs e)
   at System.Windows.Forms.Control.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnMouseUp(MouseEventArgs mevent)
   at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks)
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.ButtonBase.WndProc(Message& m)
   at System.Windows.Forms.Button.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.Run(Form mainForm)
   at WinformsControlsTest.Program.Main()

[weltkante]

Yes, the ImageListStreamer you get from deserialization needs to be disposed, I had to fix this in several unit tests as well. However from the call stack this instance looks like a WinForms framework bug and not a user code bug.

The problem seems to be that ComponentResourceManager.FillResources deserializes a disposable ImageListStreamer and assigns it to an ImageList control, but neither the ImageList nor the ComponentResourceManager feels responsible for taking ownership, so nobody disposes it and its left for the GC to collect the ImageListStreamer (which owns a NativeImageList that needs to be released).

[RussKie]

Hmm... This is nasty. We don't own ComponentResourceManager, so it's a pretty much a black box to us. At some point it creates an instance of ImageListStreamer that creates an instance of NativeImageList.
I suspect ImageListStreamer isn't getting disposed either, else it would dispose the owned instance of NativeImageList:

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

Lines 262 to 272 in f88abd1

	private void Dispose(bool disposing)
	{
	if (disposing)
	{
	if (nativeImageList != null)
	{
	nativeImageList.Dispose();
	nativeImageList = null;
	}
	}
	}

It feels that ComponentResourceManager is responsible for disposing the streamer 🤔

[weltkante]

It feels that ComponentResourceManager is responsible for disposing the streamer 🤔

I don't think so, if you generalize this responsibility it would also mean it should dispose deserialized Bitmap images, wouldn't it? That would mean everyone has to clone resources assigned through ComponentResourceManager if they are immediately disposed. Feels like unnecessary overhead.

In my opinion the ownership of the ImageListStreamer should be passed on to the ImageList control. If thats not possible (because it breaks compatibility) then you may have to revert the strict asserts and have NativeImageList be released in a finalizer. Or maybe add special support code in ImageListStreamer finalizer.

[kirsan31]

We already know two use cases (#4913, #4928) where GDI leak appear due to NativeImageList destructor removal. I wonder, is this doable work - to check for proper disposing of all existing ImageList references? And even if so, we may still have cases like #4928 and many many existing users codebase that not disposing (not properly disposing) ImageList ...

[weltkante]

NativeImageList doesn't necessarily need a finalizer, but ImageListStreamer and ImageList need to be handling it properly if it doesn't have one.

Note that this is actually two issues, ImageListStreamer has the additional issue that its not clear who is "owning" it and should dispose of it in the first place.

Generally I agree some finalizer needs to be in place, either ImageListStreamer and ImageList, or on NativeImageList. You cannot assume Dispose will be called. Things like Bitmaps don't have a clear owner either, WinForms always needs to have a Finalizer to handle cleanup when the user doesn't.

[RussKie]

I've spent a little time wading through the ListView test sample with have - it trips on undisposed NativeImageList instances (run in DEBUG).

winforms/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/ListViewTest.Designer.cs

Lines 33 to 145 in cb8eb66

	private void InitializeComponent()
	{
	this.components = new System.ComponentModel.Container();
	System.ComponentModel.ComponentResourceManager resources = new System.ComponentModel.ComponentResourceManager(typeof(ListViewTest));
	this.imageList1 = new System.Windows.Forms.ImageList(this.components);
	this.listView1 = new System.Windows.Forms.ListView();
	this.columnHeader1 = (System.Windows.Forms.ColumnHeader)(new System.Windows.Forms.ColumnHeader());
	this.columnHeader2 = (System.Windows.Forms.ColumnHeader)(new System.Windows.Forms.ColumnHeader());
	this.imageList2 = new System.Windows.Forms.ImageList(this.components);
	this.btnClearListView1 = new System.Windows.Forms.Button();
	this.btnLoadImagesListView1 = new System.Windows.Forms.Button();
	this.LargeImageList = new System.Windows.Forms.ImageList(this.components);
	this.btnReplaceImageListView1 = new System.Windows.Forms.Button();
	this.openFileDialog1 = new System.Windows.Forms.OpenFileDialog();
	this.SuspendLayout();
	//
	// imageList1
	//
	this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));
	this.imageList1.TransparentColor = System.Drawing.Color.Transparent;
	this.imageList1.Images.SetKeyName(0, "SmallA.bmp");
	this.imageList1.Images.SetKeyName(1, "SmallABlue.bmp");
	//
	// columnHeader1
	//
	this.columnHeader1.ImageIndex = 0;
	//
	// columnHeader2
	//
	this.columnHeader2.ImageIndex = 1;
	//
	// listView1
	//
	this.listView1.Anchor = System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Bottom | System.Windows.Forms.AnchorStyles.Left;
	this.listView1.Columns.AddRange(new System.Windows.Forms.ColumnHeader[] {
	this.columnHeader1,
	this.columnHeader2});
	this.listView1.Groups.AddRange(new System.Windows.Forms.ListViewGroup[] {
	(System.Windows.Forms.ListViewGroup)(resources.GetObject("listView1.Groups"))});
	this.listView1.HideSelection = false;
	this.listView1.Items.AddRange(new System.Windows.Forms.ListViewItem[] {
	(System.Windows.Forms.ListViewItem)(resources.GetObject("listView1.Items")),
	(System.Windows.Forms.ListViewItem)(resources.GetObject("listView1.Items1")),
	(System.Windows.Forms.ListViewItem)(resources.GetObject("listView1.Items2"))});
	this.listView1.LargeImageList = this.imageList2;
	resources.ApplyResources(this.listView1, "listView1");
	this.listView1.Location = new System.Drawing.Point(12, 33);
	this.listView1.Name = "listView1";
	this.listView1.Size = new System.Drawing.Size(439, 159);
	this.listView1.SmallImageList = this.imageList1;
	this.listView1.TabIndex = 0;
	this.listView1.UseCompatibleStateImageBehavior = false;
	//
	// imageList2
	//
	this.imageList2.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList2.ImageStream"));
	this.imageList2.TransparentColor = System.Drawing.Color.Transparent;
	this.imageList2.Images.SetKeyName(0, "LargeA.bmp");
	this.imageList2.Images.SetKeyName(1, "LargeABlue.bmp");
	//
	// btnClearListView1
	//
	this.btnClearListView1.Location = new System.Drawing.Point(13, 4);
	this.btnClearListView1.Name = "btnClearListView1";
	this.btnClearListView1.Size = new System.Drawing.Size(75, 23);
	this.btnClearListView1.TabIndex = 1;
	this.btnClearListView1.Text = "Clear";
	this.btnClearListView1.UseVisualStyleBackColor = true;
	this.btnClearListView1.Click += new System.EventHandler(this.btnClearListView1_Click);
	//
	// btnLoadImagesListView1
	//
	this.btnLoadImagesListView1.Location = new System.Drawing.Point(95, 4);
	this.btnLoadImagesListView1.Name = "btnLoadImagesListView1";
	this.btnLoadImagesListView1.Size = new System.Drawing.Size(75, 23);
	this.btnLoadImagesListView1.TabIndex = 2;
	this.btnLoadImagesListView1.Text = "Load images";
	this.btnLoadImagesListView1.UseVisualStyleBackColor = true;
	this.btnLoadImagesListView1.Click += new System.EventHandler(this.btnLoadImagesListView1_Click);
	//
	// LargeImageList
	//
	this.LargeImageList.ColorDepth = System.Windows.Forms.ColorDepth.Depth24Bit;
	this.LargeImageList.ImageSize = new System.Drawing.Size(256, 256);
	this.LargeImageList.TransparentColor = System.Drawing.Color.Transparent;
	//
	// openFileDialog1
	//
	this.openFileDialog1.FileName = "openFileDialog1";
	this.openFileDialog1.Multiselect = true;
	this.openFileDialog1.SupportMultiDottedExtensions = true;
	//
	// btnReplaceImageListView1
	//
	this.btnReplaceImageListView1.Location = new System.Drawing.Point(176, 4);
	this.btnReplaceImageListView1.Name = "btnReplaceImageListView1";
	this.btnReplaceImageListView1.Size = new System.Drawing.Size(87, 23);
	this.btnReplaceImageListView1.TabIndex = 3;
	this.btnReplaceImageListView1.Text = "Replace image";
	this.btnReplaceImageListView1.UseVisualStyleBackColor = true;
	this.btnReplaceImageListView1.Click += new System.EventHandler(this.btnReplaceImageListView1_Click);
	//
	// ListViewTest
	//
	resources.ApplyResources(this, "$this");
	this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
	this.Controls.Add(this.btnReplaceImageListView1);
	this.Controls.Add(this.btnLoadImagesListView1);
	this.Controls.Add(this.btnClearListView1);
	this.Controls.Add(this.listView1);
	this.Name = "ListViewTest";
	this.Text = "ListView Test";
	this.ResumeLayout(false);

I observed two different issues:

1. Leaks caused by this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));
   I think we can address this leak by the following changes:

   diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs
   index 656058094..1655ee370 100644
   --- a/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs
   +++ b/src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs
   @@ -216,7 +216,7 @@ namespace System.Windows.Forms
                        return;
                    }
    
   -                NativeImageList himl = value.GetNativeImageList();
   +                using NativeImageList himl = value.GetNativeImageList();
                    if (himl != null && himl != _nativeImageList)
                    {
                        bool recreatingHandle = HandleCreated; // We only need to fire RecreateHandle if there was a previous handle
   @@ -250,6 +250,8 @@ namespace System.Windows.Forms
                            OnRecreateHandle(EventArgs.Empty);
                        }
                    }
   +
   +                value.Dispose();
                }
            }

   This may quite likely a breaking change, but I think we can make it because ImageListStreamer class can't be instantiated nor subclassed, and is more an infrastructural type than a client-facing type. So I think the likelihood of our customers manually tinkering with it is pretty low.

2. Leaks caused by resources.ApplyResources(...); calls.
   I was unable not find how the created instance of NativeImageList is assigned and managed (I haven't had enough time to dig deep). So here I think we should just add a finalizer to ImageListStreamer.
   ❕ This will also help with the 1st issue above.

[kirsan31]

@RussKie

We already have a big silent breaking change - removal of NativeImageList finalizer. That's lead to critical problem - GDI leak, even in Winforms repo itself (#4913, #4928). May be this is not so important for small brief time running apps, but for big and long running it's a pain (due to crush after hard limit gg). GDI leaks are difficult to debug, especial not managed one and if it's not repro on local machine )))
So, this 2 known leaks are only in WinForms itself, and how many we didn't catch yet (for example TreeView uses ImageList very tightly)? I'm afraid to even think how many of them will be in the user code after migration from .net framework and .net core? And I don't understand how this breaking change silently goes live after assert here and then in #4316?

1. Leaks caused by this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));
   I think we can address this leak by the following changes:

A quick google om ImageList.ImageStream return that folks using it time to time. So it will be second breaking change (not that big like first one, but any way). On the other hand, I do not know how to properly solve this problem so as not to rely on the finalizer. The only way I see is designer generated code change - designer need to take care about disposing of ImageListStreamer. Or may be some change in ImageList, like:
this.imageList1.ImageStreamDirect = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream")); Where ImageStreamDirect will not duplicate NativeImageList (or disposing it like you suggested). But this also require designer change. I think all of this is somehow complicated...

2. Leaks caused by resources.ApplyResources(...); calls.
   I was unable not find how the created instance of NativeImageList is assigned and managed (I haven't had enough time to dig deep). So here I think we should just add a finalizer to ImageListStreamer.
   ❕ This will also help with the 1st issue above.

Here and overall I am totally agree with @weltkante :

Generally I agree some finalizer needs to be in place, either ImageListStreamer and ImageList, or on NativeImageList. You cannot assume Dispose will be called. Things like Bitmaps don't have a clear owner either, WinForms always needs to have a Finalizer to handle cleanup when the user doesn't.

To not break existing code, We need 2 finalizers in ImageListStreamer and ImageList or one in NativeImageList.

[RussKie]

We already have a big silent breaking change - removal of NativeImageList finalizer.

A fair call, we regressed in #3526.

A quick google om ImageList.ImageStream return that folks using it time to time.

Can you please share an example? I couldn't find anything specific that could break by the proposed change.