Merge pull request #2413 from AvaloniaUI/fixes/2390-command-leak

Prevent leaks due to subscriptions to commands

src/Avalonia.Controls/Button.cs

@@ -7,6 +7,7 @@ using System.Windows.Input;
 using Avalonia.Data;
 using Avalonia.Input;
 using Avalonia.Interactivity;
+using Avalonia.LogicalTree;
 using Avalonia.VisualTree;
 
 namespace Avalonia.Controls
@@ -160,6 +161,40 @@ namespace Avalonia.Controls
             }
         }
 
+        /// <inheritdoc/>
+        protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e)
+        {
+            base.OnDetachedFromVisualTree(e);
+
+            if (IsDefault)
+            {
+                if (e.Root is IInputElement inputElement)
+                {
+                    StopListeningForDefault(inputElement);
+                }
+            }
+        }
+
+        protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
+        {
+            base.OnAttachedToLogicalTree(e);
+
+            if (Command != null)
+            {
+                Command.CanExecuteChanged += CanExecuteChanged;
+            }
+        }
+
+        protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
+        {
+            base.OnDetachedFromLogicalTree(e);
+
+            if (Command != null)
+            {
+                Command.CanExecuteChanged -= CanExecuteChanged;
+            }
+        }
+
         /// <inheritdoc/>
         protected override void OnKeyDown(KeyEventArgs e)
         {
@@ -195,20 +230,6 @@ namespace Avalonia.Controls
             }
         }
 
-        /// <inheritdoc/>
-        protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e)
-        {
-            base.OnDetachedFromVisualTree(e);
-
-            if (IsDefault)
-            {
-                if (e.Root is IInputElement inputElement)
-                {
-                    StopListeningForDefault(inputElement);
-                }
-            }
-        }
-
         /// <summary>
         /// Invokes the <see cref="Click"/> event.
         /// </summary>
