[controls] fix memory leak in `BindableLayout` #13550

jonathanpeppers · 2023-02-24T22:42:50Z

Context: #12130
Context: https://github.com/angelru/CvSlowJittering

In reviewing the above sample, I found the following problem:

Setup a BindableLayout on a long-lived BindableCollection<T>
Navigate away, or otherwise remove the BindableLayout from the screen.
A BindableLayoutController object lives forever -- or as long as the BindableCollection<T>.

Note that nothing ever removes/clears BindableLayout.SetItemsSource().

This gets really bad if you have a BindableLayout inside a CollectionView. In the sample above, it has a <DataTemplate> using BindableLayout inside. On Windows, I saw it creating infinite BindableLayoutController objects while scrolling.

I could reproduce this issue in a unit test. I applied the same tricks from 0372df7 to solve the problem. #12130 is not fully solved as it still seems slow to me and still has a memory issue?

Still thinking if there is a "comprehensive" way to solve all of these types of issues across the repo...

Context: dotnet#12130 Context: https://github.com/angelru/CvSlowJittering In reviewing the above sample, I found the following problem: * Setup a `BindableLayout` on a long-lived `BindableCollection<T>` * Navigate away, or otherwise remove the `BindableLayout` from the screen. * A `BindableLayoutController` object lives forever -- or as long as the `BindableCollection<T>`. Note that nothing ever removes/clears `BindableLayout.SetItemsSource()`. This gets really bad if you have a `BindableLayout` inside a `CollectionView`. In the sample above, it has a `<DataTemplate>` using `BindableLayout` inside. On Windows, I saw it creating infinite `BindableLayoutController` objects while scrolling. I could reproduce this issue in a unit test. I applied the same tricks from 0372df7 to solve the problem. dotnet#12130 is not fully solved as it still seems slow to me and still has a memory issue? Still thinking if there is a "comprehensive" way to solve all of these types of issues across the repo...

hartez · 2023-03-16T15:26:34Z

We'll re-evaluate this one for backporting next week once we know where things end up with some upcoming WeakReference changes.

As seen in dotnet#13973, some of my recent changes had a flaw: * dotnet#13550 * dotnet#13806 * dotnet#13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass.

jonathanpeppers · 2023-03-16T20:37:02Z

Given the issue we found, I would say don't backport this yet: #13997

I'll remember to put backport/suggested on the ones I'm more confident in.

As seen in #13973, some of my recent changes had a flaw: * #13550 * #13806 * #13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass. * Fix tests in Release mode In `Release` mode, a `GC.KeepAlive()` call is needed for the tests to pass. Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>

* Removed BuildTizenDefaultTemplate and just have it call RadioButton's default template since they were identical. (#13996) * Reinstate WebView cookie functionality for Android & iOS (#13736) * Fix iOS cookies * Fix Android Cookies * Update src/Core/src/Platform/iOS/MauiWKWebView.cs Co-authored-by: Manuel de la Pena <mandel@microsoft.com> * Auto-format source code * Update MauiWKWebView.cs * Update src/Core/src/Platform/iOS/MauiWKWebView.cs --------- Co-authored-by: Manuel de la Pena <mandel@microsoft.com> Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * Revert 10759. Fix Button sizing using HorizontalOptions. (#14005) * Ensure that Grid is treating star rows/columns as Auto when unconstrained (#13999) * Ensure that Grid is treating star rows/columns as Auto when unconstrained Fixes #13993 * Auto-format source code --------- Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [iOS] Implement ScrollView Orientation (#13657) * [iOS] Remove not used mapper for ContentSize * [iOS] Implement Orientation mapping * [Samples] Add sample page for ScrollView orientation * Try without this * [iOS] Move from extension to helper * Add back removed API * Use SetNeedsLayout to call measure of ContentView * Cleanup * [Android] Fix Frame Renderer to use Wrapper View correctly (#12218) * [Android] Fix Frame to call missing mapper methods * - fix rebase * Auto-format source code * - update tests and wrapper view code * - remove code that's now generalized in ViewHandler * - cleanup frame renderer --------- Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [controls] fix cases a GC causes events to not fire (#13997) As seen in #13973, some of my recent changes had a flaw: * #13550 * #13806 * #13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass. * Fix tests in Release mode In `Release` mode, a `GC.KeepAlive()` call is needed for the tests to pass. Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [iOS] Scroll with the keyboard to not block entries and editors (#13499) --------- Co-authored-by: dustin-wojciechowski <dustin.wojciechowski@microsoft.com> Co-authored-by: Gerald Versluis <gerald.versluis@microsoft.com> Co-authored-by: Manuel de la Pena <mandel@microsoft.com> Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com> Co-authored-by: E.Z. Hart <hartez@users.noreply.github.com> Co-authored-by: Rui Marinho <me@ruimarinho.net> Co-authored-by: Shane Neuville <shneuvil@microsoft.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com> Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>

kojini · 2023-05-08T05:51:53Z

I don't see whether this fix will be backported to .NET 7.
Will it be backported to .NET 7? Do you have ETA on it?
(FYI, I'll be posting the same questions regarding some of memory leak issues/PRs as I'm tracking down when the bug fixes might be released)

jonathanpeppers marked this pull request as ready for review February 27, 2023 20:58

jonathanpeppers added the legacy-area-perf Startup / Runtime performance label Feb 27, 2023

jonathanpeppers requested review from hartez and PureWeen February 27, 2023 20:59

hartez approved these changes Feb 27, 2023

View reviewed changes

jonathanpeppers merged commit 0560202 into dotnet:main Feb 28, 2023

jonathanpeppers deleted the BindableLayoutLeak branch February 28, 2023 00:15

jonathanpeppers mentioned this pull request Mar 6, 2023

Every MAUI Page generates a Memory Leak on iOS and macCatalyst #13520

Closed

mattleibow added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Mar 9, 2023

hartez added backport/NO This change should not be backported. It may break customers. and removed backport/NO This change should not be backported. It may break customers. labels Mar 16, 2023

jonathanpeppers mentioned this pull request Mar 16, 2023

[controls] fix cases a GC causes events to not fire #13997

Merged

jonathanpeppers mentioned this pull request Mar 22, 2023

Possible Memory leaks? #12048

Closed

jonathanpeppers mentioned this pull request Mar 30, 2023

Performance degradation when navigating to a page and going back several times #12930

Closed

jonathanpeppers mentioned this pull request Apr 18, 2023

ListView/CollectionView memory leak under Windows #9162

Closed

hartez added the backport/NO This change should not be backported. It may break customers. label May 31, 2023

jonathanpeppers added memory-leak 💦 Memory usage grows / objects live forever and removed legacy-area-perf Startup / Runtime performance labels Jul 12, 2023

jonathanpeppers mentioned this pull request Sep 11, 2023

FlexLayout Memory leak #10904

Closed

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controls] fix memory leak in `BindableLayout` #13550

[controls] fix memory leak in `BindableLayout` #13550

jonathanpeppers commented Feb 24, 2023

hartez commented Mar 16, 2023

jonathanpeppers commented Mar 16, 2023

kojini commented May 8, 2023

[controls] fix memory leak in BindableLayout #13550

[controls] fix memory leak in BindableLayout #13550

Conversation

jonathanpeppers commented Feb 24, 2023

hartez commented Mar 16, 2023

jonathanpeppers commented Mar 16, 2023

kojini commented May 8, 2023

[controls] fix memory leak in `BindableLayout` #13550

[controls] fix memory leak in `BindableLayout` #13550