[ios] fix memory leak in Editor (#16348)

Context: #16346 This addresses the memory leak discovered by: src/Core/src/Platform/iOS/MauiTextView.cs(37,30): error MA0001: Event 'TextSetOrChanged' could cause memory leaks in an NSObject subclass. Remove the event or add the [UnconditionalSuppressMessage("Memory", "MA0001")] attribute with a justification as to why the event will not leak. src/Core/src/Platform/iOS/MauiTextView.cs(12,20): error MA0002: Member '_placeholderLabel' could cause memory leaks in an NSObject subclass. Remove the member, store the value as a WeakReference, or add the [UnconditionalSuppressMessage("Memory", "MA0002")] attribute with a justification as to why the member will not leak. I could reproduce a leak in a test such as: await InvokeOnMainThreadAsync(() => { var layout = new Grid(); var editor = new Editor(); layout.Add(editor); var handler = CreateHandler<LayoutHandler>(layout); viewReference = new WeakReference(editor); handlerReference = new WeakReference(editor.Handler); platformViewReference = new WeakReference(editor.Handler.PlatformView); }); await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference); Assert.False(viewReference.IsAlive, "Editor should not be alive!"); Assert.False(handlerReference.IsAlive, "Handler should not be alive!"); Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!"); I will create a similar PR for `Entry` as well.
dotnet · Jul 26, 2023 · 13cfac6 · 13cfac6
1 parent 0fb5704
commit 13cfac6
Show file tree

Hide file tree

Showing 3 changed files with 113 additions and 41 deletions.
diff --git a/src/Controls/tests/DeviceTests/Elements/Editor/EditorTests.cs b/src/Controls/tests/DeviceTests/Elements/Editor/EditorTests.cs
@@ -10,6 +10,43 @@ namespace Microsoft.Maui.DeviceTests
 	[Category(TestCategory.Editor)]
 	public partial class EditorTests : ControlsHandlerTestBase
 	{
+		void SetupBuilder()
+		{
+			EnsureHandlerCreated(builder =>
+			{
+				builder.ConfigureMauiHandlers(handlers =>
+				{
+					handlers.AddHandler<Editor, EditorHandler>();
+				});
+			});
+		}
+
+		[Fact(DisplayName = "Does Not Leak")]
+		public async Task DoesNotLeak()
+		{
+			SetupBuilder();
+
+			WeakReference viewReference = null;
+			WeakReference platformViewReference = null;
+			WeakReference handlerReference = null;
+
+			await InvokeOnMainThreadAsync(() =>
+			{
+				var layout = new Grid();
+				var editor = new Editor();
+				layout.Add(editor);
+				var handler = CreateHandler<LayoutHandler>(layout);
+				viewReference = new WeakReference(editor);
+				handlerReference = new WeakReference(editor.Handler);
+				platformViewReference = new WeakReference(editor.Handler.PlatformView);
+			});
+
+			await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
+			Assert.False(viewReference.IsAlive, "Editor should not be alive!");
+			Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
+			Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");
+		}
+
 #if !IOS && !MACCATALYST
 		// iOS is broken until this point
 		// https://github.com/dotnet/maui/issues/3425

diff --git a/src/Core/src/Handlers/Editor/EditorHandler.iOS.cs b/src/Core/src/Handlers/Editor/EditorHandler.iOS.cs
@@ -9,7 +9,7 @@ namespace Microsoft.Maui.Handlers
 {
 	public partial class EditorHandler : ViewHandler<IEditor, MauiTextView>
 	{
-		bool _set;
+		readonly MauiTextViewEventProxy _proxy = new();
 
 		protected override MauiTextView CreatePlatformView()
 		{
@@ -40,31 +40,17 @@ public override void SetVirtualView(IView view)
 		{
 			base.SetVirtualView(view);
 
-			if (!_set)
-				PlatformView.SelectionChanged += OnSelectionChanged;
-
-			_set = true;
+			_proxy.SetVirtualView(PlatformView);
 		}
 
 		protected override void ConnectHandler(MauiTextView platformView)
 		{
-			platformView.ShouldChangeText += OnShouldChangeText;
-			platformView.Started += OnStarted;
-			platformView.Ended += OnEnded;
-			platformView.TextSetOrChanged += OnTextPropertySet;
+			_proxy.Connect(VirtualView, platformView);
 		}
 
 		protected override void DisconnectHandler(MauiTextView platformView)
 		{
-			platformView.ShouldChangeText -= OnShouldChangeText;
-			platformView.Started -= OnStarted;
-			platformView.Ended -= OnEnded;
-			platformView.TextSetOrChanged -= OnTextPropertySet;
-
-			if (_set)
-				platformView.SelectionChanged -= OnSelectionChanged;
-
-			_set = false;
+			_proxy.Disconnect(platformView);
 		}
 
 		public override Size GetDesiredSize(double widthConstraint, double heightConstraint)
@@ -152,38 +138,84 @@ public static void MapFormatting(IEditorHandler handler, IEditor editor)
 		public static void MapIsEnabled(IEditorHandler handler, IEditor editor) =>
 			handler.PlatformView?.UpdateIsEnabled(editor);
 
-		bool OnShouldChangeText(UITextView textView, NSRange range, string replacementString) =>
-			VirtualView.TextWithinMaxLength(textView.Text, range, replacementString);
-
-		void OnStarted(object? sender, EventArgs eventArgs)
+		class MauiTextViewEventProxy
 		{
-			if (VirtualView != null)
-				VirtualView.IsFocused = true;
-		}
+			bool _set;
+			WeakReference<IEditor>? _virtualView;
 
-		void OnEnded(object? sender, EventArgs eventArgs)
-		{
-			if (VirtualView != null)
+			IEditor? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null;
+
+			public void Connect(IEditor virtualView, MauiTextView platformView)
 			{
-				VirtualView.IsFocused = false;
+				_virtualView = new(virtualView);
 
-				VirtualView.Completed();
+				platformView.ShouldChangeText += OnShouldChangeText;
+				platformView.Started += OnStarted;
+				platformView.Ended += OnEnded;
+				platformView.TextSetOrChanged += OnTextPropertySet;
 			}
-		}
 
-		void OnTextPropertySet(object? sender, EventArgs e) =>
-			VirtualView.UpdateText(PlatformView.Text);
+			public void Disconnect(MauiTextView platformView)
+			{
+				_virtualView = null;
 
-		private void OnSelectionChanged(object? sender, EventArgs e)
-		{
-			var cursorPosition = PlatformView.GetCursorPosition();
-			var selectedTextLength = PlatformView.GetSelectedTextLength();
+				platformView.ShouldChangeText -= OnShouldChangeText;
+				platformView.Started -= OnStarted;
+				platformView.Ended -= OnEnded;
+				platformView.TextSetOrChanged -= OnTextPropertySet;
+				if (_set)
+					platformView.SelectionChanged -= OnSelectionChanged;
+
+				_set = false;
+			}
+
+			public void SetVirtualView(MauiTextView platformView)
+			{
+				if (!_set)
+					platformView.SelectionChanged += OnSelectionChanged;
+				_set = true;
+			}
+
+			void OnSelectionChanged(object? sender, EventArgs e)
+			{
+				if (sender is MauiTextView platformView && VirtualView is IEditor virtualView)
+				{
+					var cursorPosition = platformView.GetCursorPosition();
+					var selectedTextLength = platformView.GetSelectedTextLength();
+
+					if (virtualView.CursorPosition != cursorPosition)
+						virtualView.CursorPosition = cursorPosition;
+
+					if (virtualView.SelectionLength != selectedTextLength)
+						virtualView.SelectionLength = selectedTextLength;
+				}
+			}
 
-			if (VirtualView.CursorPosition != cursorPosition)
-				VirtualView.CursorPosition = cursorPosition;
+			bool OnShouldChangeText(UITextView textView, NSRange range, string replacementString) =>
+				VirtualView?.TextWithinMaxLength(textView.Text, range, replacementString) ?? false;
 
-			if (VirtualView.SelectionLength != selectedTextLength)
-				VirtualView.SelectionLength = selectedTextLength;
+			void OnStarted(object? sender, EventArgs eventArgs)
+			{
+				if (VirtualView is IEditor virtualView)
+					virtualView.IsFocused = true;
+			}
+
+			void OnEnded(object? sender, EventArgs eventArgs)
+			{
+				if (VirtualView is IEditor virtualView)
+				{
+					virtualView.IsFocused = false;
+					virtualView.Completed();
+				}
+			}
+
+			void OnTextPropertySet(object? sender, EventArgs e)
+			{
+				if (sender is MauiTextView platformView)
+				{
+					VirtualView?.UpdateText(platformView.Text);
+				}
+			}
 		}
 	}
 }
diff --git a/src/Core/src/Platform/iOS/MauiTextView.cs b/src/Core/src/Platform/iOS/MauiTextView.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.InteropServices;
 using CoreGraphics;
 using Foundation;
@@ -9,6 +10,7 @@ namespace Microsoft.Maui.Platform
 {
 	public class MauiTextView : UITextView
 	{
+		[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: EditorTests.DoesNotLeak")]
 		readonly UILabel _placeholderLabel;
 		nfloat? _defaultPlaceholderSize;
 
@@ -34,6 +36,7 @@ public override void WillMoveToWindow(UIWindow? window)
 		// Native Changed doesn't fire when the Text Property is set in code
 		// We use this event as a way to fire changes whenever the Text changes
 		// via code or user interaction.
+		[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: EditorTests.DoesNotLeak")]
 		public event EventHandler? TextSetOrChanged;
 
 		public string? PlaceholderText