Skip to content

Commit

Permalink
[ios] fix memory leak in Picker (#16685)
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/MauiPicker.cs(21,24): error MA0002: Member 'UIPickerView' 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 `MemoryTests.cs`:

    ++[InlineData(typeof(Picker))]
    public async Task HandlerDoesNotLeak(Type type)

Solved the problem by:

* Introduce `MauiPickerProxy` for all event subscriptions. Same pattern
  as in other PRs.

* The `ShouldBeginEditing` event was originally subscribed in
  `CreatePlatformView()` and never unsubscribed. I moved to be
  subscribed/unsubscribed the same way as the other events.

* Refactored the `CreateAlert()` method to be `DisplayAlert()` instead.
  This allows the handler to do all the work -- and the
  `MauiPickerProxy` type can call a method on `PickerHandler` once.
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Aug 15, 2023
1 parent 73271cc commit 844b019
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 66 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 @@ -22,6 +22,7 @@ void SetupBuilder()
handlers.AddHandler<Editor, EditorHandler>();
handlers.AddHandler<GraphicsView, GraphicsViewHandler>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler<Picker, PickerHandler>();
handlers.AddHandler<IContentView, ContentViewHandler>();
handlers.AddHandler<Image, ImageHandler>();
handlers.AddHandler<RefreshView, RefreshViewHandler>();
Expand All @@ -42,6 +43,7 @@ void SetupBuilder()
[InlineData(typeof(GraphicsView))]
[InlineData(typeof(Image))]
[InlineData(typeof(Label))]
[InlineData(typeof(Picker))]
[InlineData(typeof(RefreshView))]
[InlineData(typeof(ScrollView))]
[InlineData(typeof(SwipeView))]
Expand Down
170 changes: 104 additions & 66 deletions src/Core/src/Handlers/Picker/PickerHandler.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,20 @@ namespace Microsoft.Maui.Handlers
{
public partial class PickerHandler : ViewHandler<IPicker, MauiPicker>
{
readonly MauiPickerProxy _proxy = new();
UIPickerView? _pickerView;

#if !MACCATALYST
protected override MauiPicker CreatePlatformView()
{
_pickerView = new UIPickerView();

var platformPicker = new MauiPicker(_pickerView) { BorderStyle = UITextBorderStyle.RoundedRect };
platformPicker.InputView = _pickerView;
platformPicker.InputAccessoryView = new MauiDoneAccessoryView(() =>
var platformPicker = new MauiPicker(_pickerView)
{
FinishSelectItem(_pickerView, platformPicker);
});
BorderStyle = UITextBorderStyle.RoundedRect,
InputView = _pickerView,
InputAccessoryView = new MauiDoneAccessoryView(_proxy.OnDone),
};

platformPicker.InputView.AutoresizingMask = UIViewAutoresizing.FlexibleHeight;
platformPicker.InputAccessoryView.AutoresizingMask = UIViewAutoresizing.FlexibleHeight;
Expand All @@ -31,26 +32,13 @@ protected override MauiPicker CreatePlatformView()

return platformPicker;
}
#else
protected override MauiPicker CreatePlatformView()
{
var platformPicker = new MauiPicker(null) { BorderStyle = UITextBorderStyle.RoundedRect };

platformPicker.ShouldBeginEditing += (textField) =>
{
var alertController = CreateAlert(textField);
var platformWindow = MauiContext?.GetPlatformWindow();
platformWindow?.BeginInvokeOnMainThread(() =>
{
_ = platformWindow?.RootViewController?.PresentViewControllerAsync(alertController, true);
});
return false;
};

return platformPicker;
}
void OnDone() => FinishSelectItem(_pickerView, PlatformView);
#else
protected override MauiPicker CreatePlatformView() =>
new MauiPicker(null) { BorderStyle = UITextBorderStyle.RoundedRect };

UIAlertController CreateAlert(UITextField uITextField)
void DisplayAlert(UITextField uITextField)
{
var paddingTitle = 0;
if (!string.IsNullOrEmpty(VirtualView.Title))
Expand Down Expand Up @@ -85,26 +73,26 @@ UIAlertController CreateAlert(UITextField uITextField)
popoverPresentation.SourceRect = uITextField.Bounds;
}

return pickerController;
var platformWindow = MauiContext?.GetPlatformWindow();
platformWindow?.BeginInvokeOnMainThread(() =>
{
_ = platformWindow?.RootViewController?.PresentViewControllerAsync(pickerController, true);
});
}
#endif

internal bool UpdateImmediately { get; set; }

protected override void ConnectHandler(MauiPicker platformView)
{
platformView.EditingDidBegin += OnStarted;
platformView.EditingDidEnd += OnEnded;
platformView.EditingChanged += OnEditing;
_proxy.Connect(this, VirtualView, platformView);

base.ConnectHandler(platformView);
}

protected override void DisconnectHandler(MauiPicker platformView)
{
platformView.EditingDidBegin -= OnStarted;
platformView.EditingDidEnd -= OnEnded;
platformView.EditingChanged -= OnEditing;
_proxy.Disconnect(platformView);

if (_pickerView != null)
{
Expand Down Expand Up @@ -172,42 +160,6 @@ public static void MapVerticalTextAlignment(IPickerHandler handler, IPicker pick
handler.PlatformView?.UpdateVerticalTextAlignment(picker);
}

void OnStarted(object? sender, EventArgs eventArgs)
{
if (VirtualView != null)
VirtualView.IsFocused = true;
}

void OnEnded(object? sender, EventArgs eventArgs)
{
if (_pickerView == null)
return;

PickerSource? model = (PickerSource)_pickerView.Model;

if (model.SelectedIndex != -1 && model.SelectedIndex != _pickerView.SelectedRowInComponent(0))
{
_pickerView.Select(model.SelectedIndex, 0, false);
}

if (VirtualView != null)
VirtualView.IsFocused = false;
}

void OnEditing(object? sender, EventArgs eventArgs)
{
if (VirtualView == null || PlatformView == null)
return;

// Reset the TextField's Text so it appears as if typing with a keyboard does not work.
var selectedIndex = VirtualView.SelectedIndex;

PlatformView.Text = VirtualView.GetItem(selectedIndex);

// Also clears the undo stack (undo/redo possible on iPads)
PlatformView.UndoManager?.RemoveAllActions();
}

void UpdatePickerFromPickerSource(PickerSource? pickerSource)
{
if (VirtualView == null || PlatformView == null || pickerSource == null)
Expand Down Expand Up @@ -237,6 +189,92 @@ void FinishSelectItem(UIPickerView? pickerView, UITextField textField)
UpdatePickerFromPickerSource(pickerSource);
textField.ResignFirstResponder();
}

class MauiPickerProxy
{
WeakReference<PickerHandler>? _handler;
WeakReference<IPicker>? _virtualView;

IPicker? VirtualView => _virtualView is not null && _virtualView.TryGetTarget(out var v) ? v : null;

PickerHandler? Handler => _handler is not null && _handler.TryGetTarget(out var h) ? h : null;

public void Connect(PickerHandler handler, IPicker virtualView, MauiPicker platformView)
{
_handler = new(handler);
_virtualView = new(virtualView);

#if MACCATALYST
platformView.ShouldBeginEditing += OnShouldBeginEditing;
#endif
platformView.EditingDidBegin += OnStarted;
platformView.EditingDidEnd += OnEnded;
platformView.EditingChanged += OnEditing;
}

public void Disconnect(MauiPicker platformView)
{
#if MACCATALYST
platformView.ShouldBeginEditing -= OnShouldBeginEditing;
#endif
platformView.EditingDidBegin -= OnStarted;
platformView.EditingDidEnd -= OnEnded;
platformView.EditingChanged -= OnEditing;
}

#if MACCATALYST
bool OnShouldBeginEditing (UITextField textField)
{
if (Handler is not PickerHandler handler)
return false;

handler.DisplayAlert(textField);
return false;
}
#else
public void OnDone()
{
if (Handler is PickerHandler handler)
{
handler.OnDone();
}
}
#endif

void OnStarted(object? sender, EventArgs eventArgs)
{
if (VirtualView is IPicker virtualView)
virtualView.IsFocused = true;
}

void OnEnded(object? sender, EventArgs eventArgs)
{
if (Handler is not PickerHandler handler || handler._pickerView is not UIPickerView pickerView)
return;

PickerSource? model = (PickerSource)pickerView.Model;
if (model.SelectedIndex != -1 && model.SelectedIndex != pickerView.SelectedRowInComponent(0))
{
pickerView.Select(model.SelectedIndex, 0, false);
}
if (VirtualView is IPicker virtualView)
virtualView.IsFocused = false;
}

void OnEditing(object? sender, EventArgs eventArgs)
{
if (sender is not MauiPicker platformView || VirtualView is not IPicker virtualView)
return;

// Reset the TextField's Text so it appears as if typing with a keyboard does not work.
var selectedIndex = virtualView.SelectedIndex;

platformView.Text = virtualView.GetItem(selectedIndex);

// Also clears the undo stack (undo/redo possible on iPads)
platformView.UndoManager?.RemoveAllActions();
}
}
}

public class PickerSource : UIPickerViewModel
Expand Down
2 changes: 2 additions & 0 deletions src/Core/src/Platform/iOS/MauiPicker.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using Foundation;
using ObjCRuntime;
using UIKit;
Expand All @@ -18,6 +19,7 @@ public MauiPicker(UIPickerView? uIPickerView)
_enableActions = new HashSet<string>(actions);
}

[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
public UIPickerView? UIPickerView { get; set; }

public override bool CanPerform(Selector action, NSObject? withSender)
Expand Down

0 comments on commit 844b019

Please sign in to comment.