DataGridView not dispose old elements

[kirsan31]

• .NET Core Version: all up to 7.0 p2

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

Problem description: As I mentioned in https://github.com/dotnet/winforms/issues/6858 DataGridView not dispose old data on DataSource change. This is lead to 2 problems:

1. All this stuff are going to finalization query because we have unnecessary finalizer here #6858.
2. If we have a ContextMenuStrip bound to Cell/Column/Row we will have a memory leak. DataGridView with 3 columns and ContextMenuStrip bound to 1000 rows, result of DS refresh:

Expected behavior: Old data must be property disposed. Probably in DataGridView.RefreshColumnsAndRows() (in DataGridView.Methods.cs)

Minimal repro: DataGridViewLeak.zip

[dreddy-work]

Thanks @kirsan31. ownership and lifespan of the DataSource need to be investigated here. Given that this has been the case since .NET framework, this will be on low priority for the team but should be able to help you if you get chance to investigate this further.

[kirsan31]

ownership and lifespan of the DataSource need to be investigated here.

No it's not about DataSource, it's about that all elements (DataGridViewCell, DataGridViewRow, DataGridViewColumn) are replaced on dataGridVie.DataSource = dataSource; (and may some other scenarios too).

Given that this has been the case since .NET framework, this will be on low priority for the team

https://social.msdn.microsoft.com/Forums/silverlight/en-US/fdba08b0-5c62-47a4-be73-48d11372fa02/colossal-memory-leak-contextmenu-on-datagridrow https://stackoverflow.com/questions/20270367/gdi-objects-memory-leak-custom-toolstripcontrolhost-within-contextmenustrip-is https://stackoverflow.com/questions/44224884/c-sharp-contextmenustrip-of-datagridviewrow-what-is-the-equivalent-of-sourcecon And so on... The general conclusion people come to is not to use ContextMenuStrip with DataGridView elements.

The problem is in good known pattern - ContextMenuStrip lifetime tracking: https://github.com/dotnet/winforms/blob/55e1755b579395e7e22fecf340905c79ccfa9b1a/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewBand.cs#L56-L79

So, if you will not remove this handler (on item delete) - element will live forever (until ContextMenuStrip dispose). Here's what I think... After we fix https://github.com/dotnet/winforms/issues/6858, may be it's a user task to null out all ContextMenuStrip properties (which he set) before DataSource update? 😁

-------------- UPDATE --------------

I am on it, due to problems with https://github.com/dotnet/winforms/pull/6878 :( Fixes have already been made, I'm studying and testing the consequences...

[kirsan31]

I need a conceptual help here 😔 What did I find out while investigating all this dispose stuff in DataGridView:

1. DataGridView does not care about the disposing of it's components at all.
2. Due to finalizers (empty) in base components (DataGridViewBand, DataGridViewCell) all of them are end in Freachable queue. And it has drastically performance impact on apps with many constantly updating DataGridViews.
3. As we cannot remove this finalizers we need implement properly dispose logic. And then I stalled 😕

My first attempt was dispose rows and cells only on databound DataGridView datasource change. But there were many downsides:

• Оnly whole datasource change was processed (removing rows in existing datasource still was not disposed).
• Subcomponents (like rowheaders) still was not disposed.
• Not databound DataGridView not affected at all.

Current attempt is to dispose only self created internal components. But it has many question too... Dispose on: clear, remove, change?

And the main one (example below is very simplified synthetic and applies to all disposable DataGridView components):

var dataGridView1 = new DataGridView();
dataGridView1.AutoGenerateColumns = false;

// we explicitly create new column
var colmn = new DataGridViewColumn() { Name = "Clmn", HeaderText = "Clmn" };
dataGridView1.Columns.Add(colmn);
// we not dispose colmn here because it's external element
dataGridView1.Columns.Clear();
// we need to dispose it manually
colmn.Dispose();

// new column will be internally created
dataGridView1.Columns.Add("Clmn", "Clmn");
// we can safely dispose it on clear?
dataGridView1.Columns.Clear();

