Skip to content

Commit

Permalink
[ios] fix memory leak with Page + Layout (#14108)
Browse files Browse the repository at this point in the history
Further fixes: #13520

7d0af63 did not appear to be a complete fix for #13520, as when I
retested the customer sample, I found I needed to remove a
`VerticalStackLayout` for the problem to go away.

I could reproduce the issue in a test by doing:

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

After lots of debugging, I found the underlying issue was that
`MauiView` on iOS held a reference to the cross-platform `IView`.

After changing the backing field for this to be a `WeakReference<IView>`
the above problem appears to be solved.

I also removed `LayoutView.CrossplatformArrange/Measure` as we can just
use the `View` property directly instead.
  • Loading branch information
jonathanpeppers committed Mar 29, 2023
1 parent 0d9ac95 commit cbce20c
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 23 deletions.
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
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,7 @@ public async Task PagesDoNotLeak()
{
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
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
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; }

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
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

0 comments on commit cbce20c

Please sign in to comment.