Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[core] fix memory leaks in bindings (dotnet#13327)
Fixes: dotnet#12039 Fixes: dotnet#10560 Context: https://github.com/Vroomer/MAUI-master-detail-memory-leak Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/memory-leak In investigating various MAUI samples, I found the following object leaking: WeakPropertyChangedProxy (referenced by ->) TypedBinding Binding In ~2016, I contributed a fix to Xamarin.Forms: xamarin/Xamarin.Forms@f6febd4 Back then, this solved the follow case: 1. You have a long-lived `ViewModel` class. Could be a singleton, etc. 2. Data-bind a `View` to this `ViewModel`. 3. The `ViewModel` indefinitely held on to any object that subscribed to `PropertyChanged`. At the time, this solved a huge memory leak, because a data-bound `View` would have a reference to its parent, then to its parent, etc. Effectively this was leaking entire `Page`'s at the time. Unfortunately, there was still a flaw in this change... `WeakPropertyChangedProxy` hangs around forever instead! I could reproduce this problem in unit tests, by accessing various internal members through reflection -- asserting they were alive or not. We do have another layer of indirection, where other objects are GC'd that can free the `WeakPropertyChangedProxy`, such as: // Regular Binding ~BindingExpressionPart() => _listener?.Unsubscribe(); // TypedBinding ~PropertyChangedProxy() => Listener?.Unsubscribe(); This means it would take two GC's for these objects to go away, but it is better than the alternative -- they *can* actually go away now. After testing apps with this change, sometimes I would get an `InvalidOperationException` in `WeakReference<T>`: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs,103 So I added a parameter to `Unsubscribe(finalizer: true)`, to skip `WeakReference<T>.SetTarget()` from finalizers. After this change, I still found an issue! In my new unit test, the following would hold onto a `ViemModel` object forever: VisualElement.BackgroundProperty.DefaultValue This held the value of `Brush.Default`, in which `Brush.Default.BindingContext` was the `ViewModel`! My first thought was for `Brush.Default` to return `ImmutableBrush`: public static Brush Default => defaultBrush ??= new ImmutableBrush(null); Because anyone could do `Brush.Default.Color = Colors.Red` if they liked. When this didn't fix it, I found the underlying `_inheritedContext` is what held a reference to my `ViewModel` object. I changed this value to a `WeakReference`. The types of leaks this fixes: * Bindings to application-lifetime, singleton `ViewModel`s * Scrolling `CollectionView`, `ListView`, etc. with data-bindings. * Styles that were used on a `View` or `Page` that is now removed from the screen via navigation, de-parenting, etc. Ok, I really think the leaks are gone now. Maybe?
- Loading branch information