Skip to content

Commit

Permalink
[ios] fix memory leak in SearchBar
Browse files Browse the repository at this point in the history 
Context: dotnet#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<LayoutHandler>(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.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Aug 28, 2023
1 parent 95c3fb7 commit 9af950d
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 65 deletions.
2 changes: 2 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ void SetupBuilder()
handlers.AddHandler<Image, ImageHandler>();
handlers.AddHandler<RefreshView, RefreshViewHandler>();
handlers.AddHandler<IScrollView, ScrollViewHandler>();
handlers.AddHandler<SearchBar, SearchBarHandler>();
handlers.AddHandler<SwipeView, SwipeViewHandler>();
handlers.AddHandler<TimePicker, TimePickerHandler>();
});
Expand All @@ -48,6 +49,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)
Expand Down
151 changes: 91 additions & 60 deletions src/Core/src/Handlers/SearchBar/SearchBarHandler.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace Microsoft.Maui.Handlers
public partial class SearchBarHandler : ViewHandler<ISearchBar, MauiSearchBar>
{
UITextField? _editor;
readonly MauiSearchBarProxy _proxy = new();

public UITextField? QueryEditor => _editor;

Expand All @@ -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);
}
Expand Down Expand Up @@ -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<SearchBarHandler>? _handler;
WeakReference<ISearchBar>? _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;
}
}
}
}
6 changes: 3 additions & 3 deletions src/Core/src/Platform/iOS/MauiSearchBar.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +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 = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
public event EventHandler<UISearchBarTextChangedEventArgs>? TextSetOrChanged;

public override string? Text
Expand All @@ -52,9 +52,9 @@ public override string? Text
}
}

[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
internal event EventHandler? OnMovedToWindow;
[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "FIXME: https://github.com/dotnet/maui/pull/16383")]
[UnconditionalSuppressMessage("Memory", "MA0001", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
internal event EventHandler? EditingChanged;

public override void WillMoveToWindow(UIWindow? window)
Expand Down
20 changes: 18 additions & 2 deletions src/Core/src/Platform/iOS/SearchBarExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 9af950d

Please sign in to comment.