From 3d91b35e2cb9d429fa0c2e1d80f5b58ea94d4301 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 26 Jul 2023 14:21:58 -0500 Subject: [PATCH] [ios] fix memory leak in SearchBar Context: https://github.com/dotnet/maui/pull/16346 This addresses the memory leak discovered by: src/Core/src/Platform/iOS/MauiSearchBar.cs(36,63): 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/MauiSearchBar.cs(54,32): error MA0001: Event 'OnMovedToWindow' 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/MauiSearchBar.cs(55,32): error MA0001: Event 'EditingChanged' 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/MauiSearchBar.cs(70,31): error MA0003: Subscribing to events with instance method 'OnEditingChanged' could cause memory leaks in an NSObject subclass. Remove the subscription or convert the method to a static method. I could reproduce a leak in a test such as: await InvokeOnMainThreadAsync(() => { var layout = new Grid(); var searchBar = new SearchBar(); layout.Add(searchBar); var handler = CreateHandler(layout); viewReference = new WeakReference(searchBar); handlerReference = new WeakReference(searchBar.Handler); platformViewReference = new WeakReference(searchBar.Handler.PlatformView); }); await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference); Assert.False(viewReference.IsAlive, "SearchBar should not be alive!"); Assert.False(handlerReference.IsAlive, "Handler should not be alive!"); Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!"); Solved the issues by making a non-NSObject `MauiSearchBarProxy` class. I found & fixed a couple related issues: * `SearchBarExtensions.GetSearchTextField()` would have always thrown `StackOverlowException` if iOS was less than 13? It just called itself?!? * Removed `MauiSearchBar.EditingChanged`, as we could subscribe to the event from the `UITextField` directly instead. --- .../tests/DeviceTests/Memory/MemoryTests.cs | 2 + .../SearchBar/SearchBarHandler.iOS.cs | 151 +++++++++++------- src/Core/src/Platform/iOS/MauiSearchBar.cs | 5 +- .../src/Platform/iOS/SearchBarExtensions.cs | 20 ++- 4 files changed, 115 insertions(+), 63 deletions(-) diff --git a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs index b50506f98fab..b929f87bf737 100644 --- a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs +++ b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs @@ -27,6 +27,7 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); }); @@ -46,6 +47,7 @@ void SetupBuilder() [InlineData(typeof(Picker))] [InlineData(typeof(RefreshView))] [InlineData(typeof(ScrollView))] + [InlineData(typeof(SearchBar))] [InlineData(typeof(SwipeView))] [InlineData(typeof(TimePicker))] public async Task HandlerDoesNotLeak(Type type) diff --git a/src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs b/src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs index 6b22b2ed7b20..6ae170ad491d 100644 --- a/src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs +++ b/src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs @@ -8,6 +8,7 @@ namespace Microsoft.Maui.Handlers public partial class SearchBarHandler : ViewHandler { UITextField? _editor; + readonly MauiSearchBarProxy _proxy = new(); public UITextField? QueryEditor => _editor; @@ -23,35 +24,14 @@ protected override MauiSearchBar CreatePlatformView() protected override void ConnectHandler(MauiSearchBar platformView) { - platformView.CancelButtonClicked += OnCancelClicked; - platformView.SearchButtonClicked += OnSearchButtonClicked; - platformView.TextSetOrChanged += OnTextPropertySet; - platformView.OnMovedToWindow += OnMovedToWindow; - platformView.ShouldChangeTextInRange += ShouldChangeText; - platformView.OnEditingStarted += OnEditingStarted; - platformView.EditingChanged += OnEditingChanged; - platformView.OnEditingStopped += OnEditingStopped; + _proxy.Connect(this, VirtualView, platformView); base.ConnectHandler(platformView); } - void OnMovedToWindow(object? sender, EventArgs e) - { - // The cancel button doesn't exist until the control has moved to the window - // so we fire this off again so it can set the color - UpdateValue(nameof(ISearchBar.CancelButtonColor)); - } - protected override void DisconnectHandler(MauiSearchBar platformView) { - platformView.CancelButtonClicked -= OnCancelClicked; - platformView.SearchButtonClicked -= OnSearchButtonClicked; - platformView.TextSetOrChanged -= OnTextPropertySet; - platformView.ShouldChangeTextInRange -= ShouldChangeText; - platformView.OnMovedToWindow -= OnMovedToWindow; - platformView.OnEditingStarted -= OnEditingStarted; - platformView.EditingChanged -= OnEditingChanged; - platformView.OnEditingStopped -= OnEditingStopped; + _proxy.Disconnect(platformView); base.DisconnectHandler(platformView); } @@ -166,56 +146,107 @@ public static void MapKeyboard(ISearchBarHandler handler, ISearchBar searchBar) handler.PlatformView?.UpdateKeyboard(searchBar); } - void OnCancelClicked(object? sender, EventArgs args) + void UpdateCancelButtonVisibility() { - if (VirtualView != null) - VirtualView.Text = string.Empty; + if (PlatformView.ShowsCancelButton != VirtualView.ShouldShowCancelButton()) + UpdateValue(nameof(ISearchBar.CancelButtonColor)); } - void OnSearchButtonClicked(object? sender, EventArgs e) + class MauiSearchBarProxy { - VirtualView?.SearchButtonPressed(); - } + WeakReference? _handler; + WeakReference? _virtualView; - void OnTextPropertySet(object? sender, UISearchBarTextChangedEventArgs a) - { - VirtualView.UpdateText(a.SearchText); + ISearchBar? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null; - UpdateCancelButtonVisibility(); - } + public void Connect(SearchBarHandler handler, ISearchBar virtualView, MauiSearchBar platformView) + { + _handler = new(handler); + _virtualView = new(virtualView); + + platformView.CancelButtonClicked += OnCancelClicked; + platformView.SearchButtonClicked += OnSearchButtonClicked; + platformView.TextSetOrChanged += OnTextPropertySet; + platformView.OnMovedToWindow += OnMovedToWindow; + platformView.ShouldChangeTextInRange += ShouldChangeText; + platformView.OnEditingStarted += OnEditingStarted; + platformView.EditingChanged += OnEditingChanged; + platformView.OnEditingStopped += OnEditingStopped; + } - bool ShouldChangeText(UISearchBar searchBar, NSRange range, string text) - { - var newLength = searchBar?.Text?.Length + text.Length - range.Length; - return newLength <= VirtualView?.MaxLength; - } + public void Disconnect(MauiSearchBar platformView) + { + _virtualView = null; + + platformView.CancelButtonClicked -= OnCancelClicked; + platformView.SearchButtonClicked -= OnSearchButtonClicked; + platformView.TextSetOrChanged -= OnTextPropertySet; + platformView.ShouldChangeTextInRange -= ShouldChangeText; + platformView.OnMovedToWindow -= OnMovedToWindow; + platformView.OnEditingStarted -= OnEditingStarted; + platformView.EditingChanged -= OnEditingChanged; + platformView.OnEditingStopped -= OnEditingStopped; + } - void OnEditingStarted(object? sender, EventArgs e) - { - if (VirtualView is not null) - VirtualView.IsFocused = true; - } + void OnMovedToWindow(object? sender, EventArgs e) + { + // The cancel button doesn't exist until the control has moved to the window + // so we fire this off again so it can set the color + if (_handler is not null && _handler.TryGetTarget(out var handler)) + { + handler.UpdateValue(nameof(ISearchBar.CancelButtonColor)); + } + } - void OnEditingChanged(object? sender, EventArgs e) - { - if (VirtualView == null || _editor == null) - return; + void OnCancelClicked(object? sender, EventArgs args) + { + if (VirtualView is ISearchBar virtualView) + virtualView.Text = string.Empty; + } - VirtualView.UpdateText(_editor.Text); + void OnSearchButtonClicked(object? sender, EventArgs e) + { + VirtualView?.SearchButtonPressed(); + } - UpdateCancelButtonVisibility(); - } + void OnTextPropertySet(object? sender, UISearchBarTextChangedEventArgs a) + { + if (VirtualView is ISearchBar virtualView) + { + virtualView.UpdateText(a.SearchText); + + if (_handler is not null && _handler.TryGetTarget(out var handler)) + { + handler.UpdateCancelButtonVisibility(); + } + } + } - void OnEditingStopped(object? sender, EventArgs e) - { - if (VirtualView is not null) - VirtualView.IsFocused = false; - } + bool ShouldChangeText(UISearchBar searchBar, NSRange range, string text) + { + var newLength = searchBar?.Text?.Length + text.Length - range.Length; + return newLength <= VirtualView?.MaxLength; + } - void UpdateCancelButtonVisibility() - { - if (PlatformView.ShowsCancelButton != VirtualView.ShouldShowCancelButton()) - UpdateValue(nameof(ISearchBar.CancelButtonColor)); + void OnEditingStarted(object? sender, EventArgs e) + { + if (VirtualView is ISearchBar virtualView) + virtualView.IsFocused = true; + } + + void OnEditingChanged(object? sender, EventArgs e) + { + if (_handler is not null && _handler.TryGetTarget(out var handler)) + { + handler.UpdateCancelButtonVisibility(); + } + } + + void OnEditingStopped(object? sender, EventArgs e) + { + if (VirtualView is ISearchBar virtualView) + virtualView.IsFocused = false; + } } } } diff --git a/src/Core/src/Platform/iOS/MauiSearchBar.cs b/src/Core/src/Platform/iOS/MauiSearchBar.cs index 8909d4207d50..62125a40b904 100644 --- a/src/Core/src/Platform/iOS/MauiSearchBar.cs +++ b/src/Core/src/Platform/iOS/MauiSearchBar.cs @@ -1,5 +1,5 @@ using System; -using System.ComponentModel.Design; +using System.Diagnostics.CodeAnalysis; using System.Drawing; using CoreGraphics; using Foundation; @@ -33,6 +33,7 @@ protected internal MauiSearchBar(NativeHandle handle) : base(handle) // 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: MemoryTests.HandlerDoesNotLeak")] public event EventHandler? TextSetOrChanged; public override string? Text @@ -51,7 +52,9 @@ public override string? Text } } + [UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")] internal event EventHandler? OnMovedToWindow; + [UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")] internal event EventHandler? EditingChanged; public override void WillMoveToWindow(UIWindow? window) diff --git a/src/Core/src/Platform/iOS/SearchBarExtensions.cs b/src/Core/src/Platform/iOS/SearchBarExtensions.cs index ccd13d9758a5..ddb0793e626a 100644 --- a/src/Core/src/Platform/iOS/SearchBarExtensions.cs +++ b/src/Core/src/Platform/iOS/SearchBarExtensions.cs @@ -10,9 +10,25 @@ public static class SearchBarExtensions internal static UITextField? GetSearchTextField(this UISearchBar searchBar) { if (OperatingSystem.IsIOSVersionAtLeast(13)) + { return searchBar.SearchTextField; - else - return searchBar.GetSearchTextField(); + } + + // Search Subviews up to two levels deep + // https://stackoverflow.com/a/58056700 + foreach (var child in searchBar.Subviews) + { + if (child is UITextField childTextField) + return childTextField; + + foreach (var grandChild in child.Subviews) + { + if (grandChild is UITextField grandChildTextField) + return grandChildTextField; + } + } + + return null; } internal static void UpdateBackground(this UISearchBar uiSearchBar, ISearchBar searchBar)