[ios] fix memory leaks in various controls (#18434)

Fixes: * `ActivityIndicator` * `BoxView` * `Polygon` * `Polyline` * `IndicatorView` Some of these were based on `PlatformGraphicsView`, which had a circular reference, for example: * `BoxView` -> `BoxViewHandler`/`PlatformGraphicsViewHandler` -> `PlatformGraphicsView` -> `BoxView` * It appears both `IDrawable` and `IGraphicsRenderer` values in `PlatformGraphicsView` were circular references. Various shapes were based on `PlatformGraphicsView`. We don't have the iOS memory analyzer running on `Microsoft.Maui.Graphics.dll` yet. Similarly: * `ActivityIndicator` -> `ActivityIndicatorHandler` -> `MauiActivityIndicator` -> `ActivityIndicator` * `IndicatorView` -> `IndicatorViewHandler` -> `MauiPageControl` -> `IndicatorView` *Open question*: Why didn't the analyzer catch the issues with `ActivityIndicator` and `IndicatorView`? Investigating why.
dotnet · Oct 30, 2023 · a589b12 · a589b12
1 parent 6fa96e9
commit a589b12
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 46 deletions.
diff --git a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
@@ -1,7 +1,9 @@
 using System;
 using System.Threading.Tasks;
 using Microsoft.Maui.Controls;
+using Microsoft.Maui.Controls.Handlers;
 using Microsoft.Maui.Controls.Handlers.Compatibility;
+using Microsoft.Maui.Controls.Shapes;
 using Microsoft.Maui.Handlers;
 using Microsoft.Maui.Hosting;
 using Xunit;
@@ -17,7 +19,9 @@ void SetupBuilder()
 		{
 			builder.ConfigureMauiHandlers(handlers =>
 			{
+				handlers.AddHandler<ActivityIndicator, ActivityIndicatorHandler>();
 				handlers.AddHandler<Border, BorderHandler>();
+				handlers.AddHandler<BoxView, BoxViewHandler>();
 				handlers.AddHandler<CheckBox, CheckBoxHandler>();
 				handlers.AddHandler<DatePicker, DatePickerHandler>();
 				handlers.AddHandler<Entry, EntryHandler>();
@@ -26,31 +30,41 @@ void SetupBuilder()
 				handlers.AddHandler<Label, LabelHandler>();
 				handlers.AddHandler<ListView, ListViewRenderer>();
 				handlers.AddHandler<Picker, PickerHandler>();
+				handlers.AddHandler<Polygon, PolygonHandler>();
+				handlers.AddHandler<Polyline, PolylineHandler>();
 				handlers.AddHandler<IContentView, ContentViewHandler>();
 				handlers.AddHandler<Image, ImageHandler>();
+				handlers.AddHandler<IndicatorView, IndicatorViewHandler>();
 				handlers.AddHandler<RefreshView, RefreshViewHandler>();
 				handlers.AddHandler<IScrollView, ScrollViewHandler>();
 				handlers.AddHandler<SwipeView, SwipeViewHandler>();
 				handlers.AddHandler<TimePicker, TimePickerHandler>();
+				handlers.AddHandler<WebView, WebViewHandler>();
 			});
 		});
 	}
 
 	[Theory("Handler Does Not Leak")]
+	[InlineData(typeof(ActivityIndicator))]
 	[InlineData(typeof(Border))]
+	[InlineData(typeof(BoxView))]
 	[InlineData(typeof(ContentView))]
 	[InlineData(typeof(CheckBox))]
 	[InlineData(typeof(DatePicker))]
 	[InlineData(typeof(Entry))]
 	[InlineData(typeof(Editor))]
 	[InlineData(typeof(GraphicsView))]
 	[InlineData(typeof(Image))]
+	[InlineData(typeof(IndicatorView))]
 	[InlineData(typeof(Label))]
 	[InlineData(typeof(Picker))]
+	[InlineData(typeof(Polygon))]
+	[InlineData(typeof(Polyline))]
 	[InlineData(typeof(RefreshView))]
 	[InlineData(typeof(ScrollView))]
 	[InlineData(typeof(SwipeView))]
 	[InlineData(typeof(TimePicker))]