@@ -281,17 +302,17 @@ namespace Avalonia.Controls
         {
             if (e.Sender is Button button)
             {
-                var oldCommand = e.OldValue as ICommand;
+                if (((ILogical)button).IsAttachedToLogicalTree)
-                var newCommand = e.NewValue as ICommand;
-
-                if (oldCommand != null)
-                {
-                    oldCommand.CanExecuteChanged -= button.CanExecuteChanged;
-                }
-
-                if (newCommand != null)
                 {
-                    newCommand.CanExecuteChanged += button.CanExecuteChanged;
+                    if (e.OldValue is ICommand oldCommand)
+                    {
+                        oldCommand.CanExecuteChanged -= button.CanExecuteChanged;
+                    }
+
+                    if (e.NewValue is ICommand newCommand)
+                    {
+                        newCommand.CanExecuteChanged += button.CanExecuteChanged;
+                    }
                 }
 
                 button.CanExecuteChanged(button, EventArgs.Empty);

src/Avalonia.Controls/MenuItem.cs

@@ -286,6 +286,26 @@ namespace Avalonia.Controls
             return new MenuItemContainerGenerator(this);
         }
 
+        protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
+        {
+            base.OnAttachedToLogicalTree(e);
+
+            if (Command != null)
+            {
+                Command.CanExecuteChanged += CanExecuteChanged;
+            }
+        }
+
+        protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
+        {
+            base.OnDetachedFromLogicalTree(e);
+
+            if (Command != null)
+            {
+                Command.CanExecuteChanged -= CanExecuteChanged;
+            }
+        }
+
         /// <summary>
         /// Called when the <see cref="MenuItem"/> is clicked.
         /// </summary>
@@ -399,14 +419,17 @@ namespace Avalonia.Controls
         {
             if (e.Sender is MenuItem menuItem)
             {
-                if (e.OldValue is ICommand oldCommand)
+                if (((ILogical)menuItem).IsAttachedToLogicalTree)
                 {
-                    oldCommand.CanExecuteChanged -= menuItem.CanExecuteChanged;
+                    if (e.OldValue is ICommand oldCommand)
-                }
+                    {
+                        oldCommand.CanExecuteChanged -= menuItem.CanExecuteChanged;
+                    }
 
-                if (e.NewValue is ICommand newCommand)
+                    if (e.NewValue is ICommand newCommand)
-                {
+                    {
-                    newCommand.CanExecuteChanged += menuItem.CanExecuteChanged;
+                        newCommand.CanExecuteChanged += menuItem.CanExecuteChanged;
+                    }
                 }
 
                 menuItem.CanExecuteChanged(menuItem, EventArgs.Empty);

tests/Avalonia.Controls.UnitTests/ButtonTests.cs

@@ -5,6 +5,7 @@ using Avalonia.Input;
 using Avalonia.Media;
 using Avalonia.Platform;
 using Avalonia.Rendering;
+using Avalonia.UnitTests;
 using Avalonia.VisualTree;
 using Moq;
 using Xunit;
@@ -21,6 +22,7 @@ namespace Avalonia.Controls.UnitTests
             {
                 Command = command,
             };
+            var root = new TestRoot { Child = target };
 
             Assert.False(target.IsEnabled);
             command.IsEnabled = true;
@@ -215,6 +217,39 @@ namespace Avalonia.Controls.UnitTests
             Assert.True(clicked);
         }
 
+        [Fact]
+        public void Button_Does_Not_Subscribe_To_Command_CanExecuteChanged_Until_Added_To_Logical_Tree()
+        {
+            var command = new TestCommand(true);
+            var target = new Button
+            {
+                Command = command,
+            };
+
+            Assert.Equal(0, command.SubscriptionCount);
+        }
+
+        [Fact]
+        public void Button_Subscribes_To_Command_CanExecuteChanged_When_Added_To_Logical_Tree()
+        {
+            var command = new TestCommand(true);
+            var target = new Button { Command = command };
+            var root = new TestRoot { Child = target };
+
+            Assert.Equal(1, command.SubscriptionCount);
+        }
+
+        [Fact]
+        public void Button_Unsubscribes_From_Command_CanExecuteChanged_When_Removed_From_Logical_Tree()
+        {
+            var command = new TestCommand(true);
+            var target = new Button { Command = command };
+            var root = new TestRoot { Child = target };
+
+            root.Child = null;
+            Assert.Equal(0, command.SubscriptionCount);
+        }
+
         private class TestButton : Button, IRenderRoot
         {
             public TestButton()
@@ -298,6 +333,7 @@ namespace Avalonia.Controls.UnitTests
 
         private class TestCommand : ICommand
         {
+            private EventHandler _canExecuteChanged;
             private bool _enabled;
 
             public TestCommand(bool enabled)
@@ -313,12 +349,18 @@ namespace Avalonia.Controls.UnitTests
                     if (_enabled != value)
                     {
                         _enabled = value;
-                        CanExecuteChanged?.Invoke(this, EventArgs.Empty);
+                        _canExecuteChanged?.Invoke(this, EventArgs.Empty);
                     }
                 }
             }
 
-            public event EventHandler CanExecuteChanged;
+            public int SubscriptionCount { get; private set; }
+
+            public event EventHandler CanExecuteChanged
+            {
+                add { _canExecuteChanged += value; ++SubscriptionCount; }
+                remove { _canExecuteChanged -= value; --SubscriptionCount; }
+            }
 
             public bool CanExecute(object parameter) => _enabled;
 

tests/Avalonia.Controls.UnitTests/MenuItemTests.cs

@@ -1,6 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Text;
+using System.Windows.Input;
+using Avalonia.UnitTests;
 using Xunit;
 
 namespace Avalonia.Controls.UnitTests
@@ -22,5 +24,57 @@ namespace Avalonia.Controls.UnitTests
 
             Assert.False(target.Focusable);
         }
+
+        [Fact]
+        public void MenuItem_Does_Not_Subscribe_To_Command_CanExecuteChanged_Until_Added_To_Logical_Tree()
+        {
+            var command = new TestCommand();
+            var target = new MenuItem
+            {
+                Command = command,
+            };
+
+            Assert.Equal(0, command.SubscriptionCount);
+        }
+
+        [Fact]
+        public void MenuItem_Subscribes_To_Command_CanExecuteChanged_When_Added_To_Logical_Tree()
+        {
+            var command = new TestCommand();
+            var target = new MenuItem { Command = command };
+            var root = new TestRoot { Child = target };
+
+            Assert.Equal(1, command.SubscriptionCount);
+        }
+
+        [Fact]
+        public void MenuItem_Unsubscribes_From_Command_CanExecuteChanged_When_Removed_From_Logical_Tree()
+        {
+            var command = new TestCommand();
+            var target = new MenuItem { Command = command };
+            var root = new TestRoot { Child = target };
+
+            root.Child = null;
+            Assert.Equal(0, command.SubscriptionCount);
+        }
+
+        private class TestCommand : ICommand
+        {
+            private EventHandler _canExecuteChanged;
+
+            public int SubscriptionCount { get; private set; }
+
+            public event EventHandler CanExecuteChanged
+            {
+                add { _canExecuteChanged += value; ++SubscriptionCount; }
+                remove { _canExecuteChanged -= value; --SubscriptionCount; }
+            }
+
+            public bool CanExecute(object parameter) => true;
+
+            public void Execute(object parameter)
+            {
+            }
+        }
     }
 }