ToolStripMenuItem can lead to memory leaks

[kirsan31]

• .NET Core Version: all up to latest (5.0.5)

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

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

• Menu with submenu.

 windowsToolStripMenuItem 
  -> listToolStripMenuItem

• Populate and clear submenu in runtime. In fact, only cleaning is important.

private void WindowsToolStripMenuItem_DropDownOpened(object sender, EventArgs e)
{
    for (int i = 0; i < 4; i++)
        listToolStripMenuItem.DropDownItems.Add("MenuItem" + i);
}

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 collection still have live references to removed menu items. I think this collection ( and may be _overflowItems too ) must be checked on item removal.

menu.mp4

Workaround:
You can call listToolStripMenuItem.DropDown.PerformLayout(); after items removal. This lead to call of SetDisplayedItems() 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

[Tanya-Solyanik]

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

[Olina-Zhang]

@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.

[elachlan]

Dispose has a section to dispose of the items. I guess we just do the same for the DisplayedItems and OverflowItems

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStrip.cs

Lines 2011 to 2020 in e199c86

	if (!Items.IsReadOnly)
	{
	// only dispose the items we actually own.
	for (int i = Items.Count - 1; i >= 0; i--)
	{
	Items[i].Dispose();
	}
	Items.Clear();
	}

[elachlan]

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).

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStrip.cs

Lines 4906 to 4909 in e199c86

	internal void OnItemRemovedInternal(ToolStripItem item)
	{
	KeyboardToolTipStateMachine.Instance.Unhook(item, ToolTip);
	}

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]

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.

GHIssue4808Verify.mp4