// new column will be internally created
dataGridView1.Columns.Add("Clmn", "Clmn");
// user cash it somehow
colmn = dataGridView1.Columns[0];
// we dispose it here because it's our internal column
dataGridView1.Columns.Clear();
// user want to add it back but colmn already disposed...
dataGridView1.Columns.Add(colmn);

At the moment, I see only one solution - to add a public property like DisposeInternalComponents with default to false not to break current implementations. What do you think? P.s. during investigation I found some small bug and the ability to improve performance - I will try to create a PRs...

/cc @dreddy-work @RussKie

[RussKie]

Thank you for your thoughts. Looks like this journey may take us into a long-forgotten places full of dust and despair :)

I had a quick test, and it looks like the DGV was implemented in a way that didn't assume any responsibility for its components.

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewTextBoxColumn.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewTextBoxColumn.cs
index 99c2bb296..849f7fb85 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewTextBoxColumn.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewTextBoxColumn.cs
@@ -5,6 +5,7 @@
 #nullable disable

 using System.ComponentModel;
+using System.Diagnostics;
 using System.Drawing;
 using System.Globalization;
 using System.Text;
@@ -19,8 +20,20 @@ namespace System.Windows.Forms
         public DataGridViewTextBoxColumn() : base(new DataGridViewTextBoxCell())
         {
             SortMode = DataGridViewColumnSortMode.Automatic;
+#if DEBUG
+            _callingStack = Environment.StackTrace;
+#endif
         }

+#if DEBUG
+        private readonly string _callingStack;
+
+        ~DataGridViewTextBoxColumn()
+        {
+            throw new InvalidOperationException($"Did not dispose {GetType().Name}. Originating stack:\n{_callingStack}");
+        }
+#endif
+
         [Browsable(false)]
         [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
         public override DataGridViewCell CellTemplate

Running the DataGridView tests from WinformsControlsTest project immediately fails with the IOE from the finalizer:

   at System.Environment.get_StackTrace()
   at System.Windows.Forms.DataGridViewTextBoxColumn..ctor() in D:\Development\dotnet-winforms\src\System.Windows.Forms\src\System\Windows\Forms\DataGridViewTextBoxColumn.cs:line 24
   at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean wrapExceptions)
   at System.Windows.Forms.DataGridViewColumn.Clone() in D:\Development\dotnet-winforms\src\System.Windows.Forms\src\System\Windows\Forms\DataGridViewColumn.cs:line 904
   at System.Windows.Forms.DataGridView.CorrectColumnFrozenStates(DataGridViewColumn[] dataGridViewColumns) in D:\Development\dotnet-winforms\src\System.Windows.Forms\src\System\Windows\Forms\DataGridView.Methods.cs:line 5202
   at System.Windows.Forms.DataGridView.OnAddingColumns(DataGridViewColumn[] dataGridViewColumns) in D:\Development\dotnet-winforms\src\System.Windows.Forms\src\System\Windows\Forms\DataGridView.Methods.cs:line 11179
   at System.Windows.Forms.DataGridViewColumnCollection.AddRange(DataGridViewColumn[] dataGridViewColumns) in D:\Development\dotnet-winforms\src\System.Windows.Forms\src\System\Windows\Forms\DataGridViewColumnCollection.cs:line 317
   at WinformsControlsTest.DataGridViewTest.InitializeComponent() in D:\Development\dotnet-winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\DataGridViewTest.Designer.cs:line 64
   at WinformsControlsTest.DataGridViewTest..ctor() in D:\Development\dotnet-winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\DataGridViewTest.cs:line 25
   ....

Without understanding of all possible use cases, there is a substantial risk in assuming the ownership of individual components by the DGV. The biggest issue with the DGV control is its size - it's too big to have a holistic view and understanding of it. I'm going to take this to the team for an internal discussion. /cc: @merriemcgaw

[kirsan31]

@RussKie

I had a quick test, and it looks like the DGV was implemented in a way that didn't assume any responsibility for its components.

