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.

Sign up for GitHub

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

Jump to bottom

[ios] fix memory leak with Page + Layout #14108

Merged
merged 1 commit into from
Mar 29, 2023
Merged

[ios] fix memory leak with Page + Layout #14108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,15 +298,19 @@ public async Task DoesNotLeak()

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

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
// 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!");
Expand Down
2 changes: 1 addition & 1 deletion src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
{
await OnLoadedAsync(shell.CurrentPage);

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

await shell.Navigation.PushAsync(page);
Expand Down
10 changes: 1 addition & 9 deletions src/Core/src/Handlers/Layout/LayoutHandler.iOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,7 @@ protected override LayoutView CreatePlatformView()
throw new InvalidOperationException($"{nameof(VirtualView)} must be set to create a LayoutViewGroup");
}

var view = new LayoutView
{
CrossPlatformMeasure = VirtualView.CrossPlatformMeasure,
CrossPlatformArrange = VirtualView.CrossPlatformArrange,
};

return view;
return new();
}

public override void SetVirtualView(IView view)
Expand All @@ -32,8 +26,6 @@ public override void SetVirtualView(IView view)
_ = MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} should have been set by base class.");

PlatformView.View = view;
PlatformView.CrossPlatformMeasure = VirtualView.CrossPlatformMeasure;
PlatformView.CrossPlatformArrange = VirtualView.CrossPlatformArrange;

// Remove any previous children
PlatformView.ClearSubviews();
Expand Down
17 changes: 10 additions & 7 deletions src/Core/src/Platform/iOS/LayoutView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Drawing;
using CoreGraphics;
using Microsoft.Maui.Graphics;
using UIKit;
Expand All @@ -15,15 +16,15 @@ public class LayoutView : MauiView
// apply to ViewHandlerExtensions.MeasureVirtualView
public override CGSize SizeThatFits(CGSize size)
{
if (CrossPlatformMeasure == null)
if (View is not ILayout layout)
{
return base.SizeThatFits(size);
}

var width = size.Width;
var height = size.Height;

var crossPlatformSize = CrossPlatformMeasure(width, height);
var crossPlatformSize = layout.CrossPlatformMeasure(width, height);
_measureValid = true;

return crossPlatformSize.ToCGSize();
Expand All @@ -36,15 +37,20 @@ public override void LayoutSubviews()
{
base.LayoutSubviews();

if (View is not ILayout layout)
{
return;
}

var bounds = AdjustForSafeArea(Bounds).ToRectangle();

if (!_measureValid)
{
CrossPlatformMeasure?.Invoke(bounds.Width, bounds.Height);
layout.CrossPlatformMeasure(bounds.Width, bounds.Height);
_measureValid = true;
}

CrossPlatformArrange?.Invoke(bounds);
layout.CrossPlatformArrange(bounds);
}

public override void SetNeedsLayout()
Expand All @@ -68,9 +74,6 @@ public override void WillRemoveSubview(UIView uiview)
Superview?.SetNeedsLayout();
}

internal Func<double, double, Size>? CrossPlatformMeasure { get; set; }
internal Func<Rect, Size>? CrossPlatformArrange { get; set; }
Comment on lines -71 to -72
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these, because you can just call View.CrossPlatformMeasure() directly.

Otherwise we might need to unset them in places, like in 7d0af63.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these, because you can just call View.CrossPlatformMeasure() directly.

The design intent here was for the handlers to be able to directly modify the xplat measure/arrange methods directly when necessary. It's not currently used on iOS (mostly for accidental historical reasons), but the other platforms do make use of it. The intent was to keep the designs symmetric on all the platforms.

(The main place you can see this at work in the Android and Windows ScrollViewHandlers.)

So I'd rather not remove these. Or, if there's a compelling memory-leak reason to remove them, we'll need to work out a way to inject custom xplat measure/arrange methods from the handlers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design intent here was for the handlers to be able to directly modify the xplat measure/arrange methods directly when necessary.

Is this design documented? So other people will know about it?

Or, if there's a compelling memory-leak reason to remove them, we'll need to work out a way to inject custom xplat measure/arrange methods from the handlers.

The leak appears to return if I put them back. It makes the shell & navigation DoNotLeak tests fail. Will share a *.gcdump file tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this design documented? So other people will know about it?

Clearly not well enough, if at all. 🙂


public override UIView HitTest(CGPoint point, UIEvent? uievent)
{
var result = base.HitTest(point, uievent);
Expand Down
11 changes: 9 additions & 2 deletions src/Core/src/Platform/iOS/MauiView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using CoreGraphics;
using System;
using CoreGraphics;
using ObjCRuntime;
using UIKit;

Expand All @@ -8,7 +9,13 @@ public abstract class MauiView : UIView
{
static bool? _respondsToSafeArea;

public IView? View { get; set; }
WeakReference<IView>? _reference;

public IView? View
{
get => _reference != null && _reference.TryGetTarget(out var v) ? v : null;
set => _reference = value == null ? null : new(value);
}

bool RespondsToSafeArea()
{
Expand Down