dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 981 forks source link

ToolStripMenuItem can lead to memory leaks #4808

Closed kirsan31 closed 1 month ago

kirsan31 commented 3 years ago

Problem description: ToolStripMenuItem can lead to memory leaks in some special conditions.

private void windowsToolStripMenuItem_DropDownClosed(object sender, EventArgs e) { while (listToolStripMenuItem.DropDownItems.Count > 0) listToolStripMenuItem.DropDownItems[listToolStripMenuItem.DropDownItems.Count - 1].Dispose(); }


In certain conditions (I think that is - open only main menu and not opening submenu `listToolStripMenuItem`) after closing main menu some of `listToolStripMenuItem` subitems will remain in memory. See video below.
This happens due to [`_displayedItems`](https://github.com/dotnet/winforms/blob/c9287e9ca780cd1828633fd3c6e43a6601896147/src/System.Windows.Forms/src/System/Windows/Forms/ToolStrip.cs#L40) collection still have live references to removed menu items. I think this collection ( and may be `_overflowItems` too ) must be checked on item removal.

https://user-images.githubusercontent.com/17767561/115139643-acf23080-a03b-11eb-9be9-c0e4c2527f38.mp4

*Workaround:*
You can call `listToolStripMenuItem.DropDown.PerformLayout();` after items removal. This lead to call of [`SetDisplayedItems()`](https://github.com/dotnet/winforms/blob/c9287e9ca780cd1828633fd3c6e43a6601896147/src/System.Windows.Forms/src/System/Windows/Forms/ToolStrip.cs#L4561) which will clear `_displayedItems` of removed elements.

**Expected behavior:**
All removed and disposed menu elements must be eligible to GC (have no live references).

**Minimal repro:**
[WinFormsCoreTest_Grid_Menu.zip](https://github.com/dotnet/winforms/files/6330797/WinFormsCoreTest_Grid_Menu.zip)
Tanya-Solyanik commented 1 year ago

@Olina-Zhang - could you please retest this issue?

Olina-Zhang commented 1 year ago

@Olina-Zhang - could you please retest this issue?

Verified this issue in the latest .Net 8.0 build as above sample application and video in VS, have same testing result, so still repro. image

elachlan commented 11 months ago

Dispose has a section to dispose of the items. I guess we just do the same for the DisplayedItems and OverflowItems https://github.com/dotnet/winforms/blob/e199c86138cf8fee176bddd3f7e19ec62a526765/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStrip.cs#L2011-L2020

elachlan commented 11 months ago

We might also want to look at OnItemRemovedInternal. Which is called by ToolStripItemCollection when an item is removed (such as when ToolStripItem.Dispose is called). https://github.com/dotnet/winforms/blob/e199c86138cf8fee176bddd3f7e19ec62a526765/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStrip.cs#L4906-L4909

We could then trigger a cleanup DisplayedItems and OverflowItems, I think ToolStripItemCollection might need to pass itself to OnItemRemovedInternal so we can cleanup based on which list had the item removed.

Olina-Zhang commented 1 month ago

Verified this issue in the latest .Net 9.0 RC2 SDK build: 9.0.100-rc.2.24468.2 + binaries built fromWinforms repo main branch, it was fixed.

https://github.com/user-attachments/assets/4eab15ae-d2d4-48e3-9e88-a772b698b30d