From 495de5b16854acd083330d8996b38a5d04c89536 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Fri, 19 May 2023 11:05:50 -0500 Subject: [PATCH] [ios] fix memory leaks in ContentView Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS Related to: https://github.com/dotnet/maui/issues/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 --- .../Handlers/iOS/FrameRenderer.cs | 4 +- .../Elements/ContentView/ContentViewTests.cs | 37 ++++++++++++++++++ .../NavigationPage/NavigationPageTests.cs | 2 + .../DeviceTests/Elements/Shell/ShellTests.cs | 3 ++ .../src/Handlers/Border/BorderHandler.iOS.cs | 8 +--- .../ContentView/ContentViewHandler.iOS.cs | 10 +---- .../RadioButton/RadioButtonHandler.iOS.cs | 2 - .../ScrollView/ScrollViewHandler.iOS.cs | 4 ++ .../SwipeItemView/SwipeItemViewHandler.iOS.cs | 19 +-------- .../SwipeView/SwipeViewHandler.iOS.cs | 8 +--- src/Core/src/Platform/iOS/ContentView.cs | 39 +++++++++++++++++-- src/Core/src/Platform/iOS/MauiSwipeView.cs | 2 +- .../src/Platform/iOS/PageViewController.cs | 9 +---- .../Platform/iOS/SemanticSwitchContentView.cs | 3 +- .../PublicAPI/net-ios/PublicAPI.Shipped.txt | 1 - .../net-maccatalyst/PublicAPI.Shipped.txt | 1 - 16 files changed, 90 insertions(+), 62 deletions(-) diff --git a/src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs index 3ecd1611a487..5f16c61d2d14 100644 --- a/src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs +++ b/src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs @@ -51,9 +51,7 @@ protected override void OnElementChanged(ElementChangedEventArgs 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(); } diff --git a/src/Controls/tests/DeviceTests/Elements/ContentView/ContentViewTests.cs b/src/Controls/tests/DeviceTests/Elements/ContentView/ContentViewTests.cs index 258939f5580b..e963b2095315 100644 --- a/src/Controls/tests/DeviceTests/Elements/ContentView/ContentViewTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/ContentView/ContentViewTests.cs @@ -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!"); + } } } diff --git a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs index 81849abee6b9..2cb7a8bc0808 100644 --- a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs @@ -36,6 +36,7 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); }); }); @@ -269,6 +270,7 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async { new Label(), new Button(), + new ContentView(), new CollectionView(), } }; diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs index 0873ee199e45..ced8f7c3dfff 100644 --- a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs @@ -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; @@ -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)); }); }); @@ -982,6 +984,7 @@ await CreateHandlerAndAddToWindow(shell, async (handler) => { new Label(), new Button(), + new ContentView(), new CollectionView(), } }; diff --git a/src/Core/src/Handlers/Border/BorderHandler.iOS.cs b/src/Core/src/Handlers/Border/BorderHandler.iOS.cs index 353c5876fe7c..98b389ae3e7c 100644 --- a/src/Core/src/Handlers/Border/BorderHandler.iOS.cs +++ b/src/Core/src/Handlers/Border/BorderHandler.iOS.cs @@ -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) @@ -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) diff --git a/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs b/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs index 2a076b28fd2f..0ec22edae886 100644 --- a/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs +++ b/src/Core/src/Handlers/ContentView/ContentViewHandler.iOS.cs @@ -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) @@ -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) @@ -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); } diff --git a/src/Core/src/Handlers/RadioButton/RadioButtonHandler.iOS.cs b/src/Core/src/Handlers/RadioButton/RadioButtonHandler.iOS.cs index fc4fc8dccc9a..27e28635681e 100644 --- a/src/Core/src/Handlers/RadioButton/RadioButtonHandler.iOS.cs +++ b/src/Core/src/Handlers/RadioButton/RadioButtonHandler.iOS.cs @@ -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) diff --git a/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs b/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs index 3ea7c6ec287b..9213812cf6a2 100644 --- a/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs +++ b/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs @@ -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); diff --git a/src/Core/src/Handlers/SwipeItemView/SwipeItemViewHandler.iOS.cs b/src/Core/src/Handlers/SwipeItemView/SwipeItemViewHandler.iOS.cs index 879ece58de34..1c1adf67f59a 100644 --- a/src/Core/src/Handlers/SwipeItemView/SwipeItemViewHandler.iOS.cs +++ b/src/Core/src/Handlers/SwipeItemView/SwipeItemViewHandler.iOS.cs @@ -5,24 +5,7 @@ namespace Microsoft.Maui.Handlers { public partial class SwipeItemViewHandler : ViewHandler, 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() { diff --git a/src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs b/src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs index 0a0f57c40960..523c335b8154 100644 --- a/src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs +++ b/src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs @@ -10,9 +10,7 @@ protected override MauiSwipeView CreatePlatformView() { return new MauiSwipeView { - CrossPlatformMeasure = VirtualView.CrossPlatformMeasure, - CrossPlatformArrange = VirtualView.CrossPlatformArrange, - Element = VirtualView + View = VirtualView }; } @@ -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) diff --git a/src/Core/src/Platform/iOS/ContentView.cs b/src/Core/src/Platform/iOS/ContentView.cs index c142b3d44173..53fc64e13c77 100644 --- a/src/Core/src/Platform/iOS/ContentView.cs +++ b/src/Core/src/Platform/iOS/ContentView.cs @@ -19,7 +19,16 @@ 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); } @@ -27,7 +36,7 @@ public override CGSize SizeThatFits(CGSize size) var widthConstraint = size.Width; var heightConstraint = size.Height; - var crossPlatformSize = CrossPlatformMeasure(widthConstraint, heightConstraint); + var crossPlatformSize = crossPlatformMeasure(widthConstraint, heightConstraint); CacheMeasureConstraints(widthConstraint, heightConstraint); @@ -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; @@ -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; @@ -70,7 +99,9 @@ public override void SetNeedsLayout() Superview?.SetNeedsLayout(); } + [Obsolete("Use View instead. This member causes memory leaks.")] internal Func? CrossPlatformMeasure { get; set; } + [Obsolete("Use View instead. This member causes memory leaks.")] internal Func? CrossPlatformArrange { get; set; } internal IBorderStroke? Clip diff --git a/src/Core/src/Platform/iOS/MauiSwipeView.cs b/src/Core/src/Platform/iOS/MauiSwipeView.cs index 0e254fb0ce51..90684611043b 100644 --- a/src/Core/src/Platform/iOS/MauiSwipeView.cs +++ b/src/Core/src/Platform/iOS/MauiSwipeView.cs @@ -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() { diff --git a/src/Core/src/Platform/iOS/PageViewController.cs b/src/Core/src/Platform/iOS/PageViewController.cs index c2f994a65ec3..d9520567e6c7 100644 --- a/src/Core/src/Platform/iOS/PageViewController.cs +++ b/src/Core/src/Platform/iOS/PageViewController.cs @@ -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) { diff --git a/src/Core/src/Platform/iOS/SemanticSwitchContentView.cs b/src/Core/src/Platform/iOS/SemanticSwitchContentView.cs index 5205abc74972..0f0cad23c885 100644 --- a/src/Core/src/Platform/iOS/SemanticSwitchContentView.cs +++ b/src/Core/src/Platform/iOS/SemanticSwitchContentView.cs @@ -14,8 +14,7 @@ internal class SemanticSwitchContentView : ContentView internal SemanticSwitchContentView(IContentView virtualView) { - CrossPlatformMeasure = virtualView.CrossPlatformMeasure; - CrossPlatformArrange = virtualView.CrossPlatformArrange; + View = virtualView; IsAccessibilityElement = true; } diff --git a/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt b/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt index 33b506e9c9ff..62632937080a 100644 --- a/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt +++ b/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt @@ -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 diff --git a/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Shipped.txt b/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Shipped.txt index c309c73fac4d..3f04bcc9bd41 100644 --- a/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Shipped.txt +++ b/src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Shipped.txt @@ -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