diff --git a/src/Controls/src/Core/BindableObject.cs b/src/Controls/src/Core/BindableObject.cs index ab9a93d90853..04424724fd5f 100644 --- a/src/Controls/src/Core/BindableObject.cs +++ b/src/Controls/src/Core/BindableObject.cs @@ -31,7 +31,7 @@ public BindableObject() readonly Dictionary _properties = new Dictionary(4); bool _applying; - object _inheritedContext; + WeakReference _inheritedContext; /// public static readonly BindableProperty BindingContextProperty = @@ -41,7 +41,7 @@ public BindableObject() /// public object BindingContext { - get => _inheritedContext ?? GetValue(BindingContextProperty); + get => _inheritedContext?.Target ?? GetValue(BindingContextProperty); set => SetValue(BindingContextProperty, value); } @@ -238,7 +238,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va if (bpContext != null && ((bpContext.Attributes & BindableContextAttributes.IsManuallySet) != 0)) return; - object oldContext = bindable._inheritedContext; + object oldContext = bindable._inheritedContext?.Target; if (ReferenceEquals(oldContext, value)) return; @@ -253,7 +253,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va } else { - bindable._inheritedContext = value; + bindable._inheritedContext = new WeakReference(value); } bindable.ApplyBindings(skipBindingContext: false, fromBindingContextChanged: true); @@ -557,7 +557,7 @@ internal void ApplyBindings(bool skipBindingContext, bool fromBindingContextChan static void BindingContextPropertyBindingChanging(BindableObject bindable, BindingBase oldBindingBase, BindingBase newBindingBase) { - object context = bindable._inheritedContext; + object context = bindable._inheritedContext?.Target; var oldBinding = oldBindingBase as Binding; var newBinding = newBindingBase as Binding; diff --git a/src/Controls/src/Core/BindingExpression.cs b/src/Controls/src/Core/BindingExpression.cs index 57bed11e3cbb..ab1fb95294f8 100644 --- a/src/Controls/src/Core/BindingExpression.cs +++ b/src/Controls/src/Core/BindingExpression.cs @@ -635,7 +635,7 @@ public void SubscribeTo(INotifyPropertyChanged source, PropertyChangedEventHandl _listener.SetTarget(listener); } - public void Unsubscribe() + public void Unsubscribe(bool finalizer = false) { INotifyPropertyChanged source; if (_source.TryGetTarget(out source) && source != null) @@ -644,6 +644,10 @@ public void Unsubscribe() if (bo != null) bo.BindingContextChanged -= _bchandler; + // If we are called from a finalizer, WeakReference.SetTarget() can throw InvalidOperationException + if (finalizer) + return; + _source.SetTarget(null); _listener.SetTarget(null); } @@ -668,6 +672,8 @@ class BindingExpressionPart readonly PropertyChangedEventHandler _changeHandler; WeakPropertyChangedProxy _listener; + ~BindingExpressionPart() => _listener?.Unsubscribe(finalizer: true); + public BindingExpressionPart(BindingExpression expression, string content, bool isIndexer = false) { _expression = expression; diff --git a/src/Controls/src/Core/Brush.cs b/src/Controls/src/Core/Brush.cs index 3cf80d7978c1..d4fc16021c19 100644 --- a/src/Controls/src/Core/Brush.cs +++ b/src/Controls/src/Core/Brush.cs @@ -7,11 +7,9 @@ namespace Microsoft.Maui.Controls [System.ComponentModel.TypeConverter(typeof(BrushTypeConverter))] public abstract partial class Brush : Element { + static ImmutableBrush defaultBrush; /// - public static Brush Default - { - get { return new SolidColorBrush(null); } - } + public static Brush Default => defaultBrush ??= new(null); public static implicit operator Brush(Color color) => new SolidColorBrush(color); diff --git a/src/Controls/src/Core/TypedBinding.cs b/src/Controls/src/Core/TypedBinding.cs index 732b1b8cd3cb..0384fd020980 100644 --- a/src/Controls/src/Core/TypedBinding.cs +++ b/src/Controls/src/Core/TypedBinding.cs @@ -251,6 +251,9 @@ class PropertyChangedProxy public BindingExpression.WeakPropertyChangedProxy Listener { get; } readonly BindingBase _binding; PropertyChangedEventHandler handler; + + ~PropertyChangedProxy() => Listener?.Unsubscribe(finalizer: true); + public INotifyPropertyChanged Part { get diff --git a/src/Controls/tests/Core.UnitTests/BindingUnitTests.cs b/src/Controls/tests/Core.UnitTests/BindingUnitTests.cs index 39c17e1bca30..babcc2c267ed 100644 --- a/src/Controls/tests/Core.UnitTests/BindingUnitTests.cs +++ b/src/Controls/tests/Core.UnitTests/BindingUnitTests.cs @@ -1,8 +1,10 @@ using System; +using System.Collections; using System.Collections.Generic; using System.ComponentModel; using System.Globalization; using System.Linq; +using System.Reflection; using System.Runtime.CompilerServices; using System.Threading.Tasks; using Xunit; @@ -1185,6 +1187,81 @@ public void ValueUpdatedWithSelfPathOnTwoWayBinding(bool isDefault) AssertNoErrorsLogged(); } + + [Theory, Category("[Binding] Complex paths")] + [InlineData(BindingMode.OneWay)] + [InlineData(BindingMode.OneWayToSource)] + [InlineData(BindingMode.TwoWay)] + public async Task WeakPropertyChangedProxyDoesNotLeak(BindingMode mode) + { + var proxies = new List(); + WeakReference weakViewModel = null, weakBindable = null; + + int i = 0; + void create () + { + if (i++ < 1024) + { + create(); + return; + } + + var binding = new Binding("Model.Model[1]"); + var bindable = new MockBindable(); + weakBindable = new WeakReference(bindable); + + var viewmodel = new ComplexMockViewModel + { + Model = new ComplexMockViewModel + { + Model = new ComplexMockViewModel() + } + }; + + weakViewModel = new WeakReference(viewmodel); + + bindable.BindingContext = viewmodel; + bindable.SetBinding(MockBindable.TextProperty, binding); + + // Access private members: + // WeakPropertyChangedProxy proxy = binding._expression._parts[i]._listener; + var flags = BindingFlags.NonPublic | BindingFlags.Instance; + var expression = binding.GetType().GetField("_expression", flags).GetValue(binding); + Assert.NotNull(expression); + var parts = expression.GetType().GetField("_parts", flags).GetValue(expression) as IEnumerable; + Assert.NotNull(parts); + foreach (var part in parts) + { + var listener = part.GetType().GetField("_listener", flags).GetValue(part); + if (listener == null) + continue; + proxies.Add(new WeakReference(listener)); + } + Assert.NotEmpty(proxies); // Should be at least 1 + }; + create(); + + await Task.Yield(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + + if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay) + Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected"); + + if (mode == BindingMode.TwoWay || mode == BindingMode.OneWayToSource) + Assert.False(weakBindable.IsAlive, "Bindable wasn't collected"); + + // WeakPropertyChangedProxy won't go away until the second GC, BindingExpressionPart unsubscribes in its finalizer + await Task.Yield(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + + foreach (var proxy in proxies) + { + Assert.False(proxy.IsAlive, "WeakPropertyChangedProxy wasn't collected"); + } + } + [Theory, Category("[Binding] Complex paths")] [InlineData(BindingMode.OneWay)] [InlineData(BindingMode.OneWayToSource)] diff --git a/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs b/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs index cbce71cb5e71..9b3695536b4e 100644 --- a/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs +++ b/src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Globalization; using System.Linq; +using System.Reflection; using System.Runtime.CompilerServices; using System.Threading.Tasks; using Microsoft.Maui.Controls.Internals; @@ -1452,7 +1453,7 @@ public async Task BindingUnsubscribesForDeadTarget() public async Task BindingDoesNotStayAliveForDeadTarget() { var viewModel = new TestViewModel(); - WeakReference bindingRef = null, buttonRef = null; + WeakReference bindingRef = null, buttonRef = null, proxyRef = null; int i = 0; Action create = null; @@ -1473,6 +1474,16 @@ public async Task BindingDoesNotStayAliveForDeadTarget() bindingRef = new WeakReference(binding); buttonRef = new WeakReference(button); + + // Access private members: + // WeakPropertyChangedProxy proxy = binding._handlers[0].Listener; + var flags = BindingFlags.NonPublic | BindingFlags.Instance; + var handlers = binding.GetType().GetField("_handlers", flags).GetValue(binding) as object[]; + Assert.NotNull(handlers); + var handler = handlers[0]; + var proxy = handler.GetType().GetProperty("Listener").GetValue(handler); + Assert.NotNull(proxy); + proxyRef = new WeakReference(proxy); }; create(); @@ -1486,6 +1497,12 @@ public async Task BindingDoesNotStayAliveForDeadTarget() Assert.False(bindingRef.IsAlive, "Binding should not be alive!"); Assert.False(buttonRef.IsAlive, "Button should not be alive!"); + + // WeakPropertyChangedProxy won't go away until the second GC, PropertyChangedProxy unsubscribes in its finalizer + await Task.Yield(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + Assert.False(proxyRef.IsAlive, "WeakPropertyChangedProxy should not be alive!"); } [Fact]