Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in ListView #1243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

4nonym0us
Copy link

Fixes a memory leak explained in #1242:

OnLoaded method of the ListView from WPF-UI uses DependencyPropertyDescriptor to invoke AddValueChanged, but never unregisters the event handler using RemoveValueChanged, which leads to a memory leak because a strong refence is created to the component.

Pull request type

Please check the type of change your PR introduces:

  • Update
  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes

What is the current behavior?

ListView leaks memory due to RemoveValueChanged is never called and the dangling reference to the control is being hold forever.

Issue Number: #1242

What is the new behavior?

  • ListView now subscribes to Unloaded event and removes the subscription to ValueChanged event of the View.

Other information

A basic unit test which displays the difference between WpfUi's ListView vs regular ListView in terms of being successfully collected by GC
public class UnitTest1
{
    [Fact]
    public Task TestWpfUiControlsListView()
    {
        return TestListViewMemoryLeakAsync<Wpf.Ui.Controls.ListView>();
    }

    [Fact]
    public Task TestSystemWindowsControlListView()
    {
        return TestListViewMemoryLeakAsync<System.Windows.Controls.ListView>();
    }

    public async Task TestListViewMemoryLeakAsync<T>() where T : ListView, new()
    {
        WeakReference? wr = null;

        await StartSTATask(() =>
        {
            var listView = new T();

            wr = new WeakReference(listView);

            listView.RaiseEvent(new RoutedEventArgs(FrameworkElement.LoadedEvent));
            listView.RaiseEvent(new RoutedEventArgs(FrameworkElement.UnloadedEvent));
        });

        GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, true);

        Assert.NotNull(wr);
        Assert.False(wr.IsAlive);
    }

    public static Task StartSTATask(Action action)
    {
        var tcs = new TaskCompletionSource<object>();
        var thread = new Thread(() =>
        {
            try
            {
                action();
                tcs.SetResult(new object());
            }
            catch (Exception e)
            {
                tcs.SetException(e);
            }
        });
        thread.SetApartmentState(ApartmentState.STA);
        thread.Start();
        return tcs.Task;
    }
}
  • Before:
    image

  • After:
    image

I'd also like to recommend adding some basic unit-tests that would test (almost) all controls to ensure that the same basic mistake is not repeated in a future

This test finds all classes that are assignable to Control from Wpf.Ui. namespace that have parameterless constructor and make an attempt to perform the same test that was used for the ListView in the previous code listing.

Fun fact: in addition to ListView, it also reports DataGrid to also have a memory leak. I didn't dig into it so it could be some false positive, which doesn't happen in real life scenario, but what I could tell is that the issue was related specifically to the components used within DataGrid.xaml (commenting out entire markup fixed the unit test).

public class MemoryLeakTests(ITestOutputHelper output)
{
    [Fact]
    public async Task TestAllControls()
    {
        // Find classes that are assignable to Control from Wpf.Ui. namespace that have parameterless constructor.
        var controlTypes = typeof(FluentWindow).Assembly.GetTypes()
            .Where(t => t.IsAssignableTo(typeof(Control)) &&
                        t.Namespace!.StartsWith("Wpf.Ui.") &&
                        t.GetConstructor(Type.EmptyTypes) != null)
            .ToList();

        output.WriteLine($"Found {controlTypes.Count} control types.");

        foreach (Type controlType in controlTypes)
        {
            output.WriteLine(controlType.FullName);
        }

        controlTypes.Remove(typeof(Wpf.Ui.Controls.DataGrid));
        
        foreach (var controlType in controlTypes)
        {
            await TestControlViewMemoryLeakAsync(controlType);
        }
    }

    public async Task TestControlViewMemoryLeakAsync(Type type)
    {
        WeakReference? wr = null;

        await StartSTATask(() =>
        {
            try
            {
                var control = Activator.CreateInstance(type) as Control;

                Assert.NotNull(control);

                wr = new WeakReference(control);

                control.RaiseEvent(new RoutedEventArgs(FrameworkElement.LoadedEvent));
                control.RaiseEvent(new RoutedEventArgs(FrameworkElement.UnloadedEvent));
            }
            catch(Exception e)
            {
                output.WriteLine($"Failed to test {type.FullName} because of an exception during object construction: {e}");
            }
        });

        GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, true);

        Assert.NotNull(wr);
        Assert.False(wr.IsAlive, $"Memory leak detected in {type.FullName}.");

        output.WriteLine($"Test passed for {type.FullName}.");
    }

    public async Task TestControlViewMemoryLeakAsync<T>() where T : Control, new()
    {
        await TestControlViewMemoryLeakAsync(typeof(T));
    }

    public static Task StartSTATask(Action action)
    {
        var tcs = new TaskCompletionSource<object>();
        var thread = new Thread(() =>
        {
            try
            {
                action();
                tcs.SetResult(new object());
            }
            catch (Exception e)
            {
                tcs.SetException(e);
            }
        });
        thread.SetApartmentState(ApartmentState.STA);
        thread.Start();
        return tcs.Task;
    }
}

Aforementioned unit test are not included in the PR (doesn't look like there are many in this repo at all), but I can commit the as well if requested.

@github-actions github-actions bot added controls Changes to the appearance or logic of custom controls. PR Pull request dotnet release labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controls Changes to the appearance or logic of custom controls. dotnet PR Pull request release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant