[ios] fix memory leak in Image (#15062)

* [ios] fix memory leak in Image Context: #14664 (comment) `Image` has two different "cycles" on iOS: * `ImageHandler` -> `MauiImageView` -> `ImageHandler` * via the `WindowChanged` event * `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler` * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>` This causes any `MauiImageView` to live forever. To solve these issues: * Get rid of the `MauiImageView.WindowChanged` event, and use a `WeakReference` to the handler instead. * `ImageSourcePartLoader` now only has a `WeakReference` to the handler. * This requires a new `ISetImageHandler` interface to be used by several handler types involving images. Hopefully the changes here are using `[Obsolete]` correctly to make things backwards compatible & less breaking changes. Unsure yet if this fully solves #14664, but at least one part of it. * Update src/Core/src/Platform/ImageSourcePartLoader.cs Co-authored-by: Matthew Leibowitz <mattleibow@live.com> * Update MauiImageView.cs * Update MauiImageView.cs * ISetImageHandler -> IImageSourcePartSetter * IImageHandler.OnWindowChanged() is now public * Fix IImageSourcePartSetter namespace * MauiImageView._handler can be readonly --------- Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
dotnet · Jun 19, 2023 · 3348cdd · 3348cdd
1 parent 0d04197
commit 3348cdd
Show file tree

Hide file tree

Showing 38 changed files with 160 additions and 65 deletions.
diff --git a/src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs b/src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs
@@ -47,5 +47,45 @@ await InvokeOnMainThreadAsync(async () =>
 				await handler.ToPlatform().AssertContainsColor(Colors.Red, MauiContext);
 			});
 		}
+
+		[Fact("Image Does Not Leak")]
+		public async Task DoesNotLeak()
+		{
+			SetupBuilder();
+			WeakReference platformViewReference = null;
+			WeakReference handlerReference = null;
+
+			await InvokeOnMainThreadAsync(async () =>
+			{
+				var layout = new VerticalStackLayout();
+				var image = new Image
+				{
+					Background = Colors.Black,
+					Source = "red.png",
+				};
+				layout.Add(image);
+
+				var handler = CreateHandler<LayoutHandler>(layout);
+				handlerReference = new WeakReference(image.Handler);
+				platformViewReference = new WeakReference(image.Handler.PlatformView);
+				await image.Wait();
+			});
+
+			Assert.NotNull(handlerReference);
+			Assert.NotNull(platformViewReference);
+
+			// Several GCs required on iOS
+			for (int i = 0; i < 5; i++)
+			{
+				if (!handlerReference.IsAlive && !platformViewReference.IsAlive)
+					break;
+				await Task.Yield();
+				GC.Collect();
+				GC.WaitForPendingFinalizers();
+			}
+
+			Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
+			Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");
+		}
 	}
 }
diff --git a/src/Core/src/Handlers/Button/ButtonHandler.Android.cs b/src/Core/src/Handlers/Button/ButtonHandler.Android.cs
@@ -121,7 +121,7 @@ public static Task MapImageSourceAsync(IButtonHandler handler, IImage image)
 			return handler.ImageSourceLoader.UpdateImageSourceAsync();
 		}
 
-		void OnSetImageSource(Drawable? obj)
+		void IImageSourcePartSetter.SetImageSource(Drawable? obj)
 		{
 			PlatformView.Icon = obj;
 		}

diff --git a/src/Core/src/Handlers/Button/ButtonHandler.Standard.cs b/src/Core/src/Handlers/Button/ButtonHandler.Standard.cs
@@ -16,6 +16,6 @@ public static void MapFont(IButtonHandler handler, ITextStyle button) { }
 		public static void MapPadding(IButtonHandler handler, IButton button) { }
 		public static void MapImageSource(IButtonHandler handler, IImage image) { }
 
-		void OnSetImageSource(object? obj) { }
+		void IImageSourcePartSetter.SetImageSource(object? obj) { }
 	}
 }
diff --git a/src/Core/src/Handlers/Button/ButtonHandler.Tizen.cs b/src/Core/src/Handlers/Button/ButtonHandler.Tizen.cs
@@ -110,7 +110,7 @@ void OnButtonPressed(object? sender, EventArgs e)
 			VirtualView?.Pressed();
 		}
 
-		void OnSetImageSource(MauiImageSource? image)
+		void IImageSourcePartSetter.SetImageSource(MauiImageSource? image)
 		{
 			if (image == null)
 				return;

diff --git a/src/Core/src/Handlers/Button/ButtonHandler.Windows.cs b/src/Core/src/Handlers/Button/ButtonHandler.Windows.cs
@@ -90,7 +90,7 @@ public static void MapImageSource(IButtonHandler handler, IImage image) =>
 				.UpdateImageSourceAsync()
 				.FireAndForget(handler);
 
-		void OnSetImageSource(ImageSource? platformImageSource)
+		void IImageSourcePartSetter.SetImageSource(ImageSource? platformImageSource)
 		{
 			PlatformView.UpdateImageSource(platformImageSource);
 		}

diff --git a/src/Core/src/Handlers/Button/ButtonHandler.cs b/src/Core/src/Handlers/Button/ButtonHandler.cs
@@ -12,11 +12,11 @@
 
 namespace Microsoft.Maui.Handlers
 {
-	public partial class ButtonHandler : IButtonHandler
+	public partial class ButtonHandler : IButtonHandler, IImageSourcePartSetter
 	{
 		ImageSourcePartLoader? _imageSourcePartLoader;
 		public ImageSourcePartLoader ImageSourceLoader =>
-			_imageSourcePartLoader ??= new ImageSourcePartLoader(this, () => (VirtualView as IImageButton), OnSetImageSource);
+			_imageSourcePartLoader ??= new ImageSourcePartLoader(this);
 
 		public static IPropertyMapper<IImage, IButtonHandler> ImageButtonMapper = new PropertyMapper<IImage, IButtonHandler>()
 		{

diff --git a/src/Core/src/Handlers/Button/ButtonHandler.iOS.cs b/src/Core/src/Handlers/Button/ButtonHandler.iOS.cs
@@ -129,7 +129,7 @@ public static void MapFormatting(IButtonHandler handler, IText button)
 			handler.PlatformView?.UpdateCharacterSpacing(button);
 		}
 
-		void OnSetImageSource(UIImage? image)
+		void IImageSourcePartSetter.SetImageSource(UIImage? image)
 		{
 			if (image != null)
 			{

diff --git a/src/Core/src/Handlers/Image/IImageHandler.cs b/src/Core/src/Handlers/Image/IImageHandler.cs
@@ -17,5 +17,9 @@ public partial interface IImageHandler : IViewHandler
 		new IImage VirtualView { get; }
 		ImageSourcePartLoader SourceLoader { get; }
 		new PlatformView PlatformView { get; }
+
+#if IOS || MACCATALYST
+		void OnWindowChanged() { }
+#endif
 	}
 }
diff --git a/src/Core/src/Handlers/Image/IImageSourcePartSetter.cs b/src/Core/src/Handlers/Image/IImageSourcePartSetter.cs
@@ -0,0 +1,20 @@
+#if __IOS__ || MACCATALYST
+using PlatformImage = UIKit.UIImage;
+#elif MONOANDROID
+using PlatformImage = Android.Graphics.Drawables.Drawable;
+#elif WINDOWS
+using PlatformImage = Microsoft.UI.Xaml.Media.ImageSource;
+#elif TIZEN
+using PlatformImage = Microsoft.Maui.Platform.MauiImageSource;
+#elif (NETSTANDARD || !PLATFORM) || (NET6_0_OR_GREATER && !IOS && !ANDROID && !TIZEN)
+using PlatformImage = System.Object;
+#endif
+
+namespace Microsoft.Maui.Handlers
+{
+	public interface IImageSourcePartSetter : IElementHandler
+	{
+		void SetImageSource(PlatformImage? obj);
+	}
+}
+
diff --git a/src/Core/src/Handlers/Image/ImageHandler.Android.cs b/src/Core/src/Handlers/Image/ImageHandler.Android.cs
@@ -50,7 +50,7 @@ public static void MapSource(IImageHandler handler, IImage image) =>
 		public static Task MapSourceAsync(IImageHandler handler, IImage image) =>
 			handler.SourceLoader.UpdateImageSourceAsync();
 
-		void OnSetImageSource(Drawable? obj) =>
+		void IImageSourcePartSetter.SetImageSource(Drawable? obj) =>
 			PlatformView.SetImageDrawable(obj);
 
 		public override void PlatformArrange(Graphics.Rect frame)

diff --git a/src/Core/src/Handlers/Image/ImageHandler.Standard.cs b/src/Core/src/Handlers/Image/ImageHandler.Standard.cs
@@ -8,6 +8,6 @@ public partial class ImageHandler : ViewHandler<IImage, object>
 		public static void MapAspect(IImageHandler handler, IImage image) { }
 		public static void MapIsAnimationPlaying(IImageHandler handler, IImage image) { }
 		public static void MapSource(IImageHandler handler, IImage image) { }
-		void OnSetImageSource(object? obj) => throw new NotImplementedException();
+		void IImageSourcePartSetter.SetImageSource(object? obj) => throw new NotImplementedException();
 	}
 }
diff --git a/src/Core/src/Handlers/Image/ImageHandler.Tizen.cs b/src/Core/src/Handlers/Image/ImageHandler.Tizen.cs
@@ -37,7 +37,7 @@ public static void MapSource(IImageHandler handler, IImage image) =>
 		public static Task MapSourceAsync(IImageHandler handler, IImage image) =>
 			handler.SourceLoader.UpdateImageSourceAsync();
 
-		void OnSetImageSource(MauiImageSource? obj)
+		void IImageSourcePartSetter.SetImageSource(MauiImageSource? obj)
 		{
 			if (obj == null)
 				return;

diff --git a/src/Core/src/Handlers/Image/ImageHandler.Windows.cs b/src/Core/src/Handlers/Image/ImageHandler.Windows.cs
@@ -38,7 +38,7 @@ public static void MapSource(IImageHandler handler, IImage image) =>
 		public static Task MapSourceAsync(IImageHandler handler, IImage image) =>
 			handler.SourceLoader.UpdateImageSourceAsync();
 
-		void OnSetImageSource(ImageSource? obj) =>
+		void IImageSourcePartSetter.SetImageSource(ImageSource? obj) =>
 			PlatformView.Source = obj;
 	}
 }
