Skip to content

Commit

Permalink
[ios] fix memory leaks in MauiDoneAccessoryView (#16380)
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/MauiDoneAccessoryView.cs(11,19): error MA0002: Member '_doneClicked' 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.

After writing a test for it, I found a pattern the analyze doesn't
currently catch:

    public MauiDoneAccessoryView(Action doneClicked)
    {
        //...
        var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, OnClicked);
        SetItems(new[] { spacer, doneButton }, false);
    }

This creates a cycle:

* `MauiDoneAccessoryView` -> `UIBarButtonItem` via list of items

* `UIBarButtonItem` -> `Action` -> `MauiDoneAccessoryView` via `OnClicked`

`MauiDoneAccessoryView` lives forever, as well as the owners of any
`Action` delegate values passed in.

I could resolve these problems by creating a new non-NSObject
`BarButtonItemProxy` type to handle events.

This solves one leak with the following controls that use
`MauiDoneAccessoryView`:

* `Editor`
* `Picker`
* `DatePicker`
* `TimePicker`

However, I'll need to send future PRs to verify all these controls are
100% leak free.
  • Loading branch information
jonathanpeppers authored Jul 26, 2023
1 parent e642faf commit a8cda9d
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 17 deletions.
50 changes: 33 additions & 17 deletions src/Core/src/Platform/iOS/MauiDoneAccessoryView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,44 +7,60 @@ namespace Microsoft.Maui.Platform
{
internal class MauiDoneAccessoryView : UIToolbar
{
WeakReference<object>? _data;
Action<object>? _doneClicked;
readonly BarButtonItemProxy _proxy;

public MauiDoneAccessoryView() : base(new CGRect(0, 0, UIScreen.MainScreen.Bounds.Width, 44))
{
_proxy = new BarButtonItemProxy();
BarStyle = UIBarStyle.Default;
Translucent = true;
var spacer = new UIBarButtonItem(UIBarButtonSystemItem.FlexibleSpace);
var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, OnClicked);
var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, _proxy.OnDataClicked);

SetItems(new[] { spacer, doneButton }, false);
}

void OnClicked(object? sender, EventArgs e)
{
if (_data?.TryGetTarget(out object? target) == true)
_doneClicked?.Invoke(target);
}
internal void SetDoneClicked(Action<object>? value) => _proxy.SetDoneClicked(value);

internal void SetDoneClicked(Action<object>? value) => _doneClicked = value;
internal void SetDataContext(object? dataContext)
{
_data = null;
if (dataContext == null)
return;

_data = new WeakReference<object>(dataContext);
}
internal void SetDataContext(object? dataContext) => _proxy.SetDataContext(dataContext);

public MauiDoneAccessoryView(Action doneClicked) : base(new CGRect(0, 0, UIScreen.MainScreen.Bounds.Width, 44))
{
_proxy = new BarButtonItemProxy(doneClicked);
BarStyle = UIBarStyle.Default;
Translucent = true;

var spacer = new UIBarButtonItem(UIBarButtonSystemItem.FlexibleSpace);
var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, (o, a) => doneClicked?.Invoke());
var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, _proxy.OnClicked);
SetItems(new[] { spacer, doneButton }, false);
}

class BarButtonItemProxy
{
readonly Action? _doneClicked;
Action<object>? _doneWithDataClicked;
WeakReference<object>? _data;

public BarButtonItemProxy() { }

public BarButtonItemProxy(Action doneClicked)
{
_doneClicked = doneClicked;
}

public void SetDoneClicked(Action<object>? value) => _doneWithDataClicked = value;

public void SetDataContext(object? dataContext) => _data = dataContext is null ? null : new(dataContext);

public void OnDataClicked(object? sender, EventArgs e)
{
if (_data is not null && _data.TryGetTarget(out var data))
_doneWithDataClicked?.Invoke(data);
}

public void OnClicked(object? sender, EventArgs e) => _doneClicked?.Invoke();
}
}
}
#endif
53 changes: 53 additions & 0 deletions src/Core/tests/DeviceTests/Memory/UIViewSubclassTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
using System;
using System.Threading.Tasks;
using Xunit;

#if IOS || MACCATALYST

namespace Microsoft.Maui.DeviceTests.Memory
{
// Set of tests to verify UIView subclasses do not leak
[Category(TestCategory.Memory)]
public class UIViewSubclassTests : TestBase
{
#if IOS // MauiDoneAccessoryView is iOS only
void DoAction() { }
void DoAction(object data) { }

[Fact]
public async Task MauiDoneAccessoryView_Ctor()
{
WeakReference reference = null;
Action action = DoAction;

await InvokeOnMainThreadAsync(() =>
{
var accessory = new MauiDoneAccessoryView(action);
reference = new(accessory);
});

await AssertionExtensions.WaitForGC(reference);
Assert.False(reference.IsAlive, "MauiDoneAccessoryView should not be alive!");
}

[Fact]
public async Task MauiDoneAccessoryView_SetDoneClicked()
{
WeakReference reference = null;
Action<object> action = DoAction;

await InvokeOnMainThreadAsync(() =>
{
var accessory = new MauiDoneAccessoryView();
reference = new(accessory);
accessory.SetDoneClicked(action);
});

await AssertionExtensions.WaitForGC(reference);
Assert.False(reference.IsAlive, "MauiDoneAccessoryView should not be alive!");
}
#endif
}
}

#endif
1 change: 1 addition & 0 deletions src/Core/tests/DeviceTests/TestCategory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public static class TestCategory
public const string IndicatorView = "IndicatorView";
public const string Label = "Label";
public const string Layout = "Layout";
public const string Memory = "Memory";
public const string NavigationView = "NavigationView";
public const string Page = "Page";
public const string Picker = "Picker";
Expand Down

0 comments on commit a8cda9d

Please sign in to comment.