Skip to content

Commit

Permalink
[ios] fix memory leak in Entry (#16349)
Browse files Browse the repository at this point in the history 
Context: #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<LayoutHandler>(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!");
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Jul 26, 2023
1 parent 13cfac6 commit 98d4bbf
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 55 deletions.
37 changes: 37 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Entry, EntryHandler>();
});
});
}

[Fact]
public async Task MaxLengthTrims()
{
Expand Down Expand Up @@ -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<LayoutHandler>(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]
Expand Down
151 changes: 96 additions & 55 deletions src/Core/src/Handlers/Entry/EntryHandler.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.Maui.Handlers
{
public partial class EntryHandler : ViewHandler<IEntry, MauiTextField>
{
bool _set;
readonly MauiTextFieldProxy _proxy = new();

protected override MauiTextField CreatePlatformView() =>
new MauiTextField
Expand All @@ -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)
Expand Down Expand Up @@ -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<IEntry>? _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;
}
}
}
}
}
3 changes: 3 additions & 0 deletions src/Core/src/Platform/iOS/MauiTextField.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics.CodeAnalysis;
using CoreGraphics;
using Foundation;
using ObjCRuntime;
Expand Down Expand Up @@ -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;
}
}

0 comments on commit 98d4bbf

Please sign in to comment.