Skip to content

Commit

Permalink
[ios/catalyst] fix memory leaks in *Cell (#22067)
Browse files Browse the repository at this point in the history 
* [ios/catalyst] fix memory leaks in `*Cell`

Context: https://github.com/AdamEssenmacher/MemoryToolkit.Maui/tree/main/samples

Retesting this sample with the `ListView` leak fixed, I noticed the
sample reports 0 leaks! Unfortunately, it still displayed memory growing
over time...

Taking a `.gcdump` snapshot, I noticed a cycle:

* `ViewCellRenderer.ViewTableCell` -> `ViewCell _viewCell` -> `ViewCellRenderer` -> `ViewTableCell`

I was able to write a test that reproduces the leak, and I extended it
for every type of `*Cell` like `TextCell`, `ImageCell`, `SwitchCell`, etc.

The fixes are:

* `ViewTableCell` now uses a `WeakReference<ViewCell> _viewCell`

* `CellTableViewCell` now uses a `WeakReference<Cell> _cell`

* `CellTableViewCell` now uses a `WeakEventManager` for `PropertyChanged`,
  as the `*Renderer` subscribes to this event.

Note that I changed `PropertyChanged` to an event, which is an API
change. (I can revisit this if needed)

* Fixed all public API changes

* Better null checking

One test was crashing (even on rerun), so maybe this will fix

* - add null check for ImageCellRenderer

* - wire everything up through handler paths

* - cleanup

* Update CellTableViewCell.cs

* - add additional unshipped

---------

Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
  • Loading branch information
@jonathanpeppers @PureWeen
jonathanpeppers and PureWeen committed May 17, 2024
1 parent f2cc45e commit e7f8467
Show file tree
Hide file tree
Showing 11 changed files with 145 additions and 103 deletions.
1 change: 1 addition & 0 deletions src/Controls/src/Core/Cells/Cell.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ async void OnForceUpdateSizeRequested()
// don't run more than once per 16 milliseconds
await Task.Delay(TimeSpan.FromMilliseconds(16));
ForceUpdateSizeRequested?.Invoke(this, null);
Handler.Invoke("ForceUpdateSizeRequested", null);

_nextCallToForceUpdateSizeQueued = false;
}
Expand Down
76 changes: 35 additions & 41 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/CellRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ public class CellRenderer : ElementHandler<Cell, UITableViewCell>, IRegisterable
{
static readonly BindableProperty RealCellProperty = BindableProperty.CreateAttached("RealCell", typeof(UITableViewCell), typeof(Cell), null);

EventHandler? _onForceUpdateSizeRequested;
PropertyChangedEventHandler? _onPropertyChangedEventHandler;
readonly UIColor _defaultCellBgColor = (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsTvOSVersionAtLeast(13)) ? UIColor.Clear : UIColor.White;

public static PropertyMapper<Cell, CellRenderer> Mapper =
Expand All @@ -22,6 +20,8 @@ public class CellRenderer : ElementHandler<Cell, UITableViewCell>, IRegisterable
new CommandMapper<Cell, CellRenderer>(ElementHandler.ElementCommandMapper);
UITableView? _tableView;

private protected event PropertyChangedEventHandler? CellPropertyChanged;

public CellRenderer() : base(Mapper, CommandMapper)
{
}
Expand All @@ -44,8 +44,6 @@ public virtual UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

tvc.Cell = item;

WireUpForceUpdateSizeRequested(item, tvc, tv);

if (OperatingSystem.IsIOSVersionAtLeast(14))
{
var content = tvc.DefaultContentConfiguration;
Expand Down Expand Up @@ -135,44 +133,9 @@ protected void UpdateBackground(UITableViewCell tableViewCell, Cell cell)
SetBackgroundColor(tableViewCell, cell, uiBgColor);
}

[Obsolete("The ForceUpdateSizeRequested event is now managed by the command mapper, so it's not necessary to invoke this event manually.")]
protected void WireUpForceUpdateSizeRequested(ICellController cell, UITableViewCell platformCell, UITableView tableView)
{
var inpc = cell as INotifyPropertyChanged;
cell.ForceUpdateSizeRequested -= _onForceUpdateSizeRequested;

if (inpc != null)
inpc.PropertyChanged -= _onPropertyChangedEventHandler;

_onForceUpdateSizeRequested = (sender, e) =>
{
var index = tableView?.IndexPathForCell(platformCell);
if (index == null && sender is Cell c)
{
index = Controls.Compatibility.Platform.iOS.CellExtensions.GetIndexPath(c);
}
if (index != null)
tableView?.ReloadRows(new[] { index }, UITableViewRowAnimation.None);
};

_onPropertyChangedEventHandler = (sender, e) =>
{
if (e.PropertyName == "RealCell" && sender is BindableObject bo && GetRealCell(bo) == null)
{
if (sender is ICellController icc)
icc.ForceUpdateSizeRequested -= _onForceUpdateSizeRequested;
if (sender is INotifyPropertyChanged notifyPropertyChanged)
notifyPropertyChanged.PropertyChanged -= _onPropertyChangedEventHandler;
_onForceUpdateSizeRequested = null;
_onPropertyChangedEventHandler = null;
}
};

cell.ForceUpdateSizeRequested += _onForceUpdateSizeRequested;
if (inpc != null)
inpc.PropertyChanged += _onPropertyChangedEventHandler;

}

Expand All @@ -190,5 +153,36 @@ internal static void SetRealCell(BindableObject cell, UITableViewCell renderer)
{
cell.SetValue(RealCellProperty, renderer);
}

public override void UpdateValue(string property)
{
base.UpdateValue(property);
var args = new PropertyChangedEventArgs(property);
if (VirtualView is BindableObject bindableObject &&
GetRealCell(bindableObject) is CellTableViewCell ctv )
{
ctv.HandlePropertyChanged(bindableObject, args);
}

CellPropertyChanged?.Invoke(VirtualView, args);
}

public override void Invoke(string command, object? args)
{
base.Invoke(command, args);

if (command == "ForceUpdateSizeRequested" &&
VirtualView is BindableObject bindableObject &&
GetRealCell(bindableObject) is UITableViewCell ctv)
{
var index = _tableView?.IndexPathForCell(ctv);
if (index == null && VirtualView is Cell c)
{
index = Controls.Compatibility.Platform.iOS.CellExtensions.GetIndexPath(c);
}
if (index != null)
_tableView?.ReloadRows(new[] { index }, UITableViewRowAnimation.None);
}
}
}
}
}
38 changes: 20 additions & 18 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/CellTableViewCell.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.Maui.Controls.Handlers.Compatibility
{
public class CellTableViewCell : UITableViewCell, INativeElementView
{
Cell _cell;
WeakReference<Cell> _cell;

public Action<object, PropertyChangedEventArgs> PropertyChanged;

Expand All @@ -21,23 +21,28 @@ public CellTableViewCell(UITableViewCellStyle style, string key) : base(style, k

public Cell Cell
{
get { return _cell; }
get => _cell?.GetTargetOrDefault();
set
{
if (_cell == value)
return;

if (_cell != null)
if (_cell is not null)
{
_cell.PropertyChanged -= HandlePropertyChanged;
BeginInvokeOnMainThread(_cell.SendDisappearing);
if (_cell.TryGetTarget(out var cell) && cell == value)
return;

if (cell is not null)
{
BeginInvokeOnMainThread(cell.SendDisappearing);
}
}
_cell = value;

if (_cell != null)
if (value is not null)
{
_cell = new(value);
BeginInvokeOnMainThread(value.SendAppearing);
}
else
{
_cell.PropertyChanged += HandlePropertyChanged;
BeginInvokeOnMainThread(_cell.SendAppearing);
_cell = null;
}
}
}
Expand Down Expand Up @@ -109,12 +114,9 @@ protected override void Dispose(bool disposing)

if (disposing)
{
PropertyChanged = null;

if (_cell != null)
if (Cell is Cell cell)
{
_cell.PropertyChanged -= HandlePropertyChanged;
CellRenderer.SetRealCell(_cell, null);
CellRenderer.SetRealCell(cell, null);
}
_cell = null;
}
Expand All @@ -124,4 +126,4 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}
}
}
}
8 changes: 4 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/EntryCellRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,23 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

