[controls] fix memory leak with Grid Row/ColumnDefinitions

[jonathanpeppers]

Fixes: #15860
Fixes: #15986

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!");
}

maui/src/Controls/src/Core/Layout/Grid.cs

Line 20 in cda3eb3

colDef.ItemSizeChanged += ((Grid)bindable).DefinitionsChanged;

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:

maui/src/Controls/src/Core/Application/Application.cs

Lines 221 to 225 in cda3eb3

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

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 #15860 to see if there are further issues.

[jonathanpeppers]

There are some existing tests that check this event still fires:

maui/src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs

Lines 77 to 78 in cda3eb3

	[Fact]
	public void ChangingChildRowInvalidatesGrid()

maui/src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs

Lines 98 to 99 in cda3eb3

	[Fact]
	public void ChangingChildColumnInvalidatesGrid()

[jonathanpeppers]

I did some testing and I think this does actually fix: #15860

[rachelkang]

The simplest solution is to make the ItemSizeChanged event use WeakEventManager as was done for other events

How does that work exactly? How does using WeakEventManager solve the issue of the DefinitionCollection living indefinitely?

(not reviewing, just asking questions to learn)

[jonathanpeppers]

How does using WeakEventManager solve the issue of the DefinitionCollection living indefinitely?

When you make a C# event:

class Child {
    public event EventHandler MyEvent;

If you are inside a class Parent and do:

child.MyEvent += this.OnMyEvent; // let's say this is inside the Parent's constructor

void OnMyEvent(object sender, EventArgs e) { }

The child now has a "strong" reference to the parent. If you put a breakpoint and looked at child.MyEvent the EventHandler has a Target property that points to the parent. This feels kind of backwards in the design of these two types: Parent should have a strong reference to Child like a List<Child> Children { get; set; }, but we now have the reverse, too. This is a general problem with all C# events.

WeakEventManager holds a WeakReference to the subscriber instead:

maui/src/Core/src/WeakEventManager.cs

Line 157 in ed18303

public readonly WeakReference? Subscriber;

A WeakReference is a special type that allows the subscriber to be collected by the GC. You can access WeakReference.IsAlive or WeakReference.Target will return null if the object is gone.

So now Grid has a strong reference to the DefinitionCollection via Grid.ColumnDefinitions, but DefinitionCollection only have a weak reference back to the Grid.

[rachelkang]

Interesting! Learned so much, thanks :)

This is a general problem with all C# events.

Would you say this design was intentional or more of an oversight? Is there anything that is generally good about the child having a strong reference to the parent?

[jonathanpeppers]

I think C# events have been like this since .NET Framework 1.0. Back then there was only WinForms, no WPF or MVVM patterns. Most events were used for things like Button.Click. It seems like it would harder to create memory leaks with only WinForms. It was definitely better than if you wrote an app back then in C++.

There is an api proposal about making "weak events" a thing:

• [API Proposal]: Add weak event listener helper class runtime#61517

[jonathanpeppers]

@rachelkang I wrote down some of the comments here for the future. Should be useful to us and customers:

https://github.com/dotnet/maui/wiki/Memory-Leaks

I didn't write a section about circular references in iOS/Catalyst yet. There is unfortunately more I need to add here...