DetachContextMenuStrip pattern can lead to memory leaks
- Dominant language
- C#
- Stars
- 4.9k
- Forks
- 1.1k
- Avg merge
- 20h 23m
- Merged PRs (30d)
- 103
Description
### .NET version
All up to .Net9.
### Did it work in .NET Framework?
No
### Did it work in any of the earlier releases of .NET Core or .NET 5+?
No.
### Issue description
This null out `ContextMenuStrip` pattern:
```cs
private void DetachContextMenuStrip(object? sender, EventArgs e) => ContextMenuStrip = null;
// And ContextMenuStrip.Set` property:
EventHandler disposedHandler = new(DetachContextMenuStrip);
if (oldValue is not null)
{
oldValue.Disposed -= disposedHandler;
}
if (value is not null)
{
value.Disposed += disposedHandler;
}
```
Can lead to memory leaks.
Using in 3 places:
- [x] Issue 1.
`Control` https://github.com/dotnet/winforms/blob/d1986025b2dafdcc3aeda0505da37e580bb3d82e/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs#L1232 More or less justified, since control in most cases lives either as long as the menu, or longer. But I didn't find a `ContextMenuStrip.Disposed -= disposedHandler;` code in `Control.Dispose`. So if `ContextMenuStrip` will outlive control (and user not null out `ContextMenuStrip` property) - we will have memory leak (control will remain in memory) at any case.
- [ ] Issue 2.
`DataGridViewBand` and `DataGridViewCell` https://github.com/dotnet/winforms/blob/1f2d238fe75b40408de8d3cd6ca363df3615132f/src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewBand.cs#L59 https://github.com/dotnet/winforms/blob/1f2d238fe75b40408de8d3cd6ca363df3615132f/src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewCell.cs#L117 Here it probably does more harm than good, since DataGridView elements in most cases [live less than the menu and they never disposed](https://github.com/dotnet/winforms/issues/6859#issuecomment-1072103983).
My thots are:
- At the very least we should add `ContextMenuStrip.Disposed -= disposedHandler;` code in `Control.Dispose`.
- Ideally, to avoid making breaking changes we should use some implementation of weak events here. 🤔
### Steps to reproduce
[DetachContextMenuStripLeaks.zip](https://github.com/user-attachments/files/17444579/DetachContextMenuStripLeaks.zip)
- Use context menu on the DataGridView from the repro app, compare snapshots before and after:
- "Refresh data" will leak `DataGridView` elements.
- Every "Add Control" + "Remove Control" will leak one `Control` class.
Contributor guide
Assessment
This issue has not been assessed yet.