Fix ToolStrip memory leak due to MouseHoverTimer and #4808

[kirsan31]

Fix #4808 and problem that was mentioned in #11334.

Proposed changes

Replace ToolStripItem? _currentItem with WeakReference<ToolStripItem?> _currentItem in MouseHoverTimer
Implement this suggestion.

Customer Impact

No more memory leak if ToolStripItem will be disposed after MouseHoverTimer start.
No more memory leak of chilled elements due to DisplayedItems and OverflowItems collections..

Regression?

• No

Risk

Minimal.

Screenshots

ToolStripItem below was disposed, but remain in memory:

Test methodology

• Existing unit tests.
• New unit tests.
• Manual with test app from ToolStripMenuItem can lead to memory leaks #4808.

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

Attention: Patch coverage is 98.57143% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 74.27562%. Comparing base (4095304) to head (e109957).
Report is 11 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11358         +/-   ##
===================================================
+ Coverage   74.24289%   74.27562%   +0.03273%     
===================================================
  Files           3022        3025          +3     
  Lines         626297      626919        +622     
  Branches       46702       46750         +48     
===================================================
+ Hits          464981      465648        +667     
+ Misses        157921      157909         -12     
+ Partials        3395        3362         -33     

Flag	Coverage Δ
Debug	74.27562% <98.57143%> (+0.03273%)	⬆️
integration	18.00034% <90.00000%> (-0.26493%)	⬇️
production	47.00213% <95.00000%> (+0.08707%)	⬆️
test	96.99459% <100.00000%> (-0.04183%)	⬇️
unit	43.99688% <95.00000%> (+0.14972%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[kirsan31]

Added commit that fix #4808 (thanks to @elachlan).

[elachlan]

@Olina-Zhang Could your team please test this pr to confirm the issues are resolved?

[Olina-Zhang]

@Olina-Zhang Could your team please test this pr to confirm the issues are resolved?

@elachlan @kirsan31 Tested this PR, just can confirm that GH issue #4808 is fixed, I am not clear how to verify #11334, can you give some more information about how to repro it?

• GH issue ToolStripMenuItem can lead to memory leaks #4808 validation
  

[kirsan31]

@Olina-Zhang

can you give some more information about how to repro it?

Same repro app, the key is to remove item immediately after MouseHoverTimer has started. So simply hover and immediately click:

timer.mp4

[Olina-Zhang]

@kirsan31 thanks for your info.
Adding the GH issue #11334 validation: fixed

[Tanya-Solyanik]

Is it possible to make _displayedItems and _overflowItems collections of weak references instead?

I wonder if we have cases when item is removed but is not disposed and will be reused.

[kirsan31]

Is it possible to make _displayedItems and _overflowItems collections of weak references instead?

That was the first thing I thought of. But there about 100 references, it will be too overcomplicated...

I wonder if we have cases when item is removed but is not disposed and will be reused.

Both collections insertions appears only in SetDisplayedItems and both are cleared at start of the method:

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

Lines 4333 to 4337 in 0aa3a4d

	protected virtual void SetDisplayedItems()
	{
	DisplayedItems.Clear();
	OverflowItems.Clear();
	HasVisibleItems = false;

So, the logic must preserve.

Also. SetDisplayedItems are called from OnLayout and both ToolStripItemCollection.Insert and ToolStripItemCollection.Add are calling OnItemAdded -> DoLayoutIfHandleCreated -> ... SetDisplayedItems.

P.s. I noticed a strange difference in the ToolStripItemCollection.Insert and ToolStripItemCollection.Add methods:

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

Lines 106 to 110 in 0aa3a4d

	if (_itemsCollection && _owner is not null)
	{
	_owner.OnItemAddedInternal(value);
	_owner.OnItemAdded(new ToolStripItemEventArgs(value));
	}

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

Lines 310 to 324 in 0aa3a4d

	if (_itemsCollection && _owner is not null)
	{
	if (_owner.IsHandleCreated)
	{
	LayoutTransaction.DoLayout(_owner, value, PropertyNames.Parent);
	}
	else
	{
	// next time we fetch the preferred size, recalc it.
	CommonProperties.xClearPreferredSizeCache(_owner);
	}
	_owner.OnItemAddedInternal(value);
	_owner.OnItemAdded(new ToolStripItemEventArgs(value));
	}

this code is not present in ToolStripItemCollection.Add:

if (_owner.IsHandleCreated) 
{ 
   LayoutTransaction.DoLayout(_owner, value, PropertyNames.Parent); 
} 
else 
{ 
   // next time we fetch the preferred size, recalc it. 
   CommonProperties.xClearPreferredSizeCache(_owner); 
} 

🤔

[Tanya-Solyanik]

Do we own all items in these collections?

[kirsan31]

Do we own all items in these collections?

No of course. More of it I doubt that we own any of them. These are menu items. But all additions of these elements are of course only ours (in SetDisplayedItems), as I wrote above.

[elachlan]

Keep in mind that we are removing the ToolStripItem from our collections here and not disposing of them. So we free up the references, but aren't responsible for the lifetime.