diff --git a/src/Core/src/Handlers/Image/ImageHandler.cs b/src/Core/src/Handlers/Image/ImageHandler.cs
@@ -14,7 +14,7 @@
 
 namespace Microsoft.Maui.Handlers
 {
-	public partial class ImageHandler : IImageHandler
+	public partial class ImageHandler : IImageHandler, IImageSourcePartSetter
 	{
 		public static IPropertyMapper<IImage, IImageHandler> Mapper = new PropertyMapper<IImage, IImageHandler>(ViewHandler.ViewMapper)
 		{
@@ -32,7 +32,7 @@ public partial class ImageHandler : IImageHandler
 
 		ImageSourcePartLoader? _imageSourcePartLoader;
 		public ImageSourcePartLoader SourceLoader =>
-			_imageSourcePartLoader ??= new ImageSourcePartLoader(this, () => VirtualView, OnSetImageSource);
+			_imageSourcePartLoader ??= new ImageSourcePartLoader(this);
 
 		public ImageHandler() : base(Mapper)
 		{

diff --git a/src/Core/src/Handlers/Image/ImageHandler.iOS.cs b/src/Core/src/Handlers/Image/ImageHandler.iOS.cs
@@ -9,23 +9,12 @@ namespace Microsoft.Maui.Handlers
 {
 	public partial class ImageHandler : ViewHandler<IImage, UIImageView>
 	{
-		protected override UIImageView CreatePlatformView() => new MauiImageView();
-
-		protected override void ConnectHandler(UIImageView platformView)
-		{
-			base.ConnectHandler(platformView);
-
-			if (PlatformView is MauiImageView imageView)
-				imageView.WindowChanged += OnWindowChanged;
-		}
+		protected override UIImageView CreatePlatformView() => new MauiImageView(this);
 
 		protected override void DisconnectHandler(UIImageView platformView)
 		{
 			base.DisconnectHandler(platformView);
 
-			if (platformView is MauiImageView imageView)
-				imageView.WindowChanged -= OnWindowChanged;
-
 			SourceLoader.Reset();
 		}
 
@@ -52,10 +41,10 @@ public static void MapSource(IImageHandler handler, IImage image) =>
 		public static Task MapSourceAsync(IImageHandler handler, IImage image) =>
 			handler.SourceLoader.UpdateImageSourceAsync();
 
-		void OnSetImageSource(UIImage? obj) =>
+		void IImageSourcePartSetter.SetImageSource(UIImage? obj) =>
 			PlatformView.Image = obj;
 
-		void OnWindowChanged(object? sender, EventArgs e)
+		public void OnWindowChanged()
 		{
 			if (SourceLoader.SourceManager.IsResolutionDependent)
 				UpdateValue(nameof(IImage.Source));

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Android.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Android.cs
@@ -18,7 +18,7 @@ protected override ShapeableImageView CreatePlatformView()
 			return platformView;
 		}
 
-		void OnSetImageSource(Drawable? obj)
+		void IImageSourcePartSetter.SetImageSource(Drawable? obj)
 		{
 			PlatformView.SetImageDrawable(obj);
 		}

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Standard.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Standard.cs
@@ -11,7 +11,7 @@ public static void MapStrokeThickness(IImageButtonHandler handler, IButtonStroke
 		public static void MapCornerRadius(IImageButtonHandler handler, IButtonStroke buttonStroke) { }
 		public static void MapPadding(IImageButtonHandler handler, IImageButton imageButton) { }
 
-		void OnSetImageSource(object? obj)
+		void IImageSourcePartSetter.SetImageSource(object? obj)
 		{
 			throw new NotImplementedException();
 		}

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Tizen.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Tizen.cs
@@ -66,7 +66,7 @@ void OnClicked(object? sender, EventArgs e)
 			VirtualView?.Clicked();
 		}
 
-		void OnSetImageSource(MauiImageSource? img)
+		void IImageSourcePartSetter.SetImageSource(MauiImageSource? img)
 		{
 			if (img == null)
 				return;

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Windows.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.Windows.cs
@@ -90,7 +90,7 @@ public static void MapPadding(IImageButtonHandler handler, IImageButton imageBut
 			(handler.PlatformView as Button)?.UpdatePadding(imageButton);
 		}
 
-		void OnSetImageSource(ImageSource? nativeImageSource)
+		void IImageSourcePartSetter.SetImageSource(ImageSource? nativeImageSource)
 		{
 			PlatformView.UpdateImageSource(nativeImageSource);
 		}

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.cs
@@ -23,7 +23,7 @@
 
 namespace Microsoft.Maui.Handlers
 {
-	public partial class ImageButtonHandler : IImageButtonHandler
+	public partial class ImageButtonHandler : IImageButtonHandler, IImageSourcePartSetter
 	{
 		public static IPropertyMapper<IImage, IImageHandler> ImageMapper = new PropertyMapper<IImage, IImageHandler>(ImageHandler.Mapper);
 
@@ -44,7 +44,7 @@ public partial class ImageButtonHandler : IImageButtonHandler
 
 		ImageSourcePartLoader? _imageSourcePartLoader;
 		public ImageSourcePartLoader SourceLoader =>
-			_imageSourcePartLoader ??= new ImageSourcePartLoader(this, () => VirtualView, OnSetImageSource);
+			_imageSourcePartLoader ??= new ImageSourcePartLoader(this);
 
 		public ImageButtonHandler() : base(Mapper)
 		{

diff --git a/src/Core/src/Handlers/ImageButton/ImageButtonHandler.iOS.cs b/src/Core/src/Handlers/ImageButton/ImageButtonHandler.iOS.cs
@@ -15,7 +15,7 @@ protected override UIButton CreatePlatformView()
 			return platformView;
 		}
 
-		void OnSetImageSource(UIImage? obj)
+		void IImageSourcePartSetter.SetImageSource(UIImage? obj)
 		{
 			PlatformView.SetImage(obj?.ImageWithRenderingMode(UIImageRenderingMode.AlwaysOriginal), UIControlState.Normal);
 			PlatformView.HorizontalAlignment = UIControlContentHorizontalAlignment.Fill;

diff --git a/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Android.cs b/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Android.cs
@@ -133,7 +133,7 @@ void UpdateSize()
 				var icons = textView.GetCompoundDrawables();
 				if (icons.Length > 1 && icons[1] != null)
 				{
-					OnSetImageSource(icons[1]);
+					((IImageSourcePartSetter)this).SetImageSource(icons[1]);
 				}
 			}
 
@@ -143,7 +143,7 @@ void UpdateSize()
 			PlatformView.SetPadding(0, buttonPadding, 0, buttonPadding);
 		}
 
-		void OnSetImageSource(Drawable? drawable)
+		void IImageSourcePartSetter.SetImageSource(Drawable? drawable)
 		{
 			if (drawable != null)
 			{

diff --git a/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Standard.cs b/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Standard.cs
@@ -23,7 +23,7 @@ public static void MapBackground(ISwipeItemMenuItemHandler handler, ISwipeItemMe
 
 		public static void MapVisibility(ISwipeItemMenuItemHandler handler, ISwipeItemMenuItem view) { }
 
-		void OnSetImageSource(object? obj)
+		void IImageSourcePartSetter.SetImageSource(object? obj)
 		{
 			throw new NotImplementedException();
 		}

diff --git a/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Tizen.cs b/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.Tizen.cs
@@ -58,7 +58,7 @@ public static void MapVisibility(ISwipeItemMenuItemHandler handler, ISwipeItemMe
 
 		}
 
-		void OnSetImageSource(MauiImageSource? obj)
+		void IImageSourcePartSetter.SetImageSource(MauiImageSource? obj)
 		{
 			if (obj != null)
 				PlatformView.Icon.ResourceUrl = obj.ResourceUrl;

diff --git a/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.cs b/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.cs
@@ -15,6 +15,9 @@
 namespace Microsoft.Maui.Handlers
 {
 	public partial class SwipeItemMenuItemHandler : ISwipeItemMenuItemHandler
+#if !WINDOWS
+		, IImageSourcePartSetter
+#endif
 	{
 		public static IPropertyMapper<ISwipeItemMenuItem, ISwipeItemMenuItemHandler> Mapper =
 			new PropertyMapper<ISwipeItemMenuItem, ISwipeItemMenuItemHandler>(ViewHandler.ElementMapper)
@@ -56,7 +59,7 @@ protected SwipeItemMenuItemHandler(IPropertyMapper? mapper, CommandMapper? comma
 #if !WINDOWS
 		ImageSourcePartLoader? _imageSourcePartLoader;
 		public ImageSourcePartLoader SourceLoader =>
-			_imageSourcePartLoader ??= new ImageSourcePartLoader(this, () => VirtualView, OnSetImageSource);
+			_imageSourcePartLoader ??= new ImageSourcePartLoader(this);
 
 
 		public static void MapSource(ISwipeItemMenuItemHandler handler, ISwipeItemMenuItem image) =>

diff --git a/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.iOS.cs b/src/Core/src/Handlers/SwipeItemMenuItem/SwipeItemMenuItemHandler.iOS.cs
@@ -97,7 +97,7 @@ void OnSwipeItemFrameChanged(object? sender, EventArgs e)
 				swipeItemMenuItemHandler.UpdateValue(nameof(ISwipeItemMenuItem.Source));
 		}
 
-		void OnSetImageSource(UIImage? image)
+		void IImageSourcePartSetter.SetImageSource(UIImage? image)
 		{
 			if (PlatformView == null || PlatformView.Frame == CGRect.Empty)
 				return;