+	[InlineData(typeof(WebView))]
 	public async Task HandlerDoesNotLeak(Type type)
 	{
 		SetupBuilder();

diff --git a/src/Core/src/Platform/iOS/MauiActivityIndicator.cs b/src/Core/src/Platform/iOS/MauiActivityIndicator.cs
@@ -8,18 +8,21 @@ namespace Microsoft.Maui.Platform
 {
 	public class MauiActivityIndicator : UIActivityIndicatorView, IUIViewLifeCycleEvents
 	{
-		IActivityIndicator? _virtualView;
+		readonly WeakReference<IActivityIndicator>? _virtualView;
+
+		bool IsRunning => _virtualView is not null && _virtualView.TryGetTarget(out var a) ? a.IsRunning : false;
 
 		public MauiActivityIndicator(CGRect rect, IActivityIndicator? virtualView) : base(rect)
 		{
-			_virtualView = virtualView;
+			if (virtualView is not null)
+				_virtualView = new(virtualView);
 		}
 
 		public override void Draw(CGRect rect)
 		{
 			base.Draw(rect);
 
-			if (_virtualView?.IsRunning == true)
+			if (IsRunning)
 				StartAnimating();
 			else
 				StopAnimating();
@@ -29,7 +32,7 @@ public override void LayoutSubviews()
 		{
 			base.LayoutSubviews();
 
-			if (_virtualView?.IsRunning == true)
+			if (IsRunning)
 				StartAnimating();
 			else
 				StopAnimating();
@@ -38,10 +41,10 @@ public override void LayoutSubviews()
 		protected override void Dispose(bool disposing)
 		{
 			base.Dispose(disposing);
-
-			_virtualView = null;
 		}
 
+		readonly WeakEventManager _weakEventManager = new();
+
 		[UnconditionalSuppressMessage("Memory", "MA0002", Justification = IUIViewLifeCycleEvents.UnconditionalSuppressMessage)]
 		EventHandler? _movedToWindow;
 		event EventHandler IUIViewLifeCycleEvents.MovedToWindow

diff --git a/src/Core/src/Platform/iOS/MauiPageControl.cs b/src/Core/src/Platform/iOS/MauiPageControl.cs
@@ -1,6 +1,5 @@
 using System;
 using System.Diagnostics.CodeAnalysis;
-using System.Reflection.Metadata;
 using CoreGraphics;
 using ObjCRuntime;
 using UIKit;
@@ -11,7 +10,7 @@ public class MauiPageControl : UIPageControl, IUIViewLifeCycleEvents
 	{
 		const int DefaultIndicatorSize = 6;
 
-		IIndicatorView? _indicatorView;
+		WeakReference<IIndicatorView>? _indicatorView;
 		bool _updatingPosition;
 
 		public MauiPageControl()
@@ -30,7 +29,7 @@ public void SetIndicatorView(IIndicatorView? indicatorView)
 			{
 				ValueChanged -= MauiPageControlValueChanged;
 			}
-			_indicatorView = indicatorView;
+			_indicatorView = indicatorView is null ? null : new(indicatorView);
 
 		}
 
@@ -81,21 +80,21 @@ public void UpdatePosition()
 
 			int GetCurrentPage()
 			{
-				if (_indicatorView == null)
+				if (_indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView))
 					return -1;
 
-				var maxVisible = _indicatorView.GetMaximumVisible();
-				var position = _indicatorView.Position;
+				var maxVisible = indicatorView.GetMaximumVisible();
+				var position = indicatorView.Position;
 				var index = position >= maxVisible ? maxVisible - 1 : position;
 				return index;
 			}
 		}
 
 		public void UpdateIndicatorCount()
 		{
-			if (_indicatorView == null)
+			if (_indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView))
 				return;
-			this.UpdatePages(_indicatorView.GetMaximumVisible());
+			this.UpdatePages(indicatorView.GetMaximumVisible());
 			UpdatePosition();
 		}
 
@@ -136,10 +135,10 @@ void UpdateCornerRadius()
 
 		void MauiPageControlValueChanged(object? sender, System.EventArgs e)
 		{
-			if (_updatingPosition || _indicatorView == null)
+			if (_updatingPosition || _indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView))
 				return;
 
-			_indicatorView.Position = (int)CurrentPage;
+			indicatorView.Position = (int)CurrentPage;
 			//if we are iOS13 or lower and we are using a Square shape
 			//we need to update the CornerRadius of the new shape.
 			if (IsSquare && !(OperatingSystem.IsIOSVersionAtLeast(14) || OperatingSystem.IsTvOSVersionAtLeast(14)))

diff --git a/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs b/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Drawing;
 using CoreGraphics;
 using Foundation;
 using UIKit;
@@ -9,9 +8,9 @@ namespace Microsoft.Maui.Graphics.Platform
 	[Register(nameof(PlatformGraphicsView))]
 	public class PlatformGraphicsView : UIView
 	{
-		private IGraphicsRenderer _renderer;
+		private WeakReference<IGraphicsRenderer> _renderer;
 		private CGColorSpace _colorSpace;
-		private IDrawable _drawable;
+		private WeakReference<IDrawable> _drawable;
 		private CGRect _lastBounds;
 
 		public PlatformGraphicsView(CGRect frame, IDrawable drawable = null, IGraphicsRenderer renderer = null) : base(frame)
@@ -35,57 +34,53 @@ public PlatformGraphicsView(IntPtr aPtr) : base(aPtr)
 
 		public IGraphicsRenderer Renderer
 		{
-			get => _renderer;
+			get => _renderer is not null && _renderer.TryGetTarget(out var r) ? r : null;
 
 			set
 			{
-				if (_renderer != null)
+				var renderer = Renderer;
+				if (renderer != null)
 				{
-					_renderer.Drawable = null;
-					_renderer.GraphicsView = null;
-					_renderer.Dispose();
+					renderer.Drawable = null;
+					renderer.GraphicsView = null;
+					renderer.Dispose();
 				}
 
-				_renderer = value ?? new DirectRenderer();
+				renderer = value ?? new DirectRenderer();
+				_renderer = new(renderer);
 
-				_renderer.GraphicsView = this;
-				_renderer.Drawable = _drawable;
+				renderer.GraphicsView = this;
+				renderer.Drawable = Drawable;
 				var bounds = Bounds;
-				_renderer.SizeChanged((float)bounds.Width, (float)bounds.Height);
+				renderer.SizeChanged((float)bounds.Width, (float)bounds.Height);
 			}
 		}
 
 		public IDrawable Drawable
 		{
-			get => _drawable;
+			get => _drawable is not null && _drawable.TryGetTarget(out var d) ? d : null;
 			set
 			{
-				_drawable = value;
-				if (_renderer != null)
+				_drawable = new(value);
+				if (Renderer is IGraphicsRenderer renderer)
 				{
-					_renderer.Drawable = _drawable;
-					_renderer.Invalidate();
+					renderer.Drawable = value;
+					renderer.Invalidate();
 				}
 			}
 		}
 
-		public void InvalidateDrawable()
-		{
-			_renderer.Invalidate();
-		}
+		public void InvalidateDrawable() => Renderer?.Invalidate();
 
-		public void InvalidateDrawable(float x, float y, float w, float h)
-		{
-			_renderer.Invalidate(x, y, w, h);
-		}
+		public void InvalidateDrawable(float x, float y, float w, float h) => Renderer?.Invalidate(x, y, w, h);
 
 		public override void WillMoveToSuperview(UIView newSuperview)
 		{
 			base.WillMoveToSuperview(newSuperview);
 
-			if (newSuperview == null)
+			if (newSuperview == null && Renderer is IGraphicsRenderer renderer)
 			{
-				_renderer.Detached();
+				renderer.Detached();
 			}
 		}
 
@@ -107,7 +102,10 @@ public override void Draw(CGRect dirtyRect)
 			coreGraphics.SetStrokeColorSpace(_colorSpace);
 			coreGraphics.SetPatternPhase(PatternPhase);
 
-			_renderer.Draw(coreGraphics, dirtyRect.AsRectangleF());
+			if (Renderer is IGraphicsRenderer renderer)
+			{
+				renderer.Draw(coreGraphics, dirtyRect.AsRectangleF());
+			}
 		}
 
 		public override CGRect Bounds
@@ -120,8 +118,12 @@ public override CGRect Bounds
 				if (_lastBounds.Width != newBounds.Width || _lastBounds.Height != newBounds.Height)
 				{
 					base.Bounds = value;
-					_renderer.SizeChanged((float)newBounds.Width, (float)newBounds.Height);
-					_renderer.Invalidate();
+
+					if (Renderer is IGraphicsRenderer renderer)
+					{
+						renderer.SizeChanged((float)newBounds.Width, (float)newBounds.Height);
+						renderer.Invalidate();
+					}
 
 					_lastBounds = newBounds;
 				}