Exactly :( And what is the most annoying is that almost all dispose logic in the control is about ContextMenuStrip lifetime tracking (see my 2 post) 😡 I mean that strictly speaking in 90% cases we don't need IDisposable here at all.

Without understanding of all possible use cases, there is a substantial risk in assuming the ownership of individual components by the DGV. The biggest issue with the DGV control is its size - it's too big to have a holistic view and understanding of it.

Same thots and this is why I suggest to introduce some property DisposeInternalComponents to not break ppl. If you don't care - use DataGridView in old free for all way :) And if you care - adopt to new rule - use with care elements created by DataGridView itself...

[RussKie]

I am now wondering, if this "liberal" handling of DGV components is somehow linked to the DGV-related tests (e.g., https://github.com/dotnet/winforms/issues/6926 and https://github.com/dotnet/winforms/issues/6739) that fail with observable persistence...

[kirsan31]

I am now wondering, if this "liberal" handling of DGV components is somehow linked to the DGV-related tests (e.g., #6926 and #6739) that fail with observable persistence...

I don't think it's related. On situation overall I see that both test fails with extra Invalidate. I think it wood be helpful (to investigate further) to know inputs (parameters) and CallStack of extra Invalidated. So, for example this test:

control.Invalidated += (sender, e) => invalidatedCallCount++;

can be transform to something like:

control.Invalidated += (sender, e) =>
{
    Assert.True(++invalidatedCallCount <= expectedInvalidatedCallCount,
        $"Input: {rowHeadersWidthSizeMode}, {rowHeadersVisible}, {autoSize}, {value}");
};

---

About dispose problem, I want to clarify it with all info that we have for now.

• Almost all main components of DataGridView implement IDisposable, through DataGridViewCell and DataGridViewBand with empty finalizers.
• DataGridView does not care about disposing these components at all.

• Most these classes use dispose to simply unsubscribe from ContextMenuStrip Disposed event. Exceptions: DataGridViewColumn:

  if (disposing)
  {
      lock (this)
      {
          Site?.Container?.Remove(this);
          _disposed?.Invoke(this, EventArgs.Empty);
      }
  }

  I'm not familiar with using Site, so I can't say what will happen if the dispose is not called.

  DataGridViewHeaderCell:

  if (FlipXPThemesBitmap is not null && disposing)
  {
      FlipXPThemesBitmap.Dispose();
  }

  FlipXPThemesBitmap is sort glyph bitmap that used only with RightToLeft.

So we have three problems with dispose not to be called:

1. Slow down due to empty finalizers on all elements. For us this is the main problem (and I think that for all too, they just don't realize it😁). Will be almost solved with suggested fix and updated docs.
2. If element have it's own ContextMenuStrip - will lead to memory leak. Can be solved by user with setting ContextMenuStrip property to null before each remove/replace of this element. And yes, in some cases this is the pain.
3. DataGridViewColumn and DataGridViewHeaderCell in some cases really need to be disposed. Can't say anything about Site logic :(

In general, after the first point is resolved, the importance of this problem for us will no longer be such significant.

[ghost]

This issue is now marked as "up for grabs", and we’re looking for a community volunteer to work on this issue. If we receive no interest in 120 days, we will close the issue. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[dreddy-work]

@RussKie, Can you summarize what the next steps here for the community given marked it for 'up-for-grabs'?

[elachlan]

@kirsan31 are you able to summarize what is left to do for this issue to be resolved?

[kirsan31]

@elachlan

are you able to summarize what is left to do for this issue to be resolved?

Most part of the work is already done. Slightly edited my last post:

So we have three problems with dispose not to be called:

1. Slow down due to empty finalizers on all elements. Completed. For us this is the main problem (and I think that for all too, they just don't realize it😁).
2. If element have it's own ContextMenuStrip - will lead to memory leak. Can be solved by user with setting ContextMenuStrip property to null before each remove/replace of this element. And yes, in some cases this is the pain.
3. DataGridViewColumn and DataGridViewHeaderCell in some cases really need to be disposed. Can't say anything about Site logic :( Here some details.