From aa60e5d2f9c99598aaee5f90e8b835e21e18a59a Mon Sep 17 00:00:00 2001 From: Julien Lebosquain Date: Wed, 24 Apr 2024 09:28:04 +0200 Subject: [PATCH] Fix binding related memory leaks (#15485) --- .../AvaloniaPropertyAccessorNode.cs | 17 +- .../Core/ExpressionNodes/MethodCommandNode.cs | 9 +- .../PropertyInfoAccessorFactory.cs | 25 ++- .../Avalonia.LeakTests/AvaloniaObjectTests.cs | 167 +++++++++++++++++- 4 files changed, 198 insertions(+), 20 deletions(-) diff --git a/src/Avalonia.Base/Data/Core/ExpressionNodes/AvaloniaPropertyAccessorNode.cs b/src/Avalonia.Base/Data/Core/ExpressionNodes/AvaloniaPropertyAccessorNode.cs index d996053c2c..09f4c9be26 100644 --- a/src/Avalonia.Base/Data/Core/ExpressionNodes/AvaloniaPropertyAccessorNode.cs +++ b/src/Avalonia.Base/Data/Core/ExpressionNodes/AvaloniaPropertyAccessorNode.cs @@ -1,17 +1,18 @@ using System; using System.Collections.Generic; using System.Text; +using Avalonia.Utilities; namespace Avalonia.Data.Core.ExpressionNodes; -internal sealed class AvaloniaPropertyAccessorNode : ExpressionNode, ISettableNode +internal sealed class AvaloniaPropertyAccessorNode : + ExpressionNode, + ISettableNode, + IWeakEventSubscriber { - private readonly EventHandler _onValueChanged; - public AvaloniaPropertyAccessorNode(AvaloniaProperty property) { Property = property; - _onValueChanged = OnValueChanged; } public AvaloniaProperty Property { get; } @@ -39,7 +40,7 @@ internal sealed class AvaloniaPropertyAccessorNode : ExpressionNode, ISettableNo { if (source is AvaloniaObject newObject) { - newObject.PropertyChanged += _onValueChanged; + WeakEvents.AvaloniaPropertyChanged.Subscribe(newObject, this); SetValue(newObject.GetValue(Property)); } } @@ -47,12 +48,12 @@ internal sealed class AvaloniaPropertyAccessorNode : ExpressionNode, ISettableNo protected override void Unsubscribe(object oldSource) { if (oldSource is AvaloniaObject oldObject) - oldObject.PropertyChanged -= _onValueChanged; + WeakEvents.AvaloniaPropertyChanged.Unsubscribe(oldObject, this); } - private void OnValueChanged(object? source, AvaloniaPropertyChangedEventArgs e) + public void OnEvent(object? sender, WeakEvent ev, AvaloniaPropertyChangedEventArgs e) { - if (e.Property == Property && source is AvaloniaObject o) + if (e.Property == Property && Source is AvaloniaObject o) SetValue(o.GetValue(Property)); } } diff --git a/src/Avalonia.Base/Data/Core/ExpressionNodes/MethodCommandNode.cs b/src/Avalonia.Base/Data/Core/ExpressionNodes/MethodCommandNode.cs index 6eccd52381..76ea564320 100644 --- a/src/Avalonia.Base/Data/Core/ExpressionNodes/MethodCommandNode.cs +++ b/src/Avalonia.Base/Data/Core/ExpressionNodes/MethodCommandNode.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.ComponentModel; using System.Text; using System.Windows.Input; +using Avalonia.Utilities; namespace Avalonia.Data.Core.ExpressionNodes; @@ -10,7 +11,7 @@ namespace Avalonia.Data.Core.ExpressionNodes; /// A node in an which converts methods to an /// . /// -internal sealed class MethodCommandNode : ExpressionNode +internal sealed class MethodCommandNode : ExpressionNode, IWeakEventSubscriber { private readonly string _methodName; private readonly Action _execute; @@ -41,7 +42,7 @@ internal sealed class MethodCommandNode : ExpressionNode protected override void OnSourceChanged(object source, Exception? dataValidationError) { if (source is INotifyPropertyChanged newInpc) - newInpc.PropertyChanged += OnPropertyChanged; + WeakEvents.ThreadSafePropertyChanged.Subscribe(newInpc, this); _command = new Command(source, _execute, _canExecute); SetValue(_command); @@ -50,10 +51,10 @@ internal sealed class MethodCommandNode : ExpressionNode protected override void Unsubscribe(object oldSource) { if (oldSource is INotifyPropertyChanged oldInpc) - oldInpc.PropertyChanged -= OnPropertyChanged; + WeakEvents.ThreadSafePropertyChanged.Unsubscribe(oldInpc, this); } - private void OnPropertyChanged(object? sender, PropertyChangedEventArgs e) + public void OnEvent(object? sender, WeakEvent ev, PropertyChangedEventArgs e) { if (string.IsNullOrEmpty(e.PropertyName) || _dependsOnProperties.Contains(e.PropertyName)) { diff --git a/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs b/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs index 6c00df5b62..e201472a9d 100644 --- a/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs +++ b/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs @@ -21,11 +21,10 @@ namespace Avalonia.Markup.Xaml.MarkupExtensions.CompiledBindings => new IndexerAccessor(target, property, argument); } - internal class AvaloniaPropertyAccessor : PropertyAccessorBase + internal class AvaloniaPropertyAccessor : PropertyAccessorBase, IWeakEventSubscriber { private readonly WeakReference _reference; private readonly AvaloniaProperty _property; - private IDisposable? _subscription; public AvaloniaPropertyAccessor(WeakReference reference, AvaloniaProperty property) { @@ -56,15 +55,31 @@ namespace Avalonia.Markup.Xaml.MarkupExtensions.CompiledBindings return false; } + public void OnEvent(object? sender, WeakEvent ev, AvaloniaPropertyChangedEventArgs e) + { + if (e.Property == _property) + { + PublishValue(Value); + } + } + protected override void SubscribeCore() { - _subscription = Instance?.GetObservable(_property).Subscribe(PublishValue); + if (_reference.TryGetTarget(out var reference)) + { + var value = reference.GetValue(_property); + PublishValue(value); + + WeakEvents.AvaloniaPropertyChanged.Subscribe(reference, this); + } } protected override void UnsubscribeCore() { - _subscription?.Dispose(); - _subscription = null; + if (_reference.TryGetTarget(out var reference)) + { + WeakEvents.AvaloniaPropertyChanged.Unsubscribe(reference, this); + } } } diff --git a/tests/Avalonia.LeakTests/AvaloniaObjectTests.cs b/tests/Avalonia.LeakTests/AvaloniaObjectTests.cs index 19208b15f3..5c012ac725 100644 --- a/tests/Avalonia.LeakTests/AvaloniaObjectTests.cs +++ b/tests/Avalonia.LeakTests/AvaloniaObjectTests.cs @@ -1,5 +1,15 @@ -using System; +#nullable enable + +using System; +using System.Collections.Generic; +using System.ComponentModel; using System.Reactive.Subjects; +using System.Runtime.CompilerServices; +using Avalonia.Controls; +using Avalonia.Data; +using Avalonia.Data.Core; +using Avalonia.Markup.Xaml.MarkupExtensions; +using Avalonia.Markup.Xaml.MarkupExtensions.CompiledBindings; using Avalonia.Threading; using JetBrains.dotMemoryUnit; using Xunit; @@ -30,7 +40,7 @@ namespace Avalonia.LeakTests var weakSource = setupBinding(); - GC.Collect(); + CollectGarbage(); Assert.Equal("foo", target.Foo); Assert.True(weakSource.IsAlive); @@ -56,11 +66,135 @@ namespace Avalonia.LeakTests }; completeSource(); + CollectGarbage(); + Assert.False(weakSource.IsAlive); + } + + [Fact] + public void CompiledBinding_To_InpcProperty_With_Alive_Source_Does_Not_Keep_Target_Alive() + { + var source = new Class2 { Foo = "foo" }; + + WeakReference SetupBinding() + { + var path = new CompiledBindingPathBuilder() + .Property( + new ClrPropertyInfo( + nameof(Class2.Foo), + target => ((Class2)target).Foo, + (target, value) => ((Class2)target).Foo = (string?)value, + typeof(string)), + PropertyInfoAccessorFactory.CreateInpcPropertyAccessor) + .Build(); + + var target = new TextBlock(); + + target.Bind(TextBlock.TextProperty, new CompiledBindingExtension + { + Source = source, + Path = path + }); + + return new WeakReference(target); + } + + var weakTarget = SetupBinding(); + + CollectGarbage(); + Assert.False(weakTarget.IsAlive); + } + + [Fact] + public void CompiledBinding_To_AvaloniaProperty_With_Alive_Source_Does_Not_Keep_Target_Alive() + { + var source = new StyledElement { Name = "foo" }; + + WeakReference SetupBinding() + { + var path = new CompiledBindingPathBuilder() + .Property(StyledElement.NameProperty, PropertyInfoAccessorFactory.CreateAvaloniaPropertyAccessor) + .Build(); + + var target = new TextBlock(); + + target.Bind(TextBlock.TextProperty, new CompiledBindingExtension + { + Source = source, + Path = path + }); + + return new WeakReference(target); + } + + var weakTarget = SetupBinding(); + + CollectGarbage(); + Assert.False(weakTarget.IsAlive); + } + + [Fact] + public void CompiledBinding_To_Method_With_Alive_Source_Does_Not_Keep_Target_Alive() + { + var source = new Class1(); + + WeakReference SetupBinding() + { + var path = new CompiledBindingPathBuilder() + .Command( + nameof(Class1.DoSomething), + (o, _) => ((Class1) o).DoSomething(), + (_, _) => true, + []) + .Build(); + + var target = new Button(); + + target.Bind(Button.CommandProperty, new CompiledBindingExtension + { + Source = source, + Path = path + }); + + return new WeakReference(target); + } + + var weakTarget = SetupBinding(); + + CollectGarbage(); + Assert.False(weakTarget.IsAlive); + } + + [Fact] + public void Binding_To_AttachedProperty_With_Alive_Source_Does_Not_Keep_Target_Alive() + { + var source = new StyledElement { Name = "foo" }; + + WeakReference SetupBinding() + { + var target = new TextBlock(); + + target.Bind(TextBlock.TextProperty, new Binding + { + Source = source, + Path = "(Grid.Row)", + TypeResolver = (_, name) => name == "Grid" ? typeof(Grid) : throw new NotSupportedException() + }); + + return new WeakReference(target); + } + + var weakTarget = SetupBinding(); + + CollectGarbage(); + Assert.False(weakTarget.IsAlive); + } + + private static void CollectGarbage() + { GC.Collect(); // Forces WeakEvent compact Dispatcher.UIThread.RunJobs(); GC.Collect(); - Assert.False(weakSource.IsAlive); } private class Class1 : AvaloniaObject @@ -83,6 +217,33 @@ namespace Avalonia.LeakTests get { return _foo; } set { SetAndRaise(FooProperty, ref _foo, value); } } + + public void DoSomething() + { + } + } + + private sealed class Class2 : INotifyPropertyChanged + { + private string? _foo; + + public string? Foo + { + get => _foo; + set + { + if (_foo != value) + { + _foo = value; + OnPropertyChanged(); + } + } + } + + public event PropertyChangedEventHandler? PropertyChanged; + + private void OnPropertyChanged([CallerMemberName] string? propertyName = null) + => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); } } }