Merge pull request #4823 from adirh3/fixes/hotkey-manager-leak-fix

Fix for hotkey manager memory leak

src/Avalonia.Controls/Button.cs

@@ -80,6 +80,7 @@ namespace Avalonia.Controls
 
         private ICommand _command;
         private bool _commandCanExecute = true;
+        private KeyGesture _hotkey;
 
         /// <summary>
         /// Initializes static members of the <see cref="Button"/> class.
@@ -207,6 +208,11 @@ namespace Avalonia.Controls
 
         protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
         {
+            if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control
+            {
+                HotKey = _hotkey;
+            }
+            
             base.OnAttachedToLogicalTree(e);
 
             if (Command != null)
@@ -217,6 +223,13 @@ namespace Avalonia.Controls
 
         protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
         {
+            // This will cause the hotkey manager to dispose the observer and the reference to this control
+            if (HotKey != null)
+            {
+                _hotkey = HotKey;
+                HotKey = null;
+            }
+
             base.OnDetachedFromLogicalTree(e);
 
             if (Command != null)

src/Avalonia.Controls/MenuItem.cs

@@ -102,6 +102,7 @@ namespace Avalonia.Controls
         private ICommand? _command;
         private bool _commandCanExecute = true;
         private Popup? _popup;
+        private KeyGesture _hotkey;
 
         /// <summary>
         /// Initializes static members of the <see cref="MenuItem"/> class.
@@ -337,6 +338,11 @@ namespace Avalonia.Controls
 
         protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
         {
+            if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control
+            {
+                HotKey = _hotkey;
+            }
+            
             base.OnAttachedToLogicalTree(e);
 
             if (Command != null)
@@ -347,6 +353,13 @@ namespace Avalonia.Controls
 
         protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
         {
+            // This will cause the hotkey manager to dispose the observer and the reference to this control
+            if (HotKey != null)
+            {
+                _hotkey = HotKey;
+                HotKey = null;
+            }
+
             base.OnDetachedFromLogicalTree(e);
 
             if (Command != null)

src/Avalonia.Controls/Utils/AncestorFinder.cs

@@ -47,6 +47,8 @@ namespace Avalonia.Controls.Utils
 
             public void Dispose()
             {
+                _child?.Dispose();
+                _subject.Dispose();
                 _disposable.Dispose();
             }
         }

tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs

@@ -1,9 +1,14 @@
-using Moq;
+using System;
+using System.Collections.Generic;
+using Avalonia.Collections;
 using Avalonia.Controls.Presenters;
 using Avalonia.Controls.Templates;
+using Avalonia.Data;
 using Avalonia.Input;
 using Avalonia.Platform;
 using Avalonia.Styling;
+using Avalonia.UnitTests;
+using Moq;
 using Xunit;
 using System;
 using Avalonia.Input.Raw;
@@ -53,7 +58,115 @@ namespace Avalonia.Controls.UnitTests.Utils
 
                 HotKeyManager.SetHotKey(button, null);
                 Assert.Empty(tl.KeyBindings);
+            }
+        }
+
+        [Fact]
+        public void HotKeyManager_Should_Release_Reference_When_Control_Detached()
+        {
+            using (AvaloniaLocator.EnterScope())
+            {
+                var styler = new Mock<Styler>();
+
+                AvaloniaLocator.CurrentMutable
+                    .Bind<IWindowingPlatform>().ToConstant(new WindowingPlatformMock())
+                    .Bind<IStyler>().ToConstant(styler.Object);
+
+                var gesture1 = new KeyGesture(Key.A, KeyModifiers.Control);
+
+                WeakReference reference = null;
+
+                var tl = new Window();
 
+                new Action(() =>
+                {
+                    var button = new Button();
+                    reference = new WeakReference(button, true);
+                    tl.Content = button;
+                    tl.Template = CreateWindowTemplate();
+                    tl.ApplyTemplate();
+                    tl.Presenter.ApplyTemplate();
+                    HotKeyManager.SetHotKey(button, gesture1);
+
+                    // Detach the button from the logical tree, so there is no reference to it
+                    tl.Content = null;
+                    tl.ApplyTemplate();
+                })();
+
+
+                // The button should be collected since it's detached from the listbox
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+
+                Assert.Null(reference?.Target);
+            }
+        }
+
+        [Fact]
+        public void HotKeyManager_Should_Release_Reference_When_Control_In_Item_Template_Detached()
+        {
+            using (UnitTestApplication.Start(TestServices.StyledWindow))
+            {
+                var styler = new Mock<Styler>();
+
+                AvaloniaLocator.CurrentMutable
+                    .Bind<IWindowingPlatform>().ToConstant(new WindowingPlatformMock())
+                    .Bind<IStyler>().ToConstant(styler.Object);
+
+                var gesture1 = new KeyGesture(Key.A, KeyModifiers.Control);
+
+                var weakReferences = new List<WeakReference>();
+                var tl = new Window { SizeToContent = SizeToContent.WidthAndHeight, IsVisible = true };
+                var lm = tl.LayoutManager;
+
+                var keyGestures = new AvaloniaList<KeyGesture> { gesture1 };
+                var listBox = new ListBox
+                {
+                    Width = 100,
+                    Height = 100,
+                    VirtualizationMode = ItemVirtualizationMode.None,
+                    // Create a button with binding to the KeyGesture in the template and add it to references list
+                    ItemTemplate = new FuncDataTemplate(typeof(KeyGesture), (o, scope) =>
+                    {
+                        var keyGesture = o as KeyGesture;
+                        var button = new Button
+                        {
+                            DataContext = keyGesture, [!Button.HotKeyProperty] = new Binding("")
+                        };
+                        weakReferences.Add(new WeakReference(button, true));
+                        return button;
+                    })
+                };
+                // Add the listbox and render it
+                tl.Content = listBox;
+                lm.ExecuteInitialLayoutPass();
+                listBox.Items = keyGestures;
+                lm.ExecuteLayoutPass();
+
+                // Let the button detach when clearing the source items
+                keyGestures.Clear();
+                lm.ExecuteLayoutPass();
+                
+                // Add it again to double check,and render
+                keyGestures.Add(gesture1);
+                lm.ExecuteLayoutPass();
+                
+                keyGestures.Clear();
+                lm.ExecuteLayoutPass();
+                
+                // The button should be collected since it's detached from the listbox
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                
+                Assert.True(weakReferences.Count > 0);
+                foreach (var weakReference in weakReferences)
+                {
+                    Assert.Null(weakReference.Target);
+                }
             }
         }