Skip to content

Commit

Permalink
[ios] fix memory leaks in ContentView
Browse files Browse the repository at this point in the history
Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: dotnet#14664

There are a couple cycles on iOS that causes memory leaks in all
controls based on `ContentView`:

* `Microsoft.Maui.Controls.ContentView` ->
    * `ContentViewHandler` ->
    * `Microsoft.Maui.Platform` ->
    * `CrossPlatformArrange/Measure` property ->
* `Microsoft.Maui.Controls.ContentView` cycle

To fix this, I made `CrossPlatformArrange/Measure` properties obsolete,
using the weak `View` property directly instead. This applies to various
other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`.

I did not yet fix `ScrollView` that appears to be more complicated to
solve. For now, it supports both code paths.

`ScrollView` will continue to have leaks in:

* Closures with captured variables
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227

* `CrossPlatformArrange/Measure`
    * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186
  • Loading branch information
jonathanpeppers authored and rmarinho committed May 30, 2023
1 parent 75347d5 commit 495de5b
Show file tree
Hide file tree
Showing 16 changed files with 90 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ protected override void OnElementChanged(ElementChangedEventArgs<Frame> e)

if (e.NewElement != null)
{
_actualView.CrossPlatformArrange = (e.NewElement as IContentView).CrossPlatformArrange;
_actualView.CrossPlatformMeasure = (e.NewElement as IContentView).CrossPlatformMeasure;

_actualView.View = e.NewElement;
SetupLayer();
UpdateShadow();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,42 @@ await InvokeOnMainThreadAsync(() =>
Assert.True(child.BindingContext == bindingContext);
});
}

[Fact(DisplayName = "ContentView Does Not Leak")]
public async Task DoesNotLeak()
{
SetupBuilder();
WeakReference viewReference = null;
WeakReference handlerReference = null;
WeakReference platformReference = null;

{
var view = new Microsoft.Maui.Controls.ContentView();
var page = new ContentPage { Content = view };
await CreateHandlerAndAddToWindow(page, () =>
{
viewReference = new(view);
handlerReference = new(view.Handler);
platformReference = new(view.Handler.PlatformView);
page.Content = null;
});
}

Assert.NotNull(viewReference);
Assert.NotNull(platformReference);
Assert.NotNull(handlerReference);

// Multiple GCs are sometimes required on iOS
for (int i = 0; i < 3; i++)
{
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
}

Assert.False(viewReference.IsAlive, "View should not be alive!");
Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
Assert.False(platformReference.IsAlive, "PlatformView should not be alive!");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ void SetupBuilder()
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler<Button, ButtonHandler>();
handlers.AddHandler<ContentView, ContentViewHandler>();
handlers.AddHandler<CollectionView, CollectionViewHandler>();
});
});
Expand Down Expand Up @@ -269,6 +270,7 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
{
new Label(),
new Button(),
new ContentView(),
new CollectionView(),
}
};
Expand Down
3 changes: 3 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.Maui.Hosting;
using Microsoft.Maui.Platform;
using Xunit;
using ContentView = Microsoft.Maui.Controls.ContentView;

#if ANDROID || IOS || MACCATALYST
using ShellHandler = Microsoft.Maui.Controls.Handlers.Compatibility.ShellRenderer;
Expand All @@ -40,6 +41,7 @@ void SetupBuilder()
SetupShellHandlers(handlers);
handlers.AddHandler(typeof(NavigationPage), typeof(NavigationViewHandler));
handlers.AddHandler(typeof(Button), typeof(ButtonHandler));
handlers.AddHandler(typeof(ContentView), typeof(ContentViewHandler));
handlers.AddHandler(typeof(CollectionView), typeof(CollectionViewHandler));
});
});
Expand Down Expand Up @@ -982,6 +984,7 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
{
new Label(),
new Button(),
new ContentView(),
new CollectionView(),
}
};
Expand Down
8 changes: 1 addition & 7 deletions src/Core/src/Handlers/Border/BorderHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ protected override ContentView CreatePlatformView()
_ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} must be set to create a {nameof(ContentView)}");
_ = MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} cannot be null");

return new ContentView
{
CrossPlatformMeasure = VirtualView.CrossPlatformMeasure,
CrossPlatformArrange = VirtualView.CrossPlatformArrange
};
return new ContentView { View = VirtualView };
}

protected override void ConnectHandler(ContentView platformView)
Expand All @@ -40,8 +36,6 @@ public override void SetVirtualView(IView view)
_ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} should have been set by base class.");

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

static void UpdateContent(IBorderHandler handler)
Expand Down
10 changes: 1 addition & 9 deletions src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ protected override ContentView CreatePlatformView()
_ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} must be set to create a {nameof(ContentView)}");
_ = MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} cannot be null");

return new ContentView
{
CrossPlatformMeasure = VirtualView.CrossPlatformMeasure,
CrossPlatformArrange = VirtualView.CrossPlatformArrange
};
return new ContentView { View = VirtualView };
}

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

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

static void UpdateContent(IContentViewHandler handler)
Expand Down Expand Up @@ -56,8 +50,6 @@ public static void MapContent(IContentViewHandler handler, IContentView page)

protected override void DisconnectHandler(ContentView platformView)
{
platformView.CrossPlatformMeasure = null;
platformView.CrossPlatformArrange = null;
platformView.RemoveFromSuperview();
base.DisconnectHandler(platformView);
}
Expand Down
2 changes: 0 additions & 2 deletions src/Core/src/Handlers/RadioButton/RadioButtonHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ public override void SetVirtualView(IView view)
_ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} should have been set by base class.");

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

static void UpdateContent(IRadioButtonHandler handler)
Expand Down
4 changes: 4 additions & 0 deletions src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,15 @@ static void InsertContentView(UIScrollView platformScrollView, IScrollView scrol
var contentContainer = new ContentView()
{
View = scrollView.PresentedContent,
#pragma warning disable CS0618 // Type or member is obsolete
CrossPlatformMeasure = ConstrainToScrollView(scrollView.CrossPlatformMeasure, platformScrollView, scrollView),
#pragma warning restore CS0618
Tag = ContentPanelTag
};

#pragma warning disable CS0618 // Type or member is obsolete
contentContainer.CrossPlatformArrange = ArrangeScrollViewContent(scrollView.CrossPlatformArrange, contentContainer, platformScrollView, scrollView);
#pragma warning restore CS0618

platformScrollView.ClearSubviews();
contentContainer.AddSubview(platformContent);
Expand Down
19 changes: 1 addition & 18 deletions src/Core/src/Handlers/SwipeItemView/SwipeItemViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,7 @@ namespace Microsoft.Maui.Handlers
{
public partial class SwipeItemViewHandler : ViewHandler<ISwipeItemView, ContentView>, ISwipeItemViewHandler
{
protected override ContentView CreatePlatformView()
{
return new ContentView
{
CrossPlatformMeasure = VirtualView.CrossPlatformMeasure,
CrossPlatformArrange = VirtualView.CrossPlatformArrange
};
}

public override void SetVirtualView(IView view)
{
base.SetVirtualView(view);
_ = VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} should have been set by base class.");
_ = PlatformView ?? throw new InvalidOperationException($"{nameof(PlatformView)} should have been set by base class.");

PlatformView.CrossPlatformMeasure = VirtualView.CrossPlatformMeasure;
PlatformView.CrossPlatformArrange = VirtualView.CrossPlatformArrange;
}
protected override ContentView CreatePlatformView() => new ContentView { View = VirtualView };

void UpdateContent()
{
Expand Down
8 changes: 2 additions & 6 deletions src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ protected override MauiSwipeView CreatePlatformView()
{
return new MauiSwipeView
{
CrossPlatformMeasure = VirtualView.CrossPlatformMeasure,
CrossPlatformArrange = VirtualView.CrossPlatformArrange,
Element = VirtualView
View = VirtualView
};
}

Expand All @@ -21,9 +19,7 @@ public override void SetVirtualView(IView view)
base.SetVirtualView(view);
_ = PlatformView ?? throw new InvalidOperationException($"{nameof(PlatformView)} should have been set by base class.");

PlatformView.Element = VirtualView;
PlatformView.CrossPlatformMeasure = VirtualView.CrossPlatformMeasure;
PlatformView.CrossPlatformArrange = VirtualView.CrossPlatformArrange;
PlatformView.View = VirtualView;
}

public static void MapContent(ISwipeViewHandler handler, ISwipeView view)
Expand Down
39 changes: 35 additions & 4 deletions src/Core/src/Platform/iOS/ContentView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,24 @@ public ContentView()

public override CGSize SizeThatFits(CGSize size)
{
if (CrossPlatformMeasure == null)
#pragma warning disable CS0618 // Type or member is obsolete
var crossPlatformMeasure = CrossPlatformMeasure;
#pragma warning restore CS0618 // Type or member is obsolete

if (crossPlatformMeasure is null && View is IContentView view)
{
crossPlatformMeasure = view.CrossPlatformMeasure;
}

if (crossPlatformMeasure is null)
{
return base.SizeThatFits(size);
}

var widthConstraint = size.Width;
var heightConstraint = size.Height;

var crossPlatformSize = CrossPlatformMeasure(widthConstraint, heightConstraint);
var crossPlatformSize = crossPlatformMeasure(widthConstraint, heightConstraint);

CacheMeasureConstraints(widthConstraint, heightConstraint);

Expand All @@ -38,6 +47,26 @@ public override void LayoutSubviews()
{
base.LayoutSubviews();

#pragma warning disable CS0618 // Type or member is obsolete
var crossPlatformMeasure = CrossPlatformMeasure;
var crossPlatformArrange = CrossPlatformArrange;
#pragma warning restore CS0618 // Type or member is obsolete

var view = View as IContentView;
if (crossPlatformMeasure is null && view is not null)
{
crossPlatformMeasure = view.CrossPlatformMeasure;
}
if (crossPlatformArrange is null && view is not null)
{
crossPlatformArrange = view.CrossPlatformArrange;
}

if (crossPlatformMeasure is null || crossPlatformArrange is null)
{
return;
}

var bounds = AdjustForSafeArea(Bounds).ToRectangle();
var widthConstraint = bounds.Width;
var heightConstraint = bounds.Height;
Expand All @@ -49,11 +78,11 @@ public override void LayoutSubviews()

if (!IsMeasureValid(widthConstraint, heightConstraint) && Superview is not MauiView)
{
CrossPlatformMeasure?.Invoke(widthConstraint, heightConstraint);
crossPlatformMeasure(widthConstraint, heightConstraint);
CacheMeasureConstraints(widthConstraint, heightConstraint);
}

CrossPlatformArrange?.Invoke(bounds);
crossPlatformArrange(bounds);

if (ChildMaskLayer != null)
ChildMaskLayer.Frame = bounds;
Expand All @@ -70,7 +99,9 @@ public override void SetNeedsLayout()
Superview?.SetNeedsLayout();
}

[Obsolete("Use View instead. This member causes memory leaks.")]
internal Func<double, double, Size>? CrossPlatformMeasure { get; set; }
[Obsolete("Use View instead. This member causes memory leaks.")]
internal Func<Rect, Size>? CrossPlatformArrange { get; set; }

internal IBorderStroke? Clip
Expand Down
2 changes: 1 addition & 1 deletion src/Core/src/Platform/iOS/MauiSwipeView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class MauiSwipeView : ContentView
bool _isResettingSwipe;
bool _isOpen;
OpenSwipeItem _previousOpenSwipeItem;
internal ISwipeView? Element { get; set; }
internal ISwipeView? Element => View as ISwipeView;

public MauiSwipeView()
{
Expand Down
9 changes: 1 addition & 8 deletions src/Core/src/Platform/iOS/PageViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,7 @@ public PageViewController(IView page, IMauiContext mauiContext)
LoadFirstView(page);
}

protected override UIView CreatePlatformView(IElement view)
{
return new ContentView
{
CrossPlatformArrange = ((IContentView)view).CrossPlatformArrange,
CrossPlatformMeasure = ((IContentView)view).CrossPlatformMeasure
};
}
protected override UIView CreatePlatformView(IElement view) => new ContentView { View = (IContentView)view };

public override void TraitCollectionDidChange(UITraitCollection? previousTraitCollection)
{
Expand Down
3 changes: 1 addition & 2 deletions src/Core/src/Platform/iOS/SemanticSwitchContentView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ internal class SemanticSwitchContentView : ContentView

internal SemanticSwitchContentView(IContentView virtualView)
{
CrossPlatformMeasure = virtualView.CrossPlatformMeasure;
CrossPlatformArrange = virtualView.CrossPlatformArrange;
View = virtualView;
IsAccessibilityElement = true;
}

Expand Down
1 change: 0 additions & 1 deletion src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2103,7 +2103,6 @@ override Microsoft.Maui.Handlers.StepperHandler.CreatePlatformView() -> UIKit.UI
override Microsoft.Maui.Handlers.StepperHandler.DisconnectHandler(UIKit.UIStepper! platformView) -> void
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.CreatePlatformElement() -> UIKit.UIButton!
override Microsoft.Maui.Handlers.SwipeItemViewHandler.CreatePlatformView() -> Microsoft.Maui.Platform.ContentView!
override Microsoft.Maui.Handlers.SwipeItemViewHandler.SetVirtualView(Microsoft.Maui.IView! view) -> void
override Microsoft.Maui.Handlers.SwipeViewHandler.CreatePlatformView() -> Microsoft.Maui.Platform.MauiSwipeView!
override Microsoft.Maui.Handlers.SwipeViewHandler.SetVirtualView(Microsoft.Maui.IView! view) -> void
override Microsoft.Maui.Handlers.SwitchHandler.ConnectHandler(UIKit.UISwitch! platformView) -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2102,7 +2102,6 @@ override Microsoft.Maui.Handlers.StepperHandler.CreatePlatformView() -> UIKit.UI
override Microsoft.Maui.Handlers.StepperHandler.DisconnectHandler(UIKit.UIStepper! platformView) -> void
override Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.CreatePlatformElement() -> UIKit.UIButton!
override Microsoft.Maui.Handlers.SwipeItemViewHandler.CreatePlatformView() -> Microsoft.Maui.Platform.ContentView!
override Microsoft.Maui.Handlers.SwipeItemViewHandler.SetVirtualView(Microsoft.Maui.IView! view) -> void
override Microsoft.Maui.Handlers.SwipeViewHandler.CreatePlatformView() -> Microsoft.Maui.Platform.MauiSwipeView!
override Microsoft.Maui.Handlers.SwipeViewHandler.SetVirtualView(Microsoft.Maui.IView! view) -> void
override Microsoft.Maui.Handlers.SwitchHandler.ConnectHandler(UIKit.UISwitch! platformView) -> void
Expand Down

0 comments on commit 495de5b

Please sign in to comment.