Skip to content

Commit

Permalink
[ios] fix memory leak in TableView (#18718)
Browse files Browse the repository at this point in the history 
Context: #18365

I could reproduce a leak in `TableView` by adding a parameterized test:

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

`TableViewModelRenderer` has two cycles:

* `TableView` ->
* `TableViewRenderer` ->
* `TableViewModelRenderer` ->
* `TableView` via `View` field

* `UITableView.Source` ->
* `TableViewModelRenderer` ->
* `UITableView` via `Table` field

I left the `Table` and `View` fields in place to not break any public
APIs. They are now unused and marked `[Obsolete]`. I used
`WeakReference<T>` to solve these two cycles and the test now passes.

The analyzer warned about these locations, but we don't have the analyzer
enabled on `Microsoft.Maui.Controls` yet:

#18318 (comment)
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Nov 17, 2023
1 parent 90cb3b5 commit c10043f
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 23 deletions.
75 changes: 52 additions & 23 deletions src/Controls/src/Core/Compatibility/Handlers/TableView/iOS/TableViewModelRenderer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#nullable disable
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using Foundation;
using ObjCRuntime;
using UIKit;
Expand All @@ -12,35 +13,55 @@ public class TableViewModelRenderer : UITableViewSource
readonly Dictionary<nint, Cell> _headerCells = new Dictionary<nint, Cell>();

protected bool HasBoundGestures;

[Obsolete("Unused due to memory leak. Will be removed in a future version.")]
[UnconditionalSuppressMessage("Memory", "MEM0002", Justification = "Unused")]
protected UITableView Table;

[Obsolete("Unused due to memory leak. Will be removed in a future version.")]
[UnconditionalSuppressMessage("Memory", "MEM0002", Justification = "Unused")]
protected TableView View;

WeakReference<UITableView> _platformView;
WeakReference<TableView> _tableView;

UITableView PlatformView
{
get => _platformView is not null && _platformView.TryGetTarget(out var t) ? t : null;
set => _platformView = value is not null ? new(value) : null;
}

internal TableView TableView
{
get => _tableView is not null && _tableView.TryGetTarget(out var t) ? t : null;
set => _tableView = value is not null ? new(value) : null;
}

public TableViewModelRenderer(TableView model)
{
View = model;
View.ModelChanged += (s, e) =>
{
if (Table != null)
Table.ReloadData();
};
TableView = model;
model.ModelChanged += (s, e) => PlatformView?.ReloadData();
AutomaticallyDeselect = true;
}

public bool AutomaticallyDeselect { get; set; }

public override UITableViewCell GetCell(UITableView tableView, NSIndexPath indexPath)
{
var cell = View.Model.GetCell(indexPath.Section, indexPath.Row);
if (TableView is not TableView table)
return null;
var cell = table.Model.GetCell(indexPath.Section, indexPath.Row);
var nativeCell = CellTableViewCell.GetPlatformCell(tableView, cell);

return nativeCell;
}

public override nfloat GetHeightForHeader(UITableView tableView, nint section)
{
if (TableView is not TableView table)
return 0;
if (!_headerCells.ContainsKey((int)section))
_headerCells[section] = View.Model.GetHeaderCell((int)section);
_headerCells[section] = table.Model.GetHeaderCell((int)section);

var result = _headerCells[section];

Expand All @@ -49,8 +70,10 @@ public override nfloat GetHeightForHeader(UITableView tableView, nint section)

public override UIView GetViewForHeader(UITableView tableView, nint section)
{
if (TableView is not TableView table)
return null;
if (!_headerCells.ContainsKey((int)section))
_headerCells[section] = View.Model.GetHeaderCell((int)section);
_headerCells[section] = table.Model.GetHeaderCell((int)section);

var result = _headerCells[section];

Expand All @@ -62,17 +85,17 @@ public override UIView GetViewForHeader(UITableView tableView, nint section)
result.ReusableCell = reusable;
result.TableView = tableView;

var cellRenderer = result.ToHandler(View.FindMauiContext());
var cellRenderer = result.ToHandler(table.FindMauiContext());
return (UIView)cellRenderer.PlatformView;
}
return null;
}

public override void WillDisplayHeaderView(UITableView tableView, UIView headerView, nint section)
{
if (headerView is UITableViewHeaderFooterView header)
if (headerView is UITableViewHeaderFooterView header && TableView is TableView table)
{
var sectionHeaderTextColor = View.Model.GetSectionTextColor((int)section);
var sectionHeaderTextColor = table.Model.GetSectionTextColor((int)section);
if (sectionHeaderTextColor is not null)
{
#pragma warning disable CA1416, CA1422 // TODO: 'UITableViewHeaderFooterView.TextLabel' is unsupported on: 'ios' 14.0 and later
Expand All @@ -87,40 +110,43 @@ public override void WillDisplayHeaderView(UITableView tableView, UIView headerV

public void LongPress(UILongPressGestureRecognizer gesture)
{
var point = gesture.LocationInView(Table);
var indexPath = Table.IndexPathForRowAtPoint(point);
if (PlatformView is not UITableView tableView)
return;

var point = gesture.LocationInView(tableView);
var indexPath = tableView.IndexPathForRowAtPoint(point);
if (indexPath == null)
return;

View.Model.RowLongPressed(indexPath.Section, indexPath.Row);
TableView?.Model.RowLongPressed(indexPath.Section, indexPath.Row);
}

public override nint NumberOfSections(UITableView tableView)
{
BindGestures(tableView);
return View.Model.GetSectionCount();
return TableView?.Model.GetSectionCount() ?? 0;
}

public override void RowSelected(UITableView tableView, NSIndexPath indexPath)
{
View.Model.RowSelected(indexPath.Section, indexPath.Row);
TableView?.Model.RowSelected(indexPath.Section, indexPath.Row);
if (AutomaticallyDeselect)
tableView.DeselectRow(indexPath, true);
}

public override nint RowsInSection(UITableView tableview, nint section)
{
return View.Model.GetRowCount((int)section);
return TableView?.Model.GetRowCount((int)section) ?? 0;
}

public override string[] SectionIndexTitles(UITableView tableView)
{
return View.Model.GetSectionIndexTitles();
return TableView?.Model.GetSectionIndexTitles();
}

public override string TitleForHeader(UITableView tableView, nint section)
{
return View.Model.GetSectionTitle((int)section);
return TableView?.Model.GetSectionTitle((int)section);
}

void BindGestures(UITableView tableview)
Expand All @@ -138,7 +164,7 @@ void BindGestures(UITableView tableview)
dismissGesture.CancelsTouchesInView = false;
tableview.AddGestureRecognizer(dismissGesture);

Table = tableview;
PlatformView = tableview;
}

void Tap(UITapGestureRecognizer gesture)
Expand All @@ -155,10 +181,13 @@ public UnEvenTableViewModelRenderer(TableView model) : base(model)

public override nfloat GetHeightForRow(UITableView tableView, NSIndexPath indexPath)
{
var cell = View.Model.GetCell(indexPath.Section, indexPath.Row);
if (TableView is not TableView table)
return 0;

var cell = table.Model.GetCell(indexPath.Section, indexPath.Row);
var h = cell.Height;

if (View.RowHeight == -1 && h == -1 && cell is ViewCell)
if (table.RowHeight == -1 && h == -1 && cell is ViewCell)
{
return UITableView.AutomaticDimension;
}
Expand Down
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 @@ -46,6 +46,7 @@ void SetupBuilder()
handlers.AddHandler<Stepper, StepperHandler>();
handlers.AddHandler<SwipeView, SwipeViewHandler>();
handlers.AddHandler<Switch, SwitchHandler>();
handlers.AddHandler<TableView, TableViewRenderer>();
handlers.AddHandler<TimePicker, TimePickerHandler>();
handlers.AddHandler<WebView, WebViewHandler>();
});
Expand Down Expand Up @@ -78,6 +79,7 @@ void SetupBuilder()
[InlineData(typeof(SwipeView))]
[InlineData(typeof(Switch))]
[InlineData(typeof(TimePicker))]
[InlineData(typeof(TableView))]
[InlineData(typeof(WebView))]
public async Task HandlerDoesNotLeak(Type type)
{
Expand Down

0 comments on commit c10043f

Please sign in to comment.