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

[controls] fix memory leaks in Page & navigation #13833

Merged
merged 3 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ void OnPageChanged(Page? oldPage, Page? newPage)
{
if (oldPage != null)
{
_menuBarTracker.Target = null;
InternalChildren.Remove(oldPage);
oldPage.HandlerChanged -= OnPageHandlerChanged;
oldPage.HandlerChanging -= OnPageHandlerChanging;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ Task SetupWindowForTests<THandler>(IWindow window, Func<Task> runTests, IMauiCon
}
finally
{
window.Handler.DisconnectHandler();
window.Handler?.DisconnectHandler();
await Task.Delay(10);
newWindow?.Close();
appStub.Handler.DisconnectHandler();
appStub.Handler?.DisconnectHandler();
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ void SetupBuilder()
handlers.AddHandler<Page, PageHandler>();
handlers.AddHandler<Window, WindowHandlerStub>();
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<Label, LabelHandler>();
});
});
}
Expand Down Expand Up @@ -287,5 +288,47 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
};
});
}

[Fact(DisplayName = "NavigationPage Does Not Leak")]
public async Task DoesNotLeak()
{
SetupBuilder();
WeakReference pageReference = null;
var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async (handler) =>
{
var page = new ContentPage { Title = "Page 2" };
pageReference = new WeakReference(page);
await navPage.Navigation.PushAsync(page);
await navPage.Navigation.PopAsync();
});

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

Assert.NotNull(pageReference);
Assert.False(pageReference.IsAlive, "Page should not be alive!");
}

[Fact(DisplayName = "Can Reuse Pages")]
public async Task CanReusePages()
{
SetupBuilder();
var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });
var reusedPage = new ContentPage
{
Content = new Label()
};

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async (handler) =>
{
await navPage.Navigation.PushAsync(reusedPage);
await navPage.Navigation.PopAsync();
await navPage.Navigation.PushAsync(reusedPage);
await OnLoadedAsync(reusedPage.Content);
});
}
}
}
58 changes: 57 additions & 1 deletion src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
});
}

#if !IOS
#if !IOS && !MACCATALYST
[Fact]
public async Task ChangingToNewMauiContextDoesntCrash()
{
Expand Down Expand Up @@ -917,6 +917,62 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
}
#endif

[Fact(DisplayName = "Pages Do Not Leak")]
public async Task PagesDoNotLeak()
{
SetupBuilder();
var shell = await CreateShellAsync(shell =>
{
shell.CurrentItem = new ContentPage() { Title = "Page 1" };
});

WeakReference pageReference = null;

await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
{
await OnLoadedAsync(shell.CurrentPage);

var page = new ContentPage { Title = "Page 2" };
pageReference = new WeakReference(page);

await shell.Navigation.PushAsync(page);
await shell.Navigation.PopAsync();
});

// 3 GCs were required in Android API 23, 2 worked otherwise
for (int i = 0; i < 3; i++)
{
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
}

Assert.NotNull(pageReference);
Assert.False(pageReference.IsAlive, "Page should not be alive!");
}

[Fact(DisplayName = "Can Reuse Pages")]
public async Task CanReusePages()
{
SetupBuilder();
var rootPage = new ContentPage();
var shell = await CreateShellAsync(shell =>
{
shell.CurrentItem = rootPage;
});
var reusedPage = new ContentPage
{
Content = new Label { Text = "HEY" }
};
await CreateHandlerAndAddToWindow<IWindowHandler>(shell, async (handler) =>
{
await shell.Navigation.PushAsync(reusedPage);
await shell.Navigation.PopAsync();
await shell.Navigation.PushAsync(reusedPage);
await OnLoadedAsync(reusedPage.Content);
});
}

