# A memory leak issue with WPF Command Binding

Background

In our application, we have a screen which hosts several tabs. In each tab, it contains a third-party  GridControl (like the WPF standard GridView control). And we need to display some column cells as hyper link, so user can click on it, view more details. So we have customized the Cell template as the following:

<DataTemplate x:Key="CellHyperlinkTemplate" >
<Button  IsHitTestVisible="True" VerticalAlignment="Center" Content="{Binding Value}" Command="{x:Static gridView:BaseView.GridClickCommand}" CommandParameter="{Binding RelativeSource={RelativeSource Self}}" ClickMode="Press">
<Button.Template>
<ControlTemplate TargetType="{x:Type Button}">
<GridLayout Style="{DynamicResource defaultCellContentStyle}" Background="Transparent">
< GridLayout.ColumnDefinitions>
<ColumnDefinition Width="Auto"/>
</GridLayout.ColumnDefinitions>
<TextBlock Name="text" Foreground="#FF2A6DBD" Text="{TemplateBinding Content}" VerticalAlignment="Center" TextDecorations="Underline"/>
</GridLayout>
<ControlTemplate.Triggers>
<Trigger Property="IsMouseOver" Value="true">
<Setter Property="Foreground" Value="#FF5E9AE2" TargetName="text"/>
<Setter Property="Cursor" Value="Hand" />
</Trigger>
</ControlTemplate.Triggers>
</ControlTemplate>
</Button.Template>
</Button>
</DataTemplate>  

It is straightforward. Only one place need to note is we have binding the Button::Command to a static command.

At the beginning, the GridClickCommand is defining as a RoutedUICommand.

public class BaseView : DevExpress.Xpf.Grid.GridControl
{
public static RoutedUICommand GridClickCommand = new RoutedUICommand();

public BaseView() : base()
{
} 

This is fine, until we have migrated the third party, and find the application is slow down after open the screens several times. The more times it opened the worse of the performance.

After struggled several weeks, we located the root problem is we misused the RoutedUICommand. We still have no idea why the problem didn’t happen before migrate the third party. But we can explain why the whole application performance slows down after open the screen.

# Reason for the Performance Issue:

As I mentioned above, there are several hyper link cells in each tab, which binding to the RoutedUICommand. Let’s look down how RoutedCommand (the base class of RoutedUICommand) implements the CanExecuteChanged method

public event EventHandler CanExecuteChanged
{
add { CommandManager.RequerySuggested += value;}
remove { CommandManager.RequerySuggested -= value;}
}  

The purpose of above implementation is obviously, the command binding source (aka Button in this case) need update its state, which depends on the Command’s CanExecute method. When the Command’s CanExecute method returns True, the button displays in Enable state, otherwise in Disable state. The problem is who will notice the binding source that the CanExecute value has been changed. One possible solution is by ourselves, when we know the CanExecute value has been changed, we explicitly raise the CanExecuteChanged event, so the button updates its state accordingly.

However the disadvantage of above approach is obviously, you need to raise the CanExecuteChanged event in every place that will affect the CanExecute value. So the common approach in WPF is, we delegate it to CommandManager, that the reason why we register the event handler to CommandManager.RequerySuggested.

With the CommandManager, WPF can automatically ask all of the commands being used in your UI if they can execute. This happens at various times, such as when input focus shifts to another control, an item is selected in a list, etc. Refer more detail about Command and CommandManager from here.

Let’s come back to the reason why RoutedUICommand slows down the performance.  It’s simple, because it is a RoutedCommand, which need to route through the element tree to determine whether it should return True or False for its CanExecute method. So it’s very expensive. That’s the reason why the more the screen opened, the worse performance it is. Because, as the number of bindinged RoutedUICommand increase, the CommandManager takes more time to execute its update status logic. And even with your simple click, the logic could be execute, so it affects the whole application’s performance.

## In our scenario, there is totally no need to update status for button; it should be always kept in enable state. Not sure why we use the RoutedUICommand before. The fix is simple; we replace it with customized DelegateCommand, with the following implement.

public class DelegateCommand : ICommand
{
private readonly Predicate<object> _canExecute;
private readonly Action<object> _execute;

public event EventHandler CanExecuteChanged;

public DelegateCommand(Action<object> execute): this(execute, null) { }

public DelegateCommand(Action<object> execute, Predicate<object> canExecute)
{
_execute = execute;
_canExecute = canExecute;
}

public bool CanExecute(object parameter)
{
if (_canExecute == null)
return true;
return _canExecute(parameter);
}

public void Execute(object parameter) { _execute(parameter); }

public void RaiseCanExecuteChanged()
{
if (CanExecuteChanged != null){
CanExecuteChanged(this, EventArgs.Empty);
}
}  

Memory Leak Issue

Several days later, we notice there is a memory leak issue during doing the release check out. By leverage WinDbg, we could see the leak is related to above change. The GridClickCommand  command is static, and its field  CanExecuteChanged property keeps reference to event handlers, and indirectly reference to the binding source (the button), which caused the whole virtual tree of the screen could not be disposed.

Solution for the Memory Leak Issue

After google, I found this useful ticket, and resolved the problem with the following solution.

internal class NoReferenceDelegateCommand : ICommand
{
protected readonly Action<object> _execute;

public NoReferenceDelegateCommand(Action<object> execute) { _execute = execute; }
public bool CanExecute(object parameter) { return true; }
public event EventHandler CanExecuteChanged  {  add { }  remove { } }
public void Execute(object parameter) { _execute(parameter); }}  

The key point is implementing a dumb CanExecuteChanged, to avoid the binding source hock on this event. In our scenario, it is fine, since the command is always executable, no need to hock the CanExecuteChanged event, no need to raise this event.

More Explanation

Firstly, let’s check why the DelegateCommand causes the memory leak. As some guys said, there is no secret behind the source code. We could get the answer from the .Net Framework source code. For the button, in order to update its state correctly, it needs to hock up the command’s CanExecuteChanged event, and waiting for the event raised. So when you assign a command to button in XAML, it will automatically hock it for us.

static ButtonBase()
{
….
ButtonBase.CommandProperty = DependencyProperty.Register("Command", typeof(ICommand), typeof(ButtonBase), new FrameworkPropertyMetadata(null, new PropertyChangedCallback(ButtonBase.OnCommandChanged)));
….
}

private static void OnCommandChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
ButtonBase buttonBase = (ButtonBase)d;
buttonBase.OnCommandChanged((ICommand)e.OldValue, (ICommand)e.NewValue);
}
private void OnCommandChanged(ICommand oldCommand, ICommand newCommand){
if (oldCommand != null)
this.UnhookCommand(oldCommand);
if (newCommand != null)
this.HookCommand(newCommand);
}
private void HookCommand(ICommand command)
{
EventHandler value = new EventHandler(this.OnCanExecuteChanged);
ButtonBase.CanExecuteChangedHandler.SetValue(this, value);
command.CanExecuteChanged += value;
this.UpdateCanExecute();
}  

Secondly, let’s check why no such problem with RoutedUICommand approaches. The key point is it uses weak reference during hock the command’s CanExecuteChanged event.

RoutedCommand:
public event EventHandler CanExecuteChanged
{
add{   CommandManager.RequerySuggested += value;}
remove {CommandManager.RequerySuggested -= value;}
}

CommandManager:
public static event EventHandler RequerySuggested
{
}