From c097135eb82925eafcf78f4794efc7e5ad18757e Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 25 Jul 2023 16:24:29 -0500 Subject: [PATCH] [ios] fix memory leak in Entry Context: https://github.com/dotnet/maui/pull/16346 This addresses the memory leak discovered by: src/Core/src/Platform/iOS/MauiTextField.cs(69,32): error MA0001: Event 'SelectionChanged' 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/MauiTextField.cs(68,30): error MA0001: Event 'TextPropertySet' 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. I could reproduce a leak in a test such as: await InvokeOnMainThreadAsync(() => { var layout = new Grid(); var entry = new Entry(); layout.Add(entry); var handler = CreateHandler(layout); viewReference = new WeakReference(entry); handlerReference = new WeakReference(entry.Handler); platformViewReference = new WeakReference(entry.Handler.PlatformView); }); await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference); Assert.False(viewReference.IsAlive, "Entry should not be alive!"); Assert.False(handlerReference.IsAlive, "Handler should not be alive!"); Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!"); --- .../DeviceTests/Elements/Entry/EntryTests.cs | 37 +++++ .../src/Handlers/Entry/EntryHandler.iOS.cs | 151 +++++++++++------- src/Core/src/Platform/iOS/MauiTextField.cs | 3 + 3 files changed, 136 insertions(+), 55 deletions(-) diff --git a/src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.cs b/src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.cs index 649aeaec1123..775282b3ef32 100644 --- a/src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.cs @@ -12,6 +12,17 @@ namespace Microsoft.Maui.DeviceTests [Category(TestCategory.Entry)] public partial class EntryTests : ControlsHandlerTestBase { + void SetupBuilder() + { + EnsureHandlerCreated(builder => + { + builder.ConfigureMauiHandlers(handlers => + { + handlers.AddHandler(); + }); + }); + } + [Fact] public async Task MaxLengthTrims() { @@ -70,6 +81,32 @@ await InvokeOnMainThreadAsync(() => }); } + [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 entry = new Entry(); + layout.Add(entry); + var handler = CreateHandler(layout); + viewReference = new WeakReference(entry); + handlerReference = new WeakReference(entry.Handler); + platformViewReference = new WeakReference(entry.Handler.PlatformView); + }); + + await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference); + Assert.False(viewReference.IsAlive, "Entry should not be alive!"); + Assert.False(handlerReference.IsAlive, "Handler should not be alive!"); + Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!"); + } + #if WINDOWS // Only Windows needs the IsReadOnly workaround for MaxLength==0 to prevent text from being entered [Fact] diff --git a/src/Core/src/Handlers/Entry/EntryHandler.iOS.cs b/src/Core/src/Handlers/Entry/EntryHandler.iOS.cs index 94a523ee30ea..9be644f1e449 100644 --- a/src/Core/src/Handlers/Entry/EntryHandler.iOS.cs +++ b/src/Core/src/Handlers/Entry/EntryHandler.iOS.cs @@ -9,7 +9,7 @@ namespace Microsoft.Maui.Handlers { public partial class EntryHandler : ViewHandler { - bool _set; + readonly MauiTextFieldProxy _proxy = new(); protected override MauiTextField CreatePlatformView() => new MauiTextField @@ -22,35 +22,17 @@ public override void SetVirtualView(IView view) { base.SetVirtualView(view); - if (!_set) - PlatformView.SelectionChanged += OnSelectionChanged; - - _set = true; + _proxy.SetVirtualView(PlatformView); } protected override void ConnectHandler(MauiTextField platformView) { - platformView.ShouldReturn += OnShouldReturn; - platformView.EditingDidBegin += OnEditingBegan; - platformView.EditingChanged += OnEditingChanged; - platformView.EditingDidEnd += OnEditingEnded; - platformView.TextPropertySet += OnTextPropertySet; - platformView.ShouldChangeCharacters += OnShouldChangeCharacters; + _proxy.Connect(VirtualView, platformView); } protected override void DisconnectHandler(MauiTextField platformView) { - platformView.ShouldReturn -= OnShouldReturn; - platformView.EditingDidBegin -= OnEditingBegan; - platformView.EditingChanged -= OnEditingChanged; - platformView.EditingDidEnd -= OnEditingEnded; - platformView.TextPropertySet -= OnTextPropertySet; - platformView.ShouldChangeCharacters -= OnShouldChangeCharacters; - - if (_set) - platformView.SelectionChanged -= OnSelectionChanged; - - _set = false; + _proxy.Disconnect(platformView); } public static void MapText(IEntryHandler handler, IEntry entry) @@ -124,53 +106,112 @@ public static void MapFormatting(IEntryHandler handler, IEntry entry) handler.PlatformView?.UpdateHorizontalTextAlignment(entry); } - protected virtual bool OnShouldReturn(UITextField view) + protected virtual bool OnShouldReturn(UITextField view) => + _proxy.OnShouldReturn(view); + + class MauiTextFieldProxy { - KeyboardAutoManager.GoToNextResponderOrResign(view); + bool _set; + WeakReference? _virtualView; - VirtualView?.Completed(); + IEntry? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null; - return false; - } + public void Connect(IEntry virtualView, MauiTextField platformView) + { + _virtualView = new(virtualView); - void OnEditingBegan(object? sender, EventArgs e) - { - if (VirtualView == null || PlatformView == null) - return; + platformView.ShouldReturn += OnShouldReturn; + platformView.EditingDidBegin += OnEditingBegan; + platformView.EditingChanged += OnEditingChanged; + platformView.EditingDidEnd += OnEditingEnded; + platformView.TextPropertySet += OnTextPropertySet; + platformView.ShouldChangeCharacters += OnShouldChangeCharacters; + } - PlatformView?.UpdateSelectionLength(VirtualView); + public void Disconnect(MauiTextField platformView) + { + _virtualView = null; - VirtualView.IsFocused = true; - } + platformView.ShouldReturn -= OnShouldReturn; + platformView.EditingDidBegin -= OnEditingBegan; + platformView.EditingChanged -= OnEditingChanged; + platformView.EditingDidEnd -= OnEditingEnded; + platformView.TextPropertySet -= OnTextPropertySet; + platformView.ShouldChangeCharacters -= OnShouldChangeCharacters; - void OnEditingChanged(object? sender, EventArgs e) => - VirtualView.UpdateText(PlatformView.Text); + if (_set) + platformView.SelectionChanged -= OnSelectionChanged; - void OnEditingEnded(object? sender, EventArgs e) - { - if (VirtualView == null || PlatformView == null) - return; + _set = false; + } - VirtualView.UpdateText(PlatformView.Text); - VirtualView.IsFocused = false; - } + public void SetVirtualView(MauiTextField platformView) + { + if (!_set) + platformView.SelectionChanged += OnSelectionChanged; + _set = true; + } - void OnTextPropertySet(object? sender, EventArgs e) => - VirtualView.UpdateText(PlatformView.Text); + public bool OnShouldReturn(UITextField view) + { + KeyboardAutoManager.GoToNextResponderOrResign(view); - bool OnShouldChangeCharacters(UITextField textField, NSRange range, string replacementString) => - VirtualView.TextWithinMaxLength(textField.Text, range, replacementString); + VirtualView?.Completed(); - private void OnSelectionChanged(object? sender, EventArgs e) - { - var cursorPosition = PlatformView.GetCursorPosition(); - var selectedTextLength = PlatformView.GetSelectedTextLength(); + return false; + } + + void OnEditingBegan(object? sender, EventArgs e) + { + if (sender is MauiTextField platformView && VirtualView is IEntry virtualView) + { + platformView.UpdateSelectionLength(virtualView); + virtualView.IsFocused = true; + } + } + + void OnEditingChanged(object? sender, EventArgs e) + { + if (sender is MauiTextField platformView) + { + VirtualView?.UpdateText(platformView.Text); + } + } - if (VirtualView.CursorPosition != cursorPosition) - VirtualView.CursorPosition = cursorPosition; + void OnEditingEnded(object? sender, EventArgs e) + { + if (sender is MauiTextField platformView && VirtualView is IEntry virtualView) + { + virtualView.UpdateText(platformView.Text); + virtualView.IsFocused = false; + } + } + + void OnTextPropertySet(object? sender, EventArgs e) + { + if (sender is MauiTextField platformView) + { + VirtualView?.UpdateText(platformView.Text); + } + } - if (VirtualView.SelectionLength != selectedTextLength) - VirtualView.SelectionLength = selectedTextLength; + bool OnShouldChangeCharacters(UITextField textField, NSRange range, string replacementString) => + VirtualView?.TextWithinMaxLength(textField.Text, range, replacementString) ?? false; + + void OnSelectionChanged(object? sender, EventArgs e) + { + if (sender is MauiTextField platformView && VirtualView is IEntry virtualView) + { + var cursorPosition = platformView.GetCursorPosition(); + var selectedTextLength = platformView.GetSelectedTextLength(); + + if (virtualView.CursorPosition != cursorPosition) + virtualView.CursorPosition = cursorPosition; + + if (virtualView.SelectionLength != selectedTextLength) + virtualView.SelectionLength = selectedTextLength; + } + } } } } \ No newline at end of file diff --git a/src/Core/src/Platform/iOS/MauiTextField.cs b/src/Core/src/Platform/iOS/MauiTextField.cs index 77812e5d8889..1bef17426e31 100644 --- a/src/Core/src/Platform/iOS/MauiTextField.cs +++ b/src/Core/src/Platform/iOS/MauiTextField.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics.CodeAnalysis; using CoreGraphics; using Foundation; using ObjCRuntime; @@ -65,7 +66,9 @@ public override UITextRange? SelectedTextRange } } + [UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: EntryTests.DoesNotLeak")] public event EventHandler? TextPropertySet; + [UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: EntryTests.DoesNotLeak")] internal event EventHandler? SelectionChanged; } } \ No newline at end of file