From b0c79e30948db9020ed9c71e1d8dc6a2440c5ed4 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 3 Apr 2020 16:12:16 +0200 Subject: [PATCH 1/7] Added failing leak test for #3738. --- tests/Avalonia.LeakTests/ControlTests.cs | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/Avalonia.LeakTests/ControlTests.cs b/tests/Avalonia.LeakTests/ControlTests.cs index 5cec5c134d..de3f0f8189 100644 --- a/tests/Avalonia.LeakTests/ControlTests.cs +++ b/tests/Avalonia.LeakTests/ControlTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.Remoting.Contexts; using Avalonia.Controls; using Avalonia.Controls.Templates; using Avalonia.Diagnostics; @@ -419,6 +420,37 @@ namespace Avalonia.LeakTests } } + [Fact] + public void Context_MenuItems_Are_Freed() + { + using (Start()) + { + void BuildAndShowContextMenu(Control control) + { + var contextMenu = new ContextMenu + { + Items = new[] + { + new MenuItem { Header = "Foo" }, + new MenuItem { Header = "Foo" }, + } + }; + + contextMenu.Open(control); + contextMenu.Close(); + } + + var window = new Window(); + BuildAndShowContextMenu(window); + BuildAndShowContextMenu(window); + + dotMemory.Check(memory => + Assert.Equal(0, memory.GetObjects(where => where.Type.Is()).ObjectsCount)); + dotMemory.Check(memory => + Assert.Equal(0, memory.GetObjects(where => where.Type.Is()).ObjectsCount)); + } + } + private IDisposable Start() { return UnitTestApplication.Start(TestServices.StyledWindow); From b42458be76891a08968f0c9fc516889a4e0dd24f Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 3 Apr 2020 23:19:42 +0200 Subject: [PATCH 2/7] Input manager subscription may be null. As we accept a null `InputManager` for unit testing. --- src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs b/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs index bf811ad008..3715bc52a4 100644 --- a/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs +++ b/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs @@ -96,7 +96,7 @@ namespace Avalonia.Controls.Platform root.Deactivated -= WindowDeactivated; } - _inputManagerSubscription!.Dispose(); + _inputManagerSubscription?.Dispose(); Menu = null; _root = null; From 76b4d17abf0de768fc9504fc1b8ef9967466429c Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 3 Apr 2020 23:21:06 +0200 Subject: [PATCH 3/7] Remove ContextMenu from logical tree. When the `ContextMenu` is not attached to a control, i.e. it is shown using `Show(control)` then we need to detach it from the logical tree once hidden otherwise it will leak, causing #3738. --- src/Avalonia.Controls/ContextMenu.cs | 41 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/Avalonia.Controls/ContextMenu.cs b/src/Avalonia.Controls/ContextMenu.cs index fc56e26add..0ee906fb55 100644 --- a/src/Avalonia.Controls/ContextMenu.cs +++ b/src/Avalonia.Controls/ContextMenu.cs @@ -20,6 +20,7 @@ namespace Avalonia.Controls private static readonly ITemplate DefaultPanel = new FuncTemplate(() => new StackPanel { Orientation = Orientation.Vertical }); private Popup _popup; + private bool _attachedToControl; /// /// Initializes a new instance of the class. @@ -69,13 +70,15 @@ namespace Avalonia.Controls { var control = (Control)e.Sender; - if (e.OldValue != null) + if (e.OldValue is ContextMenu oldMenu) { control.PointerReleased -= ControlPointerReleased; + oldMenu._attachedToControl = false; } - if (e.NewValue != null) + if (e.NewValue is ContextMenu newMenu) { + newMenu._attachedToControl = true; control.PointerReleased += ControlPointerReleased; } } @@ -145,18 +148,6 @@ namespace Avalonia.Controls return new MenuItemContainerGenerator(this); } - private void CloseCore() - { - SelectedIndex = -1; - IsOpen = false; - - RaiseEvent(new RoutedEventArgs - { - RoutedEvent = MenuClosedEvent, - Source = this, - }); - } - private void PopupOpened(object sender, EventArgs e) { Focus(); @@ -164,17 +155,27 @@ namespace Avalonia.Controls private void PopupClosed(object sender, EventArgs e) { - var contextMenu = (sender as Popup)?.Child as ContextMenu; - - if (contextMenu != null) + foreach (var i in LogicalChildren) { - foreach (var i in contextMenu.GetLogicalChildren().OfType()) + if (i is MenuItem menuItem) { - i.IsSubMenuOpen = false; + menuItem.IsSubMenuOpen = false; } + } + + SelectedIndex = -1; + IsOpen = false; - contextMenu.CloseCore(); + if (!_attachedToControl) + { + ((ISetLogicalParent)_popup).SetParent(null); } + + RaiseEvent(new RoutedEventArgs + { + RoutedEvent = MenuClosedEvent, + Source = this, + }); } private static void ControlPointerReleased(object sender, PointerReleasedEventArgs e) From 323cfdca3a9014aa8ef2ded007c6031350477a46 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Sat, 4 Apr 2020 00:23:45 +0200 Subject: [PATCH 4/7] Make ContextMenu leak tests fail again. By adding focus/input manager. --- src/Avalonia.Controls/WindowBase.cs | 2 +- tests/Avalonia.LeakTests/ControlTests.cs | 10 +++++++++- tests/Avalonia.UnitTests/MockWindowingPlatform.cs | 4 ++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Controls/WindowBase.cs b/src/Avalonia.Controls/WindowBase.cs index 196110edf7..63eabb32f4 100644 --- a/src/Avalonia.Controls/WindowBase.cs +++ b/src/Avalonia.Controls/WindowBase.cs @@ -255,7 +255,7 @@ namespace Avalonia.Controls if (scope != null) { - FocusManager.Instance.SetFocusScope(scope); + FocusManager.Instance?.SetFocusScope(scope); } IsActive = true; diff --git a/tests/Avalonia.LeakTests/ControlTests.cs b/tests/Avalonia.LeakTests/ControlTests.cs index de3f0f8189..f93b763738 100644 --- a/tests/Avalonia.LeakTests/ControlTests.cs +++ b/tests/Avalonia.LeakTests/ControlTests.cs @@ -5,6 +5,7 @@ using System.Runtime.Remoting.Contexts; using Avalonia.Controls; using Avalonia.Controls.Templates; using Avalonia.Diagnostics; +using Avalonia.Input; using Avalonia.Layout; using Avalonia.Media; using Avalonia.Platform; @@ -441,6 +442,10 @@ namespace Avalonia.LeakTests } var window = new Window(); + window.Show(); + + Assert.Same(window, FocusManager.Instance.Current); + BuildAndShowContextMenu(window); BuildAndShowContextMenu(window); @@ -453,7 +458,10 @@ namespace Avalonia.LeakTests private IDisposable Start() { - return UnitTestApplication.Start(TestServices.StyledWindow); + return UnitTestApplication.Start(TestServices.StyledWindow.With( + focusManager: new FocusManager(), + keyboardDevice: () => new KeyboardDevice(), + inputManager: new InputManager())); } private class Node diff --git a/tests/Avalonia.UnitTests/MockWindowingPlatform.cs b/tests/Avalonia.UnitTests/MockWindowingPlatform.cs index a6701ef655..782e4a0974 100644 --- a/tests/Avalonia.UnitTests/MockWindowingPlatform.cs +++ b/tests/Avalonia.UnitTests/MockWindowingPlatform.cs @@ -21,6 +21,10 @@ namespace Avalonia.UnitTests { var win = Mock.Of(x => x.Scaling == 1); var mock = Mock.Get(win); + mock.Setup(x => x.Show()).Callback(() => + { + mock.Object.Activated?.Invoke(); + }); mock.Setup(x => x.CreatePopup()).Returns(() => { if (popupImpl != null) From 1cb7d830c78c50385cdc4130cdec4d68b91bb99d Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Sat, 4 Apr 2020 00:32:08 +0200 Subject: [PATCH 5/7] Hack to reset focus when context menu closed. Including a fix to `FocusManager` to ensure that it doesn't try to set focus to an invisble focus scope. Fixes #3738. --- src/Avalonia.Controls/ContextMenu.cs | 5 +++++ src/Avalonia.Input/FocusManager.cs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Avalonia.Controls/ContextMenu.cs b/src/Avalonia.Controls/ContextMenu.cs index 0ee906fb55..61e4b69c9b 100644 --- a/src/Avalonia.Controls/ContextMenu.cs +++ b/src/Avalonia.Controls/ContextMenu.cs @@ -21,6 +21,7 @@ namespace Avalonia.Controls new FuncTemplate(() => new StackPanel { Orientation = Orientation.Vertical }); private Popup _popup; private bool _attachedToControl; + private IInputElement _previousFocus; /// /// Initializes a new instance of the class. @@ -150,6 +151,7 @@ namespace Avalonia.Controls private void PopupOpened(object sender, EventArgs e) { + _previousFocus = FocusManager.Instance?.Current; Focus(); } @@ -171,6 +173,9 @@ namespace Avalonia.Controls ((ISetLogicalParent)_popup).SetParent(null); } + // HACK: Reset the focus when the popup is closed. We need to fix this so it's automatic. + FocusManager.Instance?.Focus(_previousFocus); + RaiseEvent(new RoutedEventArgs { RoutedEvent = MenuClosedEvent, diff --git a/src/Avalonia.Input/FocusManager.cs b/src/Avalonia.Input/FocusManager.cs index 500677c545..bcae8a3c53 100644 --- a/src/Avalonia.Input/FocusManager.cs +++ b/src/Avalonia.Input/FocusManager.cs @@ -168,7 +168,7 @@ namespace Avalonia.Input { var scope = control as IFocusScope; - if (scope != null) + if (scope != null && control.VisualRoot?.IsVisible == true) { yield return scope; } From c4da0f75b805a3c9aa6c7dfc78c2c7ea95077678 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Sat, 4 Apr 2020 19:41:54 +0200 Subject: [PATCH 6/7] Added another failing ContextMenu leak test. --- tests/Avalonia.LeakTests/ControlTests.cs | 38 +++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/tests/Avalonia.LeakTests/ControlTests.cs b/tests/Avalonia.LeakTests/ControlTests.cs index f93b763738..0afb2465ee 100644 --- a/tests/Avalonia.LeakTests/ControlTests.cs +++ b/tests/Avalonia.LeakTests/ControlTests.cs @@ -422,7 +422,43 @@ namespace Avalonia.LeakTests } [Fact] - public void Context_MenuItems_Are_Freed() + public void Attached_ContextMenu_Is_Freed() + { + using (Start()) + { + void AttachShowAndDetachContextMenu(Control control) + { + var contextMenu = new ContextMenu + { + Items = new[] + { + new MenuItem { Header = "Foo" }, + new MenuItem { Header = "Foo" }, + } + }; + + control.ContextMenu = contextMenu; + contextMenu.Open(control); + contextMenu.Close(); + control.ContextMenu = null; + } + + var window = new Window(); + window.Show(); + + Assert.Same(window, FocusManager.Instance.Current); + + AttachShowAndDetachContextMenu(window); + + dotMemory.Check(memory => + Assert.Equal(0, memory.GetObjects(where => where.Type.Is()).ObjectsCount)); + dotMemory.Check(memory => + Assert.Equal(0, memory.GetObjects(where => where.Type.Is()).ObjectsCount)); + } + } + + [Fact] + public void Standalone_ContextMenu_Is_Freed() { using (Start()) { From c4433369281c6aaed7d2b7278dc07f5fb76764bc Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Sat, 4 Apr 2020 19:48:06 +0200 Subject: [PATCH 7/7] Fix leaks in attached context menu, --- src/Avalonia.Controls/ContextMenu.cs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/Avalonia.Controls/ContextMenu.cs b/src/Avalonia.Controls/ContextMenu.cs index 61e4b69c9b..bb9b853c28 100644 --- a/src/Avalonia.Controls/ContextMenu.cs +++ b/src/Avalonia.Controls/ContextMenu.cs @@ -20,7 +20,7 @@ namespace Avalonia.Controls private static readonly ITemplate DefaultPanel = new FuncTemplate(() => new StackPanel { Orientation = Orientation.Vertical }); private Popup _popup; - private bool _attachedToControl; + private Control _attachedControl; private IInputElement _previousFocus; /// @@ -74,12 +74,13 @@ namespace Avalonia.Controls if (e.OldValue is ContextMenu oldMenu) { control.PointerReleased -= ControlPointerReleased; - oldMenu._attachedToControl = false; + oldMenu._attachedControl = null; + ((ISetLogicalParent)oldMenu._popup).SetParent(null); } if (e.NewValue is ContextMenu newMenu) { - newMenu._attachedToControl = true; + newMenu._attachedControl = control; control.PointerReleased += ControlPointerReleased; } } @@ -95,8 +96,18 @@ namespace Avalonia.Controls /// The control. public void Open(Control control) { - if (control == null) + if (control is null && _attachedControl is null) + { throw new ArgumentNullException(nameof(control)); + } + + if (control is object && _attachedControl is object && control != _attachedControl) + { + throw new ArgumentException( + "Cannot show ContentMenu on a different control to the one it is attached to.", + nameof(control)); + } + if (IsOpen) { return; @@ -168,7 +179,7 @@ namespace Avalonia.Controls SelectedIndex = -1; IsOpen = false; - if (!_attachedToControl) + if (_attachedControl is null) { ((ISetLogicalParent)_popup).SetParent(null); }