Skip to content

Commit

Permalink
[controls] fix memory leak with Grid Row/ColumnDefinitions
Browse files Browse the repository at this point in the history
Context: dotnet#15860

In a customer's app, they have the setup:

    <!-- Resources/Styles/Styles.xaml -->
    <Style TargetType="Grid" x:Key="GridStyleWithColumnDefinitions">
        <Setter Property="ColumnDefinitions" Value="18,*"/>
    </Style>

Then a page with:

    <Grid Style="{StaticResource GridStyleWithColumnDefinitions}" />

Navigating forward & back from this page would show that the entire
`Page` would live forever! What is interesting, is simply removing the
Grid's `Style` solved the problem?!?

I narrowed this down to a unit test:

    [Fact]
    public async Task ColumnDefinitionDoesNotLeak()
    {
        // Long-lived column, like from a Style in App.Resources
        var column = new ColumnDefinition();
        WeakReference reference;

        {
            var grid = new Grid();
            grid.ColumnDefinitions.Add(column);
            reference = new(grid);
        }

        await Task.Yield();
        GC.Collect();
        GC.WaitForPendingFinalizers();

        Assert.False(reference.IsAlive, "Grid should not be alive!");
    }

https://github.com/dotnet/maui/blob/cda3eb3381cfb686567ed05e3eb8e8f26c02d785/src/Controls/src/Core/Layout/Grid.cs#L20

`Grid` subscribes to `DefinitionCollection<T>.ItemSizeChanged`, and in
this case the `DefinitionCollection<T>` lived indefinitely in an
application-wide `ResourceDictionary`.

The simplest solution is to make the `ItemSizeChanged` event use
`WeakEventManager` as was done for other events like
`Application.RequestedThemeChanged`:

https://github.com/dotnet/maui/blob/cda3eb3381cfb686567ed05e3eb8e8f26c02d785/src/Controls/src/Core/Application/Application.cs#L221-L225

Since MAUI owns the property, we can use the simple solution here.

This is one issue solved in the customer app, but I will need to retest
the app in dotnet#15860 to see if there are further issues.
  • Loading branch information
jonathanpeppers committed Jul 13, 2023
1 parent cda3eb3 commit 20cb950
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 5 deletions.
11 changes: 7 additions & 4 deletions src/Controls/src/Core/DefinitionCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ namespace Microsoft.Maui.Controls
{
public class DefinitionCollection<T> : IList<T>, ICollection<T> where T : IDefinition
{
readonly WeakEventManager _weakEventManager = new WeakEventManager();
readonly List<T> _internalList;

internal DefinitionCollection() => _internalList = new List<T>();
Expand Down Expand Up @@ -101,13 +102,15 @@ public void RemoveAt(int index)
OnItemSizeChanged(this, EventArgs.Empty);
}

public event EventHandler ItemSizeChanged;
public event EventHandler ItemSizeChanged
{
add => _weakEventManager.AddEventHandler(value);
remove => _weakEventManager.RemoveEventHandler(value);
}

void OnItemSizeChanged(object sender, EventArgs e)
{
EventHandler eh = ItemSizeChanged;
if (eh != null)
eh(this, EventArgs.Empty);
_weakEventManager.HandleEvent(this, e, nameof(ItemSizeChanged));
}
}
}
44 changes: 43 additions & 1 deletion src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using NSubstitute;
using Xunit;

Expand Down Expand Up @@ -184,5 +186,45 @@ public void ColumnDefinitionsGetBindingContext()

Assert.Equal(def2.BindingContext, context);
}

[Fact]
public async Task ColumnDefinitionDoesNotLeak()
{
// Long-lived column, like from a Style in App.Resources
var column = new ColumnDefinition();
WeakReference reference;

{
var grid = new Grid();
grid.ColumnDefinitions.Add(column);
reference = new(grid);
}

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(reference.IsAlive, "Grid should not be alive!");
}

[Fact]
public async Task RowDefinitionDoesNotLeak()
{
// Long-lived row, like from a Style in App.Resources
var row = new RowDefinition();
WeakReference reference;

{
var grid = new Grid();
grid.RowDefinitions.Add(row);
reference = new(grid);
}

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(reference.IsAlive, "Grid should not be alive!");
}
}
}

0 comments on commit 20cb950

Please sign in to comment.