dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.01k stars 1.16k forks source link

DataGrid: memory leaks after items cleared #6983

Open KirillOsenkov opened 1 year ago

KirillOsenkov commented 1 year ago

There appear to be multiple memory leaks in DataGrid.

It feels like it was never tested for memory leaks after the items were cleared.

After a DataGrid is cleared (items collection view is empty), several properties and fields are still holding onto an item that was last selected. I don't see any logic that would clear this field after things are deselected or cleared.

These fields need to be cleared when selection is cleared (directly or via selected items being deleted):

https://github.com/dotnet/wpf/blob/b63c69eaf5b58e758765a37bb18064bbc94832ad/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGrid.cs#L8652

https://github.com/dotnet/wpf/blob/b63c69eaf5b58e758765a37bb18064bbc94832ad/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGrid.cs#L2806

https://github.com/dotnet/wpf/blob/b63c69eaf5b58e758765a37bb18064bbc94832ad/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGrid.cs#L3423

and potentially

https://github.com/dotnet/wpf/blob/b63c69eaf5b58e758765a37bb18064bbc94832ad/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGrid.cs#L2849

https://github.com/dotnet/wpf/blob/b63c69eaf5b58e758765a37bb18064bbc94832ad/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGrid.cs#L2882

As a workaround, I wrote an extension method that you call after setting the ItemsSource on the DataGrid:

        private static readonly FieldInfo _selectionAnchorField = typeof(DataGrid).GetField("_selectionAnchor", BindingFlags.Instance | BindingFlags.NonPublic);
        private static readonly FieldInfo _focusedInfoField = typeof(ItemsControl).GetField("_focusedInfo", BindingFlags.Instance | BindingFlags.NonPublic);
        private static readonly PropertyInfo FocusedCellProperty = typeof(DataGrid).GetProperty("FocusedCell", BindingFlags.Instance | BindingFlags.NonPublic);

        /// <summary>
        /// https://github.com/dotnet/wpf/issues/6983
        /// </summary>
        public static void FixDataGridClearingLeak(this DataGrid dataGrid)
        {
            var items = dataGrid.ItemsSource;
            if (items is IEnumerable)
            {
                var view = CollectionViewSource.GetDefaultView(items);
                view.CollectionChanged += (s, e) =>
                {
                    if (view.IsEmpty)
                    {
                        _selectionAnchorField.SetValue(dataGrid, null);
                        _focusedInfoField.SetValue(dataGrid, null);
                        FocusedCellProperty.SetValue(dataGrid, null);

                        dataGrid.CurrentItem = null;
                        dataGrid.CurrentCell = default;
                        dataGrid.CurrentColumn = null;
                    }
                };
            }
        }
KirillOsenkov commented 1 year ago

Also I'm seeing retention paths from Automation peers that I don't know what to do about:

image

Not sure why DataGridCellsPresenter is being kept alive, but looks like _focusedInfo isn't being cleared as well: https://github.com/dotnet/wpf/blob/4912f487b97f8316c9cded79d66212933d31ccec/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/ItemsControl.cs#L3242

KirillOsenkov commented 1 year ago

Update: after another GC those AutomationPeers have fell off, so it looks like my proposed workaround code is all that was needed to fix the leak.