Skip to content

Commit

Permalink
[windows] fix memory leak in TabbedPage
Browse files Browse the repository at this point in the history
Context: dotnet#23166

This expands upon dotnet#23166 (iOS/Catalyst memory leaks for `TabbedPage`),
fixing the same problem on Windows. I could reproduce the problem in
`MemoryTests.cs`.

I found the `NavigationViewItemViewModel.Data` property held the
`ContentPage` that held onto the `TabbedPage`. I made the backing field
a `WeakReference` and the test in `MemoryTests.cs` now passes on Windows.
  • Loading branch information
jonathanpeppers committed Jun 28, 2024
1 parent 3e3cd67 commit 75325f1
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 16 deletions.
27 changes: 12 additions & 15 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,9 @@ void SetupBuilder()
[InlineData(typeof(TabbedPage))]
public async Task PagesDoNotLeak(Type type)
{
#if WINDOWS
// FIXME: there is still an issue with TabbedPage on Windows
if (type == typeof(TabbedPage))
return;
#endif

SetupBuilder();

WeakReference viewReference = null;
WeakReference handlerReference = null;
WeakReference platformViewReference = null;

var references = new List<WeakReference>();
var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });

await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
Expand All @@ -111,16 +102,22 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
await navPage.Navigation.PushModalAsync(page);
viewReference = new WeakReference(page);
handlerReference = new WeakReference(page.Handler);
platformViewReference = new WeakReference(page.Handler.PlatformView);
references.Add(new(page));
references.Add(new(page.Handler));
references.Add(new(page.Handler.PlatformView));
// Windows requires Loaded event to fire before unloading
await OnLoadedAsync(pageToWaitFor);
if (pageToWaitFor != page)
{
references.Add(new(pageToWaitFor));
references.Add(new(pageToWaitFor.Handler));
references.Add(new(pageToWaitFor.Handler.PlatformView));
}
await navPage.Navigation.PopModalAsync();
});

await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
await AssertionExtensions.WaitForGC(references.ToArray());
}

[Theory("Handler Does Not Leak")]
Expand Down
7 changes: 6 additions & 1 deletion src/Core/src/Platform/Windows/NavigationViewItemViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ internal class NavigationViewItemViewModel : INotifyPropertyChanged
WBrush? _unselectedForeground;
ObservableCollection<NavigationViewItemViewModel>? _menuItemsSource;
WIconElement? _icon;
WeakReference<object>? _data;

public object? Content
{
Expand All @@ -112,7 +113,11 @@ public WBrush? Background
get => IsSelected ? SelectedBackground : UnselectedBackground;
}

public object? Data { get; set; }
public object? Data
{
get => _data?.GetTargetOrDefault();
set => _data = value is null ? null : new(value);
}

public ObservableCollection<NavigationViewItemViewModel>? MenuItemsSource
{
Expand Down

0 comments on commit 75325f1

Please sign in to comment.