var tvc = reusableCell as EntryCellTableViewCell;
if (tvc == null)
{
tvc = new EntryCellTableViewCell(item.GetType().FullName);
}
else
{
tvc.PropertyChanged -= HandlePropertyChanged;
CellPropertyChanged -= HandlePropertyChanged;
tvc.TextFieldTextChanged -= OnTextFieldTextChanged;
tvc.KeyboardDoneButtonPressed -= OnKeyBoardDoneButtonPressed;
}

SetRealCell(item, tvc);

tvc.Cell = item;
tvc.PropertyChanged += HandlePropertyChanged;
CellPropertyChanged += HandlePropertyChanged;
tvc.TextFieldTextChanged += OnTextFieldTextChanged;
tvc.KeyboardDoneButtonPressed += OnKeyBoardDoneButtonPressed;

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateBackground(tvc, entryCell);
UpdateLabel(tvc, entryCell);
UpdateText(tvc, entryCell);
Expand Down
8 changes: 3 additions & 5 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/ImageCellRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,

var imageCell = (ImageCell)item;

WireUpForceUpdateSizeRequested(item, result, tv);

SetImage(imageCell, result);

return result;
Expand All @@ -47,12 +45,12 @@ void SetImage(ImageCell cell, CellTableViewCell target)

source.LoadImage(cell.FindMauiContext(), (result) =>
{
var uiimage = result.Value;
if (uiimage != null)
var uiimage = result?.Value;
if (uiimage is not null)
{
NSRunLoop.Main.BeginInvokeOnMainThread(() =>
{
if (target.Cell != null)
if (target.Cell is not null)
{
target.ImageView.Image = uiimage;
target.SetNeedsLayout();
Expand Down
6 changes: 2 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/SwitchCellRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
else
{
uiSwitch = tvc.AccessoryView as UISwitch;
tvc.PropertyChanged -= HandlePropertyChanged;
CellPropertyChanged -= HandlePropertyChanged;
}

SetRealCell(item, tvc);
Expand All @@ -45,16 +45,14 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
_defaultOnColor = UISwitch.Appearance.OnTintColor;

tvc.Cell = item;
tvc.PropertyChanged += HandlePropertyChanged;
CellPropertyChanged += HandlePropertyChanged;
tvc.AccessoryView = uiSwitch;
#pragma warning disable CA1416, CA1422 // TODO: 'UITableViewCell.TextLabel' is unsupported on: 'ios' 14.0 and later
tvc.TextLabel.Text = boolCell.Text;
#pragma warning restore CA1416, CA1422

uiSwitch.On = boolCell.On;

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateBackground(tvc, item);
UpdateIsEnabled(tvc, boolCell);
UpdateFlowDirection(tvc, boolCell);
Expand Down
6 changes: 2 additions & 4 deletions src/Controls/src/Core/Compatibility/Handlers/ListView/iOS/TextCellRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,19 @@ public override UITableViewCell GetCell(Cell item, UITableViewCell reusableCell,
if (!(reusableCell is CellTableViewCell tvc))
tvc = new CellTableViewCell(UITableViewCellStyle.Subtitle, item.GetType().FullName);
else
tvc.PropertyChanged -= HandleCellPropertyChanged;
CellPropertyChanged -= HandleCellPropertyChanged;

SetRealCell(item, tvc);

tvc.Cell = textCell;
tvc.PropertyChanged = HandleCellPropertyChanged;
CellPropertyChanged += HandleCellPropertyChanged;

#pragma warning disable CA1416, CA1422 // TODO: 'UITableViewCell.TextLabel', DetailTextLabel is unsupported on: 'ios' 14.0 and later
tvc.TextLabel.Text = textCell.Text;
tvc.DetailTextLabel.Text = textCell.Detail;
tvc.TextLabel.TextColor = (textCell.TextColor ?? DefaultTextColor).ToPlatform();
tvc.DetailTextLabel.TextColor = (textCell.DetailColor ?? DefaultDetailColor).ToPlatform();

WireUpForceUpdateSizeRequested(item, tvc, tv);

UpdateIsEnabled(tvc, textCell);
#pragma warning restore CA1416, CA1422

Expand Down

0 comments on commit e7f8467

Please sign in to comment.