[ios/catalyst] fix memory leaks in *Cell#22067
Conversation
|
Needs a rebase to get the updated snapshot for Issue18242Test and fix the build. |
86e9d49 to
08d9594
Compare
|
@jsuarezruiz do you know what's wrong with the one test lane? I wonder if it is caused by the changes here. |
|
/rebase |
a9ff7f9 to
0e57c49
Compare
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)
One test was crashing (even on rerun), so maybe this will fix
PureWeen
left a comment
There was a problem hiding this comment.
I'm going to try to rework this without needing the InternalPropertyChanged
| 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.")] |
There was a problem hiding this comment.
This method was just a really complicated way of wiring up to _onForceUpdateSizeRequested and making sure to unwire from it.
We can just achieve this same thing through the commandmapper now
| else | ||
| { | ||
| tvc.PropertyChanged -= HandlePropertyChanged; | ||
| CellPropertyChanged -= HandlePropertyChanged; |
There was a problem hiding this comment.
Because this event now is just local to this class this isn't going to cause a circular reference. I opted to keep this event path because this maintains the timing of when HandlePropertyChanged will start and stop firing
I reworked most of this PR so my review isn't relevant anymore
Fixes: #20195
Context: https://github.com/AdamEssenmacher/MemoryToolkit.Maui/tree/main/samples
Retesting this sample with the
ListViewleak fixed, I noticed the sample reports 0 leaks! Unfortunately, it still displayed memory growing over time...Taking a
.gcdumpsnapshot, I noticed a cycle:ViewCellRenderer.ViewTableCell->ViewCell _viewCell->ViewCellRenderer->ViewTableCellI was able to write a test that reproduces the leak, and I extended it for every type of
*CelllikeTextCell,ImageCell,SwitchCell, etc.The fixes are:
ViewTableCellnow uses aWeakReference<ViewCell> _viewCellCellTableViewCellnow uses aWeakReference<Cell> _cellCellTableViewCellnow uses aWeakEventManagerforPropertyChanged, as the*Renderersubscribes to this event.Note that I changed
PropertyChangedto an event, which is an API change. (I can revisit this if needed)