DetachContextMenuStrip pattern can lead to memory leaks

[kirsan31]

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

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.

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

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

[elachlan]

@LeafShi1 can your team please test this?

[kirsan31]

@elachlan DataGridView case (point 2) was tested in #6859. I will adapt repro app to test also point 1...

---------------------UPD---------------------

Updated 1 post with repro app and instructions.

[LeafShi1]

@LeafShi1 can your team please test this?

This problem does exist

[LeafShi1]

Before executing Dispose or bind new data to the datagridview, setting the control's ContextMenuStrip property to null seems to work.

[kirsan31]

Before executing Dispose or bind new data to the datagridview, setting the control's ContextMenuStrip property to null seems to work.

The funniest thing is that this whole story with Disposed event was invented so that the user would not do intuitively understandable things like nullout ContextMenuStrip property of the control before disposing the menu 🤷 But in the end this led to that the user having to do completely unintuitive things like nullout ContextMenuStrip property of the control before disposing the control itself 🤔

[LeafShi1]

The cause of this problem is that when setting the ContextMenuStrip property for the control, the ContextMenuStrip's Disposed event is subscribed, but the subscription is not correctly unsubscribed.

if (value is not null)
{
    value.Disposed += disposedHandler;
}

The best time to unsubscribe should be when the control or the control it is attached to is about to be disposed, and the unsubscription logic has been added to the DataGridViewBands's Dispose logic,

 protected virtual void Dispose(bool disposing)
 {
     if (disposing)
     {
         ContextMenuStrip? contextMenuStrip = ContextMenuStripInternal;
         if (contextMenuStrip is not null)
         {
             contextMenuStrip.Disposed -= DetachContextMenuStrip;
         }
     }

but the control is not disposed actively in the sample project, so contextMenuStrip.Disposed -= DetachContextMenuStrip;has not been executed. Manually adding dispose can ensure that the bound control is disposed successfully

[kirsan31]

@LeafShi1

Manually adding dispose can ensure that the bound control is disposed successfully

Yea, or null out ContextMenuStrip property like with Control. All of this is described in 1 post. But once again this is not logical and not intuitive from the user's point of view 🤷‍♂️

[LeafShi1]

@elachlan @JeremyKuhne What do you think about this issue?

[elachlan]

A user is responsible for the lifespan of a control they create. But in this case, we aren't asking for the user to dispose of the ContextMenuStrip. Its the reference to it. That seems wrong, and the ContextMenuStrip property should be cleared when a row is removed from the datagridview.

[JeremyKuhne]

For Control we should defensively set the ContextMenuStrip property to null in Dispose. Doing so will unhook whatever is there and remove the reference.

For existing code, we need to be careful that we don't dispose something that we've created that user's code might have a reference to.

[kirsan31]

@JeremyKuhne what do you think about implementing this Disposed subscribe through some kind of weak event? It seems to me that the implementation of a weak event for this particular internal case will be quite simple (I can try to do a PR)...

[JeremyKuhne]

@JeremyKuhne what do you think about implementing this Disposed subscribe through some kind of weak event? It seems to me that the implementation of a weak event for this particular internal case will be quite simple (I can try to do a PR)...

@kirsan31 I'm not opposed in general. If someone wants to do this I'll entertain it, but be forewarned that I don't have a lot of bandwidth at the moment. :) Things I would want to see:

• Reusable code
• Detailed guidance
• Tests
• Looping in the Runtime issue asking for weak events

[kirsan31]

@JeremyKuhne

• Looping in the Runtime issue asking for weak events

I read this topic, so I suggested implementing it in a narrow specific case, where we can avoid problems of wide public implementation. My thots are: Simple List<WeakReference<EventHandler>> and main rule - subscribe only through delegates explicitly declared as class members. That is, to make a class not for widespread use, but strictly to solve one old problem (maybe not to make a separate class at all?).

[JeremyKuhne]

@kirsan31 I believe we should encapsulate what we do here. I pointed out the issue not to say that we need to solve it, just that we need to consider and reference it so that:

1. Those looking at the issue are aware of a real need in .NET
2. If something is ever added that we can potentially replace whatever we've done

Our helper doesn't need to solve all things, just the immediate need.