protected Task<Shell> CreateShellAsync(Action<Shell> action) =>
InvokeOnMainThreadAsync(() =>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ public static void MapContent(IContentViewHandler handler, IContentView page)
protected override void DisconnectHandler(ContentViewGroup platformView)
{
// If we're being disconnected from the xplat element, then we should no longer be managing its children
platformView.CrossPlatformMeasure = null;
platformView.CrossPlatformArrange = null;
platformView.RemoveAllViews();
base.DisconnectHandler(platformView);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,12 @@ public static void MapContent(IContentViewHandler handler, IContentView page)
{
UpdateContent(handler);
}

protected override void DisconnectHandler(ContentPanel platformView)
{
platformView.CrossPlatformMeasure = null;
platformView.CrossPlatformArrange = null;
base.DisconnectHandler(platformView);
}
}
}
8 changes: 8 additions & 0 deletions src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,13 @@ public static void MapContent(IContentViewHandler handler, IContentView page)
{
UpdateContent(handler);
}

protected override void DisconnectHandler(ContentView platformView)
{
platformView.CrossPlatformMeasure = null;
platformView.CrossPlatformArrange = null;
platformView.RemoveFromSuperview();
base.DisconnectHandler(platformView);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ public override void OnResume()
base.OnResume();
}

public override void OnDestroy()
{
_currentView = null;
_fragmentContainerView = null;

base.OnDestroy();
}

public override Animation OnCreateAnimation(int transit, bool enter, int nextAnim)
{
int id = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ override Microsoft.Maui.Handlers.RadioButtonHandler.PlatformArrange(Microsoft.Ma
override Microsoft.Maui.Handlers.ShapeViewHandler.GetDesiredSize(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool
override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int
override Microsoft.Maui.Platform.NavigationViewFragment.OnDestroy() -> void
override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool
override Microsoft.Maui.SizeRequest.GetHashCode() -> int
static Microsoft.Maui.FontSize.operator !=(Microsoft.Maui.FontSize left, Microsoft.Maui.FontSize right) -> bool
Expand Down
1 change: 1 addition & 0 deletions src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Microsoft.Maui.LifecycleEvents.iOSLifecycle.PerformFetch
Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool
Microsoft.Maui.Platform.MauiScrollView
Microsoft.Maui.Platform.MauiScrollView.MauiScrollView() -> void
override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentView! platformView) -> void
override Microsoft.Maui.Handlers.SwipeItemButton.Frame.get -> CoreGraphics.CGRect
override Microsoft.Maui.Handlers.SwipeItemButton.Frame.set -> void
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.ConnectHandler(UIKit.UIButton! platformView) -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Microsoft.Maui.Layouts.FlexBasis.Equals(Microsoft.Maui.Layouts.FlexBasis other)
Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool
Microsoft.Maui.Platform.MauiScrollView
Microsoft.Maui.Platform.MauiScrollView.MauiScrollView() -> void
override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentView! platformView) -> void
override Microsoft.Maui.Handlers.SwipeItemButton.Frame.get -> CoreGraphics.CGRect
override Microsoft.Maui.Handlers.SwipeItemButton.Frame.set -> void
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.ConnectHandler(UIKit.UIButton! platformView) -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Microsoft.Maui.Hosting.MauiApp.DisposeAsync() -> System.Threading.Tasks.ValueTas
Microsoft.Maui.Layouts.FlexBasis.Equals(Microsoft.Maui.Layouts.FlexBasis other) -> bool
Microsoft.Maui.Platform.MauiWebView.MauiWebView(Microsoft.Maui.Handlers.WebViewHandler! handler) -> void
Microsoft.Maui.SizeRequest.Equals(Microsoft.Maui.SizeRequest other) -> bool
override Microsoft.Maui.Handlers.ContentViewHandler.DisconnectHandler(Microsoft.Maui.Platform.ContentPanel! platformView) -> void
override Microsoft.Maui.Layouts.FlexBasis.Equals(object? obj) -> bool
override Microsoft.Maui.Layouts.FlexBasis.GetHashCode() -> int
override Microsoft.Maui.SizeRequest.Equals(object? obj) -> bool
Expand Down