Fix binding related memory leaks (#15485)

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<AvaloniaPropertyChangedEventArgs>
 {
-    private readonly EventHandler<AvaloniaPropertyChangedEventArgs> _onValueChanged;
-
     public AvaloniaPropertyAccessorNode(AvaloniaProperty property)
     {
         Property = property;
-        _onValueChanged = OnValueChanged;
     }
 
     public AvaloniaProperty Property { get; }
@@ -39,20 +40,20 @@ protected override void OnSourceChanged(object? source, Exception? dataValidatio
     {
         if (source is AvaloniaObject newObject)
         {
-            newObject.PropertyChanged += _onValueChanged;
+            WeakEvents.AvaloniaPropertyChanged.Subscribe(newObject, this);
             SetValue(newObject.GetValue(Property));
         }
     }
 
     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));
     }
 }

src/Avalonia.Base/Data/Core/ExpressionNodes/MethodCommandNode.cs

@@ -3,14 +3,15 @@
 using System.ComponentModel;
 using System.Text;
 using System.Windows.Input;
+using Avalonia.Utilities;
 
 namespace Avalonia.Data.Core.ExpressionNodes;
 
 /// <summary>
 /// A node in an <see cref="BindingExpression"/> which converts methods to an
 /// <see cref="ICommand"/>.
 /// </summary>
-internal sealed class MethodCommandNode : ExpressionNode
+internal sealed class MethodCommandNode : ExpressionNode, IWeakEventSubscriber<PropertyChangedEventArgs>
 {
     private readonly string _methodName;
     private readonly Action<object, object?> _execute;
@@ -41,7 +42,7 @@ public override void BuildString(StringBuilder builder)
     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 @@ protected override void OnSourceChanged(object source, Exception? dataValidation
     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))
         {

src/Markup/Avalonia.Markup.Xaml/MarkupExtensions/CompiledBindings/PropertyInfoAccessorFactory.cs

@@ -21,11 +21,10 @@ public static IPropertyAccessor CreateIndexerPropertyAccessor(WeakReference<obje
             => new IndexerAccessor(target, property, argument);
     }
 
-    internal class AvaloniaPropertyAccessor : PropertyAccessorBase
+    internal class AvaloniaPropertyAccessor : PropertyAccessorBase, IWeakEventSubscriber<AvaloniaPropertyChangedEventArgs>
     {
         private readonly WeakReference<AvaloniaObject?> _reference;
         private readonly AvaloniaProperty _property;
-        private IDisposable? _subscription;
 
         public AvaloniaPropertyAccessor(WeakReference<AvaloniaObject?> reference, AvaloniaProperty property)
         {
@@ -56,15 +55,31 @@ public override bool SetValue(object? value, BindingPriority priority)
             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);
+            }
         }
     }
 

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 @@ public void Binding_To_Direct_Property_Does_Not_Get_Collected()
 
             var weakSource = setupBinding();
 
-            GC.Collect();
+            CollectGarbage();
 
             Assert.Equal("foo", target.Foo);
             Assert.True(weakSource.IsAlive);
@@ -56,11 +66,135 @@ public void Binding_To_Direct_Property_Gets_Collected_When_Completed()
             };
 
             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 @@ public string Foo
                 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));
         }